tristanpenman / valijson

Header-only C++ library for JSON Schema validation, with support for many popular parsers
BSD 2-Clause "Simplified" License
351 stars 105 forks source link

Test compilation failure in picojson with gcc-12 due to -Werror=maybe-uninitialized #164

Closed hhoffstaette closed 2 years ago

hhoffstaette commented 2 years ago

After packaging valijson-0.7 for Gentoo without any problems using gcc-11.3 & clang-14 - including running the test suite - I was recently alerted to a new test compilation failure with gcc-12:

[1/3] /usr/bin/x86_64-pc-linux-gnu-g++ -DBOOST_ALL_DYN_LINK -DPICOJSON_USE_INT64 -DQT_CORE_LIB -DQT_NO_DEBUG -DVALIJSON_BUILD_BOOST_JSON_ADAPTER -DVALIJSON_BUILD_BOOST_PROPERTY_TREE_ADAPTER -DVALIJSON_BUILD_QT_ADAPTER -DVALIJSON_USE_EXCEPTIONS=1 -I/tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/gtest-1.11.0/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/json11-ec4e452 -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/jsoncpp-1.9.5/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/rapidjson-48fbd8c/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0 -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/nlohmann-json-3.1.2 -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/yaml-cpp-0.7.0/include -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtCore -isystem /usr/lib64/qt5/mkspecs/linux-clang -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/gtest-1.11.0/googletest/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/gtest-1.11.0/googletest  -pipe -march=native -O2 -fno-semantic-interposition -DBOOST_BIND_GLOBAL_PLACEHOLDERS -std=c++11 -Wall  -pedantic -Werror -Wshadow -Wunused -fPIC -MD -MT CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o -MF CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o.d -o CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o -c /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/tests/test_adapter_comparison.cpp
FAILED: CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o 
/usr/bin/x86_64-pc-linux-gnu-g++ -DBOOST_ALL_DYN_LINK -DPICOJSON_USE_INT64 -DQT_CORE_LIB -DQT_NO_DEBUG -DVALIJSON_BUILD_BOOST_JSON_ADAPTER -DVALIJSON_BUILD_BOOST_PROPERTY_TREE_ADAPTER -DVALIJSON_BUILD_QT_ADAPTER -DVALIJSON_USE_EXCEPTIONS=1 -I/tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/gtest-1.11.0/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/json11-ec4e452 -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/jsoncpp-1.9.5/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/rapidjson-48fbd8c/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0 -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/nlohmann-json-3.1.2 -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/yaml-cpp-0.7.0/include -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtCore -isystem /usr/lib64/qt5/mkspecs/linux-clang -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/gtest-1.11.0/googletest/include -isystem /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/gtest-1.11.0/googletest  -pipe -march=native -O2 -fno-semantic-interposition -DBOOST_BIND_GLOBAL_PLACEHOLDERS -std=c++11 -Wall  -pedantic -Werror -Wshadow -Wunused -fPIC -MD -MT CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o -MF CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o.d -o CMakeFiles/test_suite.dir/tests/test_adapter_comparison.cpp.o -c /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/tests/test_adapter_comparison.cpp
In file included from /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/tests/test_adapter_comparison.cpp:6:
In copy constructor 'picojson::value::value(const picojson::value&)',
    inlined from 'void std::__new_allocator<_Tp>::construct(_Up*, _Args&& ...) [with _Up = picojson::value; _Args = {picojson::value}; _Tp = picojson::value]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/new_allocator.h:175:4,
    inlined from 'static void std::allocator_traits<std::allocator<_CharT> >::construct(allocator_type&, _Up*, _Args&& ...) [with _Up = picojson::value; _Args = {picojson::value}; _Tp = picojson::value]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/alloc_traits.h:516:17,
    inlined from 'void std::vector<_Tp, _Alloc>::emplace_back(_Args&& ...) [with _Args = {picojson::value}; _Tp = picojson::value; _Alloc = std::allocator<picojson::value>]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/vector.tcc:117:30,
    inlined from 'void std::vector<_Tp, _Alloc>::push_back(value_type&&) [with _Tp = picojson::value; _Alloc = std::allocator<picojson::value>]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/stl_vector.h:1294:21,
    inlined from 'bool picojson::default_parse_context::parse_array_item(picojson::input<Iter>&, size_t) [with Iter = __gnu_cxx::__normal_iterator<const char*, std::__cxx11::basic_string<char> >]' at /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0/picojson.h:853:18,
    inlined from 'bool picojson::_parse_array(Context&, input<Iter>&) [with Context = default_parse_context; Iter = __gnu_cxx::__normal_iterator<const char*, std::__cxx11::basic_string<char> >]' at /tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0/picojson.h:695:33:
/tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0/picojson.h:249:10: error: '<unnamed>.picojson::value::u_' may be used uninitialized [-Werror=maybe-uninitialized]
  249 |       u_ = x.u_;
      |       ~~~^~~~~~
/tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0/picojson.h: In function 'bool picojson::_parse_array(Context&, input<Iter>&) [with Context = default_parse_context; Iter = __gnu_cxx::__normal_iterator<const char*, std::__cxx11::basic_string<char> >]':
/tmp/portage/dev-cpp/valijson-0.7/work/valijson-0.7/thirdparty/picojson-1.3.0/picojson.h:853:19: note: '<anonymous>' declared here
  853 |       a.push_back(value());
      |                   ^~~~~~~
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

This is an interesting problem for several reasons:

The obvious solution is to build everything with clang ;-) but I would still like to understand what is wrong here, but so far I have not been able to understand what valijson does to/with picojson (other than using it), and what gcc complains about. I looked at picojson.h:249 aka value::value(..) and it does some funky conditional definition that seems to trip gcc's lifetime analyser.

I realize you can't help with the Gentoo build details, but maybe you have an idea why picojson has this problem (or what it is doing here to begin with, because I don't get it) and why gcc-12 complains about it.

hhoffstaette commented 2 years ago

I was just poking around in thirdparty/picojson and can re-recreate the warning by adding -O2 to the Makefile:

In file included from test.cc:28:
In copy constructor 'picojson::value::value(const picojson::value&)',
    inlined from 'void std::__new_allocator<_Tp>::construct(_Up*, _Args&& ...) [with _Up = picojson::value; _Args = {picojson::value}; _Tp = picojson::value]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/new_allocator.h:175:4,
    inlined from 'static void std::allocator_traits<std::allocator<_CharT> >::construct(allocator_type&, _Up*, _Args&& ...) [with _Up = picojson::value; _Args = {picojson::value}; _Tp = picojson::value]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/alloc_traits.h:516:17,
    inlined from 'std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::emplace_back(_Args&& ...) [with _Args = {picojson::value}; _Tp = picojson::value; _Alloc = std::allocator<picojson::value>]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/vector.tcc:117:30,
    inlined from 'void std::vector<_Tp, _Alloc>::push_back(value_type&&) [with _Tp = picojson::value; _Alloc = std::allocator<picojson::value>]' at /usr/lib/gcc/x86_64-pc-linux-gnu/12.2.0/include/g++-v12/bits/stl_vector.h:1294:21,
    inlined from 'bool picojson::default_parse_context::parse_array_item(picojson::input<Iter>&, size_t) [with Iter = std::istreambuf_iterator<char, std::char_traits<char> >]' at picojson.h:853:18,
    inlined from 'bool picojson::_parse_array(Context&, input<Iter>&) [with Context = default_parse_context; Iter = std::istreambuf_iterator<char, std::char_traits<char> >]' at picojson.h:695:33,
    inlined from 'bool picojson::_parse(Context&, input<Iter>&) [with Context = default_parse_context; Iter = std::istreambuf_iterator<char, std::char_traits<char> >]' at picojson.h:762:26:
picojson.h:249:10: warning: '<unnamed>.picojson::value::u_' may be used uninitialized [-Wmaybe-uninitialized]
  249 |       u_ = x.u_;
      |       ~~~^~~~~~
picojson.h: In function 'bool picojson::_parse(Context&, input<Iter>&) [with Context = default_parse_context; Iter = std::istreambuf_iterator<char, std::char_traits<char> >]':
picojson.h:853:19: note: '<anonymous>' declared here
  853 |       a.push_back(value());
      |                   ^~~~~~~

Probably due to inlining/move optimizations kicking in. picojson's build uses no optimizations by default, which is probably why this has not surfaced earlier. This also explains why I haven't been able to reproduce this when building valijson from git: by default it also doesn't add any optimization flags. Simply running cmake with -DCMAKE_CXX_FLAGS="-O2" does the trick.

tristanpenman commented 2 years ago

This is a weird one, especially seeing as optimisation causes it to be detected as an error.

My best guess, from looking at picojson.h, is that the default constructor for picojson::value initialises type_, but does not initialise the value of u_. The test harness constructs an empty picojson::value before populating it using valijson::utils::loadDocument. Knowing that the default constructor is used, GCC 12 could be detecting that the value of u_ is uninitialised in the copy constructor, and complains because the result of u_ = x.u_ would therefore be undefined.

It would be interesting to see if this warning could be suppressed using a pragma, as described here: https://stackoverflow.com/a/58464018 I believe the pragma lines would have to wrap #include <picojson.h>, like they do for MSVC. So I'm thinking all picojson includes would have to look like this:

#ifdef _MSC_VER
#pragma warning(disable: 4706)
#include <picojson.h>
#pragma warning(default: 4706)
#else
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
#include <picojson.h>
#pragma GCC diagnostic pop
#endif
hhoffstaette commented 2 years ago

Thanks for looking into this!

This is a weird one, especially seeing as optimisation causes it to be detected as an error.

Enabling the optimisation only causes the warning inside picojson; the fact that this caused an error in valijson was due to -Werror use in its test suite. That's why I had to remove it for Gentoo's CI/QA.

As for the actual problem, of course u_ is a union..but it seems a simple _storage u_ = {}; seems to do the trick. \o/ Do you think this would be an acceptable solution for picojson upstream? It seems kind of dead, so not sure how to proceed here. I guess with gcc-12 hitting more distros people will encounter this problem more often.

hhoffstaette commented 2 years ago

I had another go at it and fixed all warnings (there were a few more than just the uninitialized union), so please feel free to merge: https://github.com/hhoffstaette/valijson/commit/df90bb7997e6096b54c66c49b7e8109cfa8d1e52 https://github.com/hhoffstaette/valijson/commit/51c23f96de0d6483ba25ee1936987fcfc76052da

holger>pwd
/tmp/valijson/thirdparty/picojson-1.3.0
holger>make clean && make CXXFLAGS="-pipe -O2 -pedantic"                     
rm -f test-core test-core-int64
c++ -pipe -O2 -pedantic -Wall test.cc picotest/picotest.c -o test-core
c++ -pipe -O2 -pedantic -Wall -DPICOJSON_USE_INT64 test.cc picotest/picotest.c -o test-core-int64
./test-core
ok 1 - picojson::value(true)
..
1..99
./test-core-int64
ok 1 - picojson::value(true)
..
1..125
tristanpenman commented 2 years ago

Thanks for the patches. Instead of applying these directly to the valijson repo, I've forked picojson and re-added it as a submodule. Then I applied your patches to the fork.

I'm planning to add all of the other parser libraries as submodules, so this served as a good excuse to get started on that.

You can find this on the submodule branch: https://github.com/tristanpenman/valijson/tree/submodules

Can you let me know if that branch works for you with GCC 12?

tristanpenman commented 2 years ago

FYI, the submodule changes have been merged into master

hhoffstaette commented 2 years ago

I just did a quick make CXXFLAGS="-pipe -O2 -pedantic" in the picojson module and it worked fine. Thanks!

hhoffstaette commented 2 years ago

Closing this since the problem has been found and fixed. Unfortunately master now fails because of a new problem in nlohmann-json, but that's for another day.