dfhack icon indicating copy to clipboard operation
dfhack copied to clipboard

Compilation fails with GCC 12.1 due to -Wuninitialized raised in jsoncpp-sub

Open bpampel opened this issue 2 years ago • 16 comments

Currently the code is not compiling correctly on my machine (Arch linux) because it raises a "-Werror=uninitialized" when compiling the jsoncpp-sub dependency (full output at the bottom).

I did a bit of research and the issue might come from Arch linux already using GCC 12.1 which introduced

-Wuninitialized warns about using uninitialized variables in member initializer lists (PR19808)

This change in GCC behavior together with the strict CMake flag -Werror of the project likely causes the compilation error, although the code itself is the same as before. If my theory is correct, this will soon affect everyone switching to GCC 12. Possible fixes would be to either remove the -Werror flag for this warning, or to fix the issue upstream.

Full error message below:

[ 36%] Building CXX object depends/jsoncpp-sub/src/lib_json/CMakeFiles/jsoncpp_lib_static.dir/json_value.cpp.o
In file included from /usr/include/c++/12.1.0/bits/exception_ptr.h:43,
                 from /usr/include/c++/12.1.0/exception:168,
                 from /usr/include/c++/12.1.0/ios:39,
                 from /usr/include/c++/12.1.0/istream:38,
                 from /usr/include/c++/12.1.0/sstream:38,
                 from /home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/include/json/assertions.h:10,
                 from /home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp:7:
In function ‘std::_Require<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std::is_move_assignable<_Tp> > std::swap(_Tp&, _Tp&) [with _Tp = Json::Value::ValueHolder]’,
    inlined from ‘void Json::Value::swapPayload(Json::Value&)’ at /home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp:530:12,
    inlined from ‘void Json::Value::swap(Json::Value&)’ at /home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp:543:14,
    inlined from ‘Json::Value::Value(Json::Value&&)’ at /home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp:492:7,
    inlined from ‘Json::Value& Json::Value::append(Json::Value&&)’ at /home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp:1151:81:
/usr/include/c++/12.1.0/bits/move.h:204:11: error: ‘<unnamed>.Json::Value::value_’ is used uninitialized [-Werror=uninitialized]
  204 |       _Tp __tmp = _GLIBCXX_MOVE(__a);
      |           ^~~~~
/home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp: In member function ‘Json::Value& Json::Value::append(Json::Value&&)’:
/home/benjamin/Repository/pkgs/dfhack/dfhack/depends/jsoncpp-sub/src/lib_json/json_value.cpp:1151:81: note: ‘<anonymous>’ declared here
 1151 |   Value& Value::append(Value&& value) { return (*this)[size()] = std::move(value); }
      |                                                                                 ^
cc1plus: all warnings being treated as errors
make[2]: *** [depends/jsoncpp-sub/src/lib_json/CMakeFiles/jsoncpp_lib_static.dir/build.make:90: depends/jsoncpp-sub/src/lib_json/CMakeFiles/jsoncpp_lib_static.dir/json_value.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1103: depends/jsoncpp-sub/src/lib_json/CMakeFiles/jsoncpp_lib_static.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

bpampel avatar Jul 26 '22 12:07 bpampel

Note: this of course applies to the master and not the develop branch. I created this small patch that adds the -Wno-uninitialized flag to the jsoncpp_lib_static to make the specific error go away… only to run into a -Werror=restricted error further down.

pbnoxious avatar Jul 26 '22 13:07 pbnoxious

@cppcooper is this the same issue you ran into? Do you remember what we did to fix the issue?

myk002 avatar Jul 26 '22 14:07 myk002

@cppcooper is this the same issue you ran into? Do you remember what we did to fix the issue?

It is not, afaict. I've rarely used dfhack/master. When I run into errors like this I usually try to correct the code since I have heavy doubts we'll ever drop the -Werror flag. There is probably some cmake tweaking @lethosor could suggest

cppcooper avatar Jul 26 '22 15:07 cppcooper

It looks like gcc 12.1 is too new for the GitHub build containers. Could you try adding -Wno-uninitialized to the json-cpp build command as in pbnoxious's patch? Does that get things working for you?

myk002 avatar Jul 26 '22 15:07 myk002

Note: this of course applies to the master and not the develop branch.

@pbnoxious why is this? I'm not aware of us making any jsoncpp changes since the last release.

I would recommend adding -Wno-restricted to address your followup issue, like so:

diff --git a/depends/CMakeLists.txt b/depends/CMakeLists.txt
index 0fcb4ca3c..28fbc99ce 100644
--- a/depends/CMakeLists.txt
+++ b/depends/CMakeLists.txt
@@ -14,7 +14,7 @@ option(JSONCPP_WITH_TESTS "Compile and (for jsoncpp_check) run JsonCpp test exec
 option(JSONCPP_WITH_POST_BUILD_UNITTEST "Automatically run unit-tests as a post build step" OFF)
 add_subdirectory(jsoncpp-sub EXCLUDE_FROM_ALL)
 if(UNIX)
-    set_target_properties(jsoncpp_lib_static PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
+    set_target_properties(jsoncpp_lib_static PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations -Wno-uninitialized -Wno-restricted")
 endif()
 # build clsocket static and only as a dependency. Setting those options here overrides its own default settings.
 option(CLSOCKET_SHARED "Build clsocket lib as shared." OFF)

lethosor avatar Jul 26 '22 15:07 lethosor

Thanks at everyone for looking at this! The reason why it doesn't compile anymore without code changes was speculated by me already: GCC 12 now also raises the -Wuninitialized flag in member initializer lists. Haven't actually looked at the code in detail so it's just speculation. If this is true, I also assume that this could be fixed upstream easily (i.e. just initialize the variable where it throws the error)

bpampel avatar Jul 27 '22 07:07 bpampel

I would recommend adding -Wno-restricted to address your followup issue, like so:

The restricted warning is not raised in the dependency but in the actual dfhack code (funnily when it includes the <string> library):

[ 48%] Building CXX object library/CMakeFiles/dfhack.dir/modules/Gui.cpp.o
In file included from /usr/include/c++/12.1.0/string:40,
                 from /home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/library/modules/Gui.cpp:28:
In static member function ‘static std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)’,
    inlined from ‘static void std::basic_string<_CharT, _Traits, _Alloc>::_M_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12.1.0/bits/cow_string.h:402:21,
    inlined from ‘std::basic_string<_CharT, _Traits, _Alloc>& std::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12.1.0/bits/cow_string.h:3297:16,
    inlined from ‘std::basic_string<_CharT, _Traits, _Alloc>& std::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12.1.0/bits/cow_string.h:1609:21,
    inlined from ‘std::basic_string<_CharT, _Traits, _Alloc> std::operator+(const _CharT*, basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = char_traits<char>; _Alloc = allocator<char>]’ at /usr/include/c++/12.1.0/bits/basic_string.h:3541:36:
/usr/include/c++/12.1.0/bits/char_traits.h:431:56: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
  431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [library/CMakeFiles/dfhack.dir/build.make:6229: library/CMakeFiles/dfhack.dir/modules/Gui.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1399: library/CMakeFiles/dfhack.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

As the restricted C stuff is outside of my experience, I don't want to go bug-chasing here. To fix it temporarily you'd likely have to disable the warning globally, I added the flag here: https://github.com/DFHack/dfhack/blob/585888c2d36593882f756c5f5de459ec6b42b9ab/CMakeLists.txt#L247

that made it compile until a -Werror=deprecated-declarations error occured (at plugins/autochop.cpp:3). As I couldn't spot that this is behaving differently in GCC 12 (same for the -Wrestrict) in the changefile, I'm not sure if the issue is just the newer GCC version that raises more / different warnings or something else.

I'm also too lazy to add more and more warnings as exceptions currently, personally I just removed the -Werror flag to have it compile (but that decision is of course up to you and not me for the project).

pbnoxious avatar Jul 27 '22 10:07 pbnoxious

that made it compile until a -Werror=deprecated-declarations error occured (at plugins/autochop.cpp:3

Mind including a more complete stack trace for this one? That line is #include "uicommon.h", which means the warning could be coming from anywhere within the file.

Anyway, we're keeping -Werror. At this time, it sounds like we haven't been able to reproduce these warnings on GCC 12, so it's difficult to fix them, but we would be happy to accept a patch that fixes them with as narrowly-scoped changes as possible.

lethosor avatar Jul 29 '22 05:07 lethosor

The restricted warning is not raised in the dependency but in the actual dfhack code (funnily when it includes the <string> library):

Actually, that is an issue within the string library itself in GCC 12.1 (i.e. not a DFHack issue): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329 maybe also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105545

lethosor avatar Jul 29 '22 05:07 lethosor

Here is some more infos for reproducing the behavior: I'm on Arch Linux with GCC 12.1.0 and the CMake flags (called from within the build directory) are

  cmake \
    -DCMAKE_INSTALL_PREFIX=/opt/dwarffortress \
    -DCMAKE_BUILD_TYPE=Release \
    -DBUILD_DOCS=ON \
    -DBUILD_STONESENSE=ON \
    -DDFHACK_BUILD_ARCH=64 \
    ..

Disabling -Wrestrict in the main CMake file and -Wuninitialized only for jsoncpp_lib_static (see the patches in the arch linux package) results in the following output

[ 55%] Building CXX object plugins/CMakeFiles/autochop.dir/autochop.cpp.o
In file included from /home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/plugins/autochop.cpp:3:
/home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/plugins/uicommon.h: In function ‘std::string& ltrim(std::string&)’:
/home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/plugins/uicommon.h:177:89: error: ‘std::pointer_to_unary_function<_Arg, _Result> std::ptr_fun(_Result (*)(_Arg)) [with _Arg = int; _Result = int]’ is deprecated: use 'std::function' instead [-Werror=deprecated-declarations]
  177 |     s.erase(s.begin(), std::find_if(s.begin(), s.end(), std::not1(std::ptr_fun<int, int>(std::isspace))));
      |                                                                   ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from /usr/include/c++/12.1.0/functional:49,
                 from /home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/plugins/uicommon.h:5:
/usr/include/c++/12.1.0/bits/stl_function.h:1126:5: note: declared here
 1126 |     ptr_fun(_Result (*__x)(_Arg))
      |     ^~~~~~~
/home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/plugins/uicommon.h: In function ‘std::string& rtrim(std::string&)’:
/home/benjamin/Repository/pkgs/test/dfhack/src/dfhack/plugins/uicommon.h:183:80: error: ‘std::pointer_to_unary_function<_Arg, _Result> std::ptr_fun(_Result (*)(_Arg)) [with _Arg = int; _Result = int]’ is deprecated: use 'std::function' instead [-Werror=deprecated-declarations]
  183 |     s.erase(std::find_if(s.rbegin(), s.rend(), std::not1(std::ptr_fun<int, int>(std::isspace))).base(), s.end());
      |                                                          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/usr/include/c++/12.1.0/bits/stl_function.h:1126:5: note: declared here
 1126 |     ptr_fun(_Result (*__x)(_Arg))
      |     ^~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [plugins/CMakeFiles/autochop.dir/build.make:76: plugins/CMakeFiles/autochop.dir/autochop.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1593: plugins/CMakeFiles/autochop.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

so the compiler warns about the usage of the deprecated std::ptr_fun in ltrim() and rtrim() in plugins/uicommon.h

If I can help further to let you reproduce the errors (If I understand it correctly, you do not get these even with GCC 12?) let me know.

pbnoxious avatar Jul 29 '22 07:07 pbnoxious

so the compiler warns about the usage of the deprecated std::ptr_fun in ltrim() and rtrim() in plugins/uicommon.h

what version of DFHack are you compiling? We removed those calls to ptr_fun in cec8a358b5, which was part of 0.47.05-r6.

(If I understand it correctly, you do not get these even with GCC 12?)

We don't get them with what Ubuntu calls GCC "12-20220319-1ubuntu1", as you can see here: https://github.com/DFHack/dfhack/runs/7560233419?check_suite_focus=true

We do not yet have the ability to install GCC 12.1 easily in our CI environment, and you appear to be the only people so far who have tried it locally. Again, we are happy to take patches against the most recent version, as long as they don't have the effect of disabling warnings across our own codebase.

lethosor avatar Jul 30 '22 16:07 lethosor

Sorry for the late reply, I'm using the master branch (as I was interested in using the stable version), which is before https://github.com/DFHack/dfhack/commit/cec8a358b5e7c34a7aa5870b762718e7958bba87 . So the second issue is likely fixed already in your code but the last release (that people that just want to play DF are probably using) is currently broken with GCC 12.1 (for me).

I guess for a throughout testing of your codebase you'd have to wait until GCC 12 becomes available.

pbnoxious avatar Aug 08 '22 07:08 pbnoxious

Oops, looks like the master branch was not updated for the most recent stable release (it was still on 0.47.05-r5). Try now - it should be updated to 0.47.05-r6, which does contain cec8a358b5e7c34a7aa5870b762718e7958bba87.

lethosor avatar Aug 08 '22 14:08 lethosor

That indeed seems to have fixed the problem, it compiles now without any problem here.

pbnoxious avatar Aug 08 '22 15:08 pbnoxious

Are you still using some patches? I don't think we've done anything that would affect the restricted errors you were seeing related to std::string in https://github.com/DFHack/dfhack/issues/2250#issuecomment-1196549459

lethosor avatar Aug 08 '22 15:08 lethosor

Yes, I still compile with the -Wno-restrict flag added in the CMakeLists.txt, but now I do not run in any additional errors.

pbnoxious avatar Aug 08 '22 16:08 pbnoxious

I believe this is fixed. Compilation works fine in gcc-13.2.1

myk002 avatar Jan 13 '24 02:01 myk002