search

valhalla / valhalla

Open Source Routing Engine for OpenStreetMap
https://valhalla.github.io/valhalla/
Other
4.44k stars 674 forks source link

boost geometry generates warning-as-error with gcc 13.2.1 in Release mode #4673

Open nilsnolde opened 5 months ago

nilsnolde commented 5 months ago

hmpf, in #4669 we introduced some "new" boost geometry header and my compiler (gcc 13.2.1) doesn't like v1.83. it throws warnings (as errors) like

boost::geometry::cs::cartesian>::m_values[0]’ may be used uninitialized [-Werror=maybe-uninitialized]
[build]    65 |         if (less(coord, get<min_corner, Dimension>(box)))
[build]       |         ^~

and some more. note, curiously only in Release mode, not in Debug mode.

I couldn't figure out yet how to remedy (well, other than -DENABLE_SINGLE_FILES_WERROR=OFF).. declaring the target_include_directories as SYSTEM didn't work (which should've helped according to the docs).

anyone got an idea how to solve this?

nilsnolde commented 5 months ago

maybe we should have one more linux build with a more up-to-date compiler.. tests could be omitted, then we could also do it on GHA.

kevinkreiser commented 5 months ago

is it fixed if you disable -wextra?

kevinkreiser commented 5 months ago

gcc docs seem to see its enabled for wall as well..

sletuffe commented 5 months ago

Hi, Here on a fresh debian 12 (gcc 12.2) the compilation fails as well with the same kind of errors (in boost system headers) :

at /home/valhalla-source/src/mjolnir/admin.cc:284:19:
/usr/include/boost/geometry/algorithms/detail/expand/indexed.hpp:70:9: error: ‘helper_mbr.boost::geometry::model::box<boost::geometry::model::d2::point_xy<double, boost::geometry::cs::cartesian> >::m_max_corner.boost::geometry::model::d2::point_xy<double, boost::geometry::cs::cartesian>::<unnamed>.boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>::m_values[1]’ may be used uninitialized [-Werror=maybe-uninitialized]
   70 |         if (greater(coord, get<max_corner, Dimension>(box)))
      |         ^~

However, running cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_SINGLE_FILES_WERROR=OFF

Does the trick, probably by removing the -Werror flag

kevinkreiser commented 5 months ago

@nilsnolde i also thought the right solution is to mark them as system as tried. if you do verbose=1 make ... does the compiler line actually show -isystem before the directory where it gets boost stuff?

nilsnolde commented 5 months ago

when compiling admin.cc, it doesn't -I the boost directory when it fails, but the rest of the dependencies are included with -isystem:

ccache /usr/bin/g++ -DAUTO_DOWNLOAD=0 -DBOOST_ALLOW_DEPRECATED_HEADERS -DBOOST_BIND_GLOBAL_PLACEHOLDERS -DBOOST_NO_CXX11_SCOPED_ENUMS -DDATA_TOOLS -DENABLE_GDAL -DHAS_REMOTE_API=0 -DLOGGING_LEVEL_INFO -I/home/nilsnolde/dev/cpp/valhalla -I/home/nilsnolde/dev/cpp/valhalla/valhalla -I/home/nilsnolde/dev/cpp/valhalla/build/Release/src/mjolnir -I/home/nilsnolde/dev/cpp/valhalla/build/Release/src/mjolnir/valhalla -I/home/nilsnolde/dev/cpp/valhalla/build/Release/src -I/home/nilsnolde/dev/cpp/valhalla/build/Release/src/valhalla -I/usr/include/luajit-2.1 -isystem /home/nilsnolde/dev/cpp/valhalla/third_party/date/include -isystem /home/nilsnolde/dev/cpp/valhalla/third_party/rapidjson/include -isystem /home/nilsnolde/dev/cpp/valhalla/third_party/just_gtfs/include -isystem /home/nilsnolde/dev/cpp/valhalla/third_party/robin-hood-hashing/src/include -O3 -DNDEBUG -std=gnu++17 -fPIC -mfpmath=sse -msse2 -Werror -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -MD -MT src/mjolnir/CMakeFiles/valhalla-mjolnir.dir/admin.cc.o -MF src/mjolnir/CMakeFiles/valhalla-mjolnir.dir/admin.cc.o.d -o src/mjolnir/CMakeFiles/valhalla-mjolnir.dir/admin.cc.o -c /home/nilsnolde/dev/cpp/valhalla/src/mjolnir/admin.cc
In file included from /usr/include/boost/geometry/strategy/cartesian/expand_box.hpp:24,
                 from /usr/include/boost/geometry/strategy/cartesian/envelope_box.hpp:35,
                 from /usr/include/boost/geometry/strategy/cartesian/envelope.hpp:29,
                 from /usr/include/boost/geometry/strategies/cartesian/intersection.hpp:38,
                 from /usr/include/boost/geometry/strategies/intersection_strategies.hpp:25,
                 from /usr/include/boost/geometry/strategies/strategies.hpp:37,
                 from /usr/include/boost/geometry/geometry.hpp:57,
                 from /usr/include/boost/geometry.hpp:17,
                 from /home/nilsnolde/dev/cpp/valhalla/valhalla/mjolnir/admin.h:4,
                 from /home/nilsnolde/dev/cpp/valhalla/src/mjolnir/admin.cc:1:

so I wonder how it even finds boost headers in the first place😅

kevinkreiser commented 5 months ago

it finds them where they are in the default include path that comes with the compiler right where its printing the warn/error message /usr/include. that is an implicit include path you can get from gcc telling you:

$ echo | gcc -xc++ -E -v -
Using built-in specs.
COLLECT_GCC=gcc
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 11.4.0-1ubuntu1~22.04' --with-bugurl=file:///usr/share/doc/gcc-11/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-11 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --enable-cet --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none=/build/gcc-11-XeT9lY/gcc-11-11.4.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-11-XeT9lY/gcc-11-11.4.0/debian/tmp-gcn/usr --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu --with-build-config=bootstrap-lto-lean --enable-link-serialization=2
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) 
COLLECT_GCC_OPTIONS='-E' '-v' '-mtune=generic' '-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/11/cc1plus -E -quiet -v -imultiarch x86_64-linux-gnu -D_GNU_SOURCE - -mtune=generic -march=x86-64 -fasynchronous-unwind-tables -fstack-protector-strong -Wformat -Wformat-security -fstack-clash-protection -fcf-protection -dumpbase -
ignoring duplicate directory "/usr/include/x86_64-linux-gnu/c++/11"
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/11/include-fixed"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/11/../../../../x86_64-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/include/c++/11
 /usr/include/x86_64-linux-gnu/c++/11
 /usr/include/c++/11/backward
 /usr/lib/gcc/x86_64-linux-gnu/11/include
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
# 0 "<stdin>"
# 0 "<built-in>"
# 0 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "<command-line>" 2
# 1 "<stdin>"
COMPILER_PATH=/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/
LIBRARY_PATH=/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/11/../../../x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/11/../../../../lib/:/lib/x86_64-linux-gnu/:/lib/../lib/:/usr/lib/x86_64-linux-gnu/:/usr/lib/../lib/:/usr/lib/gcc/x86_64-linux-gnu/11/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-E' '-v' '-mtune=generic' '-march=x86-64'

so in this case we get boost for free really from the default system includes. however i bet that when we do FindPackage(Boost or whatever it sets the include path for boost headers to Boost_INCLUDE_DIRS. all we have to do then is actually include that in our header path manually. i would add it to src/CMakeLists.txt as a system include. hopefully it goes away then on your compiler

nilsnolde commented 5 months ago

hm don't think that'd help. we don't need to use those cmake variables anymore, it's recommended to work with targets and they know implicitly all their properties, so Boost::Boost (or whatever) will be included with target_include_directories() which is basically doing the same as what you're suggesting.

I tried to include all dependencies as SYSTEM which should've worked for boost as well. I'd also kinda expect it to be the default that stuff in /usr/include is treated as -isystem.. who'd ever want to lint system includes?!

anyways, I'll try again tmrw, maybe I was doing smth wrong..

kevinkreiser commented 5 months ago

at the end of the day what matters is the commands that cmake generates (the stuff that shows up when you do VERBOSE=1 make). given what you've posted above it is clear that this:

so Boost::Boost (or whatever) will be included with target_include_directories()

is not true because there is no reference at all to /usr/include in your output. i guess it is possible that cmake also checks if that path is in the compilers default include paths and therefor elides it from the imported Boost::Boost target?

anyway you can check by just manually running the command and seeing if it completes. copy that line from above and run it from the location where its run but add -isystem /usr/include to the command. if it completes without erroring on a warning we can try to make sure it gets in there via whatever cmake machinery makes sense.

indeed i dont really care what mechanism we use to get it in the generated command. perhaps we just need to modify the valhalla_module function to use the stuff we put in the DEPENDS when its doing the target_include_directories for a given "module". im not sure if you can use imported targets that way but maybe cmake has the magic to tell you are using it in the include path and can then extract the include dirs out of that imported target? like maybe it just knows the right thing to do.

i hope im making sense, if not we can pair on it for a minute or two whenever makes sense :smile:

nilsnolde commented 5 months ago

copy that line from above and run it from the location where its run but add -isystem /usr/include to the command

that fails properly with 

In file included from /usr/include/boost/config/platform/linux.hpp:15,
                 from /usr/include/boost/config.hpp:57,
                 from /usr/include/boost/geometry/geometry.hpp:23,
                 from /usr/include/boost/geometry.hpp:17,
                 from /home/nilsnolde/dev/cpp/valhalla/valhalla/mjolnir/admin.h:4,
                 from /home/nilsnolde/dev/cpp/valhalla/src/mjolnir/admin.cc:1:
/usr/include/c++/13.2.1/cstdlib:79:15: fatal error: stdlib.h: No such file or directory
   79 | #include_next <stdlib.h>
      |               ^~~~~~~~~~
compilation terminated.

there's several mentions I found that using -isystem /usr/include would make problems with #include_next, because it re-orders include path priorities..

but on the other hand, gcc is supposed to know its own system include dirs and treat them as such. apparently cmake does some effort so that system directories never show up in the generated commands:

nilsnolde commented 5 months ago

I'll ask in some forum and see what people suggest. I have no idea what else to try..