guillaumechereau / goxel

Goxel: Free and Open Source 3D Voxel Editor
GNU General Public License v3.0
2.83k stars 226 forks source link

compilation with clang fails due to -Wnan-infinity-disabled #386

Closed sleeptightAnsiC closed 3 months ago

sleeptightAnsiC commented 3 months ago

Hey again @guillaumechereau ,

In reference to your https://github.com/guillaumechereau/goxel/issues/385#issuecomment-2274982737 - I just tried to compile the project with CC=clang make release and it fails on my side due to warning -Wnan-infinity-disabled in following way:

clang -o src/gesture.o -c -std=gnu99 -Wall -Wno-unknow-pragma -Wno-unknown-warning-option -Werror -Ofast -include src/config.h -pthread -DNDEBUG -DHAVE_LIBPNG=1 -Isrc -I. -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/atk-1.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/harfbuzz -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/libpng16 -I/usr/include/pixman-1 -I/usr/include/cloudproviders -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/fribidi -I/usr/include/sysprof-6 -I/usr/include/gio-unix-2.0 -Iext_src -Iext_src/uthash -Iext_src/stb -Iext_src/nfd -Iext_src/noc -Iext_src/xxhash -Iext_src/meshoptimizer src/gesture.c
In file included from src/formats/wavefront.c:25:
src/../ext_src/voxelizer/voxelizer.h:610:48: error: use of infinity via a macro is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  610 |     aabb->max.x = aabb->max.y = aabb->max.z = -INFINITY;
      |                                                ^
src/../ext_src/voxelizer/voxelizer.h:611:47: error: use of infinity via a macro is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  611 |     aabb->min.x = aabb->min.y = aabb->min.z = INFINITY;
      |

I suspect this is the same one you encountered on your side. Since this is just a warning, you can suppress it with -Wno-nan-infinity-disabled like so:

CC=clang CFLAGS=-Wno-nan-infinity-disabled CXXFLAGS=-Wno-nan-infinity-disabled make release

Although, it makes the compilation go through successfully, I'm not sure if it causes any other issues at runtime (I don't really see any right now, hard to tell). More about said warning here: https://clang.llvm.org/docs/DiagnosticsReference.html#wnan-infinity-disabled

Cheers!

$ clang --version
clang version 18.1.8
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ git show
commit 90a984fb9e242df7e26d9d9be3f1836c6095f2d1 (HEAD -> master, origin/master, origin/HEAD)
guillaumechereau commented 3 months ago

Thanks for the info. On my Mac with clang 15.0.0 it compiles fine. I'll check on my linux machine later today when I have the time.

guillaumechereau commented 3 months ago

I pushed a potential fix using a pragma before the include (so that I don't disable the warning globally). I think this warning should be fine to ignore, though eventually this infinity could be replaced with a FLT_MAX so that it works fine even with -ffast-math.

sleeptightAnsiC commented 3 months ago

Hey @guillaumechereau,

https://github.com/guillaumechereau/goxel/commit/9e79543493a5a451bedf0063020cc424c58c3ec1 does not fully fix it on my side. Doing CC=clang make release still fails, just a bit later like so:

clang -o src/gui/about.o -c -std=gnu99 -Wall -Wno-unknow-pragma -Wno-unknown-warning-option -Werror -Ofast -include src/config.h -pthread -DNDEBUG -DHAVE_LIBPNG=1 -Isrc -I. -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/atk-1.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/harfbuzz -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/libpng16 -I/usr/include/pixman-1 -I/usr/include/cloudproviders -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/fribidi -I/usr/include/sysprof-6 -I/usr/include/gio-unix-2.0 -Iext_src -Iext_src/uthash -Iext_src/stb -Iext_src/nfd -Iext_src/noc -Iext_src/xxhash -Iext_src/meshoptimizer src/gui/about.c
src/goxel.c:298:24: error: use of infinity via a macro is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  298 |     float dist, best = INFINITY;
      |                        ^
src/goxel.c:687:41: error: use of NaN via a macro is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  687 |             inputs2.touches[i].pos[0] = NAN;
      |                                         ^
src/goxel.c:687:41: error: use of NaN is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  687 |             inputs2.touches[i].pos[0] = NAN;
      |                                         ^~~
/usr/include/math.h:98:16: note: expanded from macro 'NAN'
   98 | #  define NAN (__builtin_nanf (""))
      |                ^~~~~~~~~~~~~~~~~~~
src/goxel.c:688:41: error: use of NaN via a macro is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  688 |             inputs2.touches[i].pos[1] = NAN;
      |                                         ^
src/goxel.c:688:41: error: use of NaN is undefined behavior due to the currently enabled floating-point options [-Werror,-Wnan-infinity-disabled]
  688 |             inputs2.touches[i].pos[1] = NAN;
      |                                         ^~~
/usr/include/math.h:98:16: note: expanded from macro 'NAN'
   98 | #  define NAN (__builtin_nanf (""))
      |                ^~~~~~~~~~~~~~~~~~~
5 errors generated.

I took a look at some .c files and there's lots of them that would require similar suppressing. tbh silently suppressing any warning about undefined behavior is a bit dangerous. This may cause super nasty bugs in release builds that will be hard to track down, even with sanitizer/debugger attached.

Is there a reason to use such high optimization as -ffast-math ? According to GCC manual, this option is super unsafe and even -O3 doesn't enable it by default. For contrast, lots of Linux distros consider -O3 as unsafe and rarely go past -O2, see: https://wiki.gentoo.org/wiki/GCC_optimization

guillaumechereau commented 3 months ago

I thought using ffast-math would improve the performances of things like matrix multiplications. I never compared with or without and I wouldn't be surprised if in practice it makes no difference.

I think I will switch from -Ofast to -O3 for now. I would still like to try to avoid using NAN or INFINITY in all the code if possible.

guillaumechereau commented 3 months ago

OK I switched to -O3. I am 99% sure this shouldn't affect the performances, but if it does then I can check how to enable it only in the part of the code that matters. I still removed the internal usage of NAN, since I don't think this was a good idea anyway.

sleeptightAnsiC commented 3 months ago

Thanks, compilation goes through successfully on https://github.com/guillaumechereau/goxel/commit/292588ca2ed8cff6d179762419eca6e5a7bad6ce