Time  Nick      Message
03:15 kaeza     I can reproduce #4810 in release mode, but not in debug mode :/
03:15 ShadowBot https://github.com/minetest/minetest/issues/4810 -- Unittests crash on Release build
03:19 sofar     I somehow think that the condition is optimized out
03:19 sofar     at least, that's how it is looking to me
03:26 kaeza     wish compilation from scratch didn't take half an hour
03:27 kaeza     but then again, what can you expect from, *ahem* a ~2009 dual core
03:27 sofar     I was about to say....
03:27 sofar     $ time make -C build -j12
03:28 sofar     my cpu is q1-2011
03:28 sofar     but, 12 threads XD
03:29 kaeza     Intel E5700 here. probably older than 2009
03:29 sofar     q3'10
03:30 sofar     real	2m22.396s
03:30 sofar     user	24m31.483s
03:30 sofar     2 1/2 mins to compile minetest
03:31 sofar     even though it's HT, the 6 threads give way more than just a few % increase
03:32 sofar     144 secs total, each thread average consumed 123 seconds
03:32 sofar     86% of theoretical ideal HT performance
03:32 kaeza     "BOUND IS NOT 0: 0"
03:33 kaeza     nice gcc :(
03:35 sofar     you know
03:35 sofar     kaeza: try this
03:35 sofar     -               return next();
03:35 sofar     +               abort();
03:35 sofar     just recompile and run
03:38 sofar     don't make clean, just recompile noise.cpp
03:39 kaeza     same thing
03:40 sofar     does that abort or FPE?
03:40 kaeza     SIGFPE
03:40 sofar     try this one:
03:40 sofar     -               return next();
03:40 sofar     +               return this->next();
03:40 sofar     that one fixes the issue for me
03:41 sofar     that confirms my suspicion that the condition is optimized away
03:42 sofar     kaeza: what compile optimization level did you do for the `abort()` build?
03:43 kaeza     I used CMAKE_BUILD_TYPE=RelWithDebInfo. I think it has -O2 in there, but not sure
03:43 kaeza     lemme check
03:45 kaeza     /usr/bin/c++ -DUSE_CMAKE_CONFIG_H [-I...] -DNDEBUG -Wall -ffast-math -Wall -pipe -funroll-loops -O3 -fomit-frame-pointer   -o CMakeFiles/minetest.dir/noise.cpp.o -c .../src/minetest/src/noise.cpp
03:46 sofar     I have no clue how to tell cmake to use -O0 or anything else instead
03:46 sofar     otherwise I'd assume -O2 would already fix it
03:47 sofar     O3 is definitely bad here, I think
03:47 kaeza     you can set COMPILE_FLAGS property of minetest[.exe] to "-O2" in CMakeLists.txt
03:48 kaeza     or maybe just make CFLAGS=-O2
03:49 kaeza     make CFLAGS does not work it seems
03:59 sofar     try -O0
04:34 kaeza     sofar, compiling with -O2 fixes the thing, so -O3 is definitely broken
04:36 kaeza     BTW: real	5m58.210s | user	10m40.148s
07:27 nrzkt     Zeno`, sofar sfan5 can you look at #5020 i imported a part of Rogier-5 PR (#4764) the performance fix + some constness)
07:27 ShadowBot https://github.com/minetest/minetest/issues/5020 -- Performance fix + constness on SAO by nerzhul
07:31 nrzkt     #4764
07:31 ShadowBot https://github.com/minetest/minetest/issues/4764 -- Sao refactor 2 by Rogier-5
07:31 nrzkt     seems the bot only read first github issue
12:25 juhdanad  In #4986 the palettes are implemented!
12:25 ShadowBot https://github.com/minetest/minetest/issues/4986 -- Hardware node coloring by juhdanad
12:25 VanessaE  niice
12:28 Calinou   "no textures -> higher framerate"
12:28 Calinou   what kind of hardware is this? :P
12:30 juhdanad  Sorry, I don't understand what you don't understand.
14:33 nrzkt     merging #5020 in few minutes, thanks sfan5 for the review
14:33 ShadowBot https://github.com/minetest/minetest/issues/5020 -- Performance fix + constness + SAO attribute factorization by nerzhul
14:34 sfan5     uh wheres the second approval
14:54 nrzkt     i'm a coredev
14:54 nrzkt     i approve rogier
14:54 sfan5     oh the original is by him
14:55 nrzkt     yes i split his PR in a simpler part, and i will do another PR with second refactor part (move some functions) because the current original PR needs to be rebase and is a little bit painful as is
14:55 nrzkt     (i will do it later)
15:38 juhdanad  Is there a way to set the depth function to GL_LEQUAL in Irrlicht?
15:52 juhdanad  I found it! Sorry for bothering you...
19:25 nrzkt     merging #5021
19:25 ShadowBot https://github.com/minetest/minetest/issues/5021 -- Make nametag removable with set_nametag_attributes by Rui-Minetest
21:11 kaeza     sofar, re: unittest crash, your patch does nothing for me
21:11 sfan5     it should actually do nothing
21:11 kaeza     keep getting SIGFPE
21:12 sfan5     since next() references the next in the same class and is thus called like this->next() anyway
21:12 sfan5     to me it sounds like the compiler is doing something wrong
21:12 kaeza     see discussion yesterday
21:12 kaeza     or earlier
21:13 kaeza     changing -O3 to -O2 fixes the issue here
21:14 kaeza     something funny is going on with gcc
21:17 sfan5     compilers are advanced magic
21:17 sfan5     sometimes that magic goes wrong
21:17 KaadmY    :D
21:38 garywhite Hi Tmanyo
21:38 Tmanyo    hi
21:39 garywhite Sorry, wrong channel again
21:48 nrzkt     sfan5, i added a new PR from rogier SAO refactor commit #5022, easy part, it's the factorization of exactly similar parts between LuaEntitySAO & + a little perf fix
21:48 ShadowBot https://github.com/minetest/minetest/issues/5022 -- Cleanup content_sao by factorizing similar code parts by nerzhul
21:53 est31     idk maybe -O3 is just using undefined behaviour?
21:53 est31     compilers are weird, e.g. overflow is undefined
21:55 sfan5     no that's not how it works
21:55 sfan5     at the C/C++ language level overflow is undefined
21:55 sfan5     which gives the compiler the right to do what it wants
21:55 sfan5     however at processor level overflow is very well defined and the compiler knows about that
21:55 kaeza     the compiler is optimizing the `if` (and `return`) out
21:56 est31     sfan5: yes
21:56 kaeza     maybe somebody is crazy enough to `gcc -S` it and check?
21:56 est31     sfan5: I was talking about C++ level overflow
21:56 est31     in asm its defined yes
21:57 sfan5     but using -O3 doesn't modify the c++ code
21:57 est31     what issue  are you talking about either way?
21:58 kaeza     #4810
21:58 ShadowBot https://github.com/minetest/minetest/issues/4810 -- Unittests crash on Release build
22:01 sfan5     kaeza: _ZN9PcgRandom5rangeEj:    testl   %esi, %esi            je      .L64
22:01 sfan5     the check is there
22:01 sfan5     i used to be able to reproduce that issue too but it doesn't work anymore, probably after a compiler update
22:02 est31     maybe a compiler bug then?
22:02 est31     either its a bug in our program, or a compiler bug
22:04 est31     how to run unittests again?
22:04 est31     ah --run-unittests
22:05 est31     mhh cant reproduce either
22:06 est31     but have gcc 6.2.0 so it might be too old
22:07 est31     also its weird that turning on debug info makes the bug go away
22:07 est31     since when is debug info a problem
22:07 kaeza     it's not debug info
22:07 est31     "Running the unittests on a Release build crashes with:" [...]
22:08 sfan5     RelWithDebInfo probably has slightly different optimization flags from Release
22:08 est31     "In Debug, RelWithDebInfo or MinSizeRel they run fine."
22:08 kaeza     debug builds use -O2
22:08 est31     yeah
22:08 kaeza     release use -O3
22:08 est31     mhh well first step is to minimize the test
22:08 est31     then to look at the generated assembly and the code
22:08 est31     then to look at it again
22:08 est31     then to look at what's defined and undefined
22:08 est31     then to look at it again
22:09 est31     and then to file a bug
22:09 est31     (in the compiler)
22:09 sfan5     that will waste the maintainers time
22:09 est31     also probably gcc devs already know of this
22:09 sfan5     since the bug is obviously already fixed in later verions
22:09 sfan5     versions*
22:09 est31     is it?
22:09 sfan5     ...you just said you can't reproduce
22:09 kaeza     probably
22:09 est31     I was on debug build :)
22:09 kaeza     >gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4)
22:10 est31     it seems to appear with 5.4.0 but not with 6.2.1 and then it appears again on 6.3.0
22:10 est31     how weird is that
22:10 est31     probably some other factor than compiler version plays into it
22:10 est31     is it spurious?
22:10 sfan5     it either always happens or never
22:11 est31     so no.
22:11 sfan5     maybe g++ devs "accidentally" fixed the bug and then unfixed it
22:11 est31     could be
22:11 est31     but thats bad, dont they have a testsuite
22:11 sfan5     they do
22:12 sfan5     but who knows what special factors cause this issue
22:12 est31     idk about gcc, but rust has a gigantic testsuite and each single bug that gets fixed gets put inside so that it never regresses
22:12 sfan5     their test suite cannot cover the infinite set of C++ code one could write
22:12 est31     yeah
22:12 est31     but as you said, maybe the fixing of the issue was an accident
22:12 est31     oookay
22:12 est31     I can reproduce
22:13 est31     now next step: minification
22:13 sfan5     wait which gcc version?
22:13 est31     6.2
22:13 est31     dunno whether I used clang though
22:13 sfan5     never happened for me w/ clang
22:14 est31     GNU C11 6.2.0 20161005 -mlong-double-80 -mtune=generic -march=x86-64 -g -g -g -O2 -O2 -O2 -fbuilding-libgcc -fno-stack-protector -fpic
22:14 est31     thats from strings bin/minetest
22:14 est31     so it must be true
22:14 est31     strings never lies
22:14 kaeza     try touch src/noise.cpp ; make VERBOSE=y
22:14 est31     err... -O2 ??
22:15 sfan5     you're looking at the opt flags for c code
22:15 sfan5     use kaeza's method to determine what really is used
22:15 est31     ?
22:15 sfan5     also -fbuilding-libgcc suggests that you have catched the build flags when libgcc was built by your distro
22:16 est31     yeah :D
22:16 est31     well dunno, suprising that libgcc gets statically linked either way
22:16 est31     guess libgcc != glibc
22:16 est31     mhh
22:16 sfan5     huh i can reproduce again
22:16 sfan5     with /usr/bin/c++   -DSERVER -DUSE_CMAKE_CONFIG_H -I/usr/include/leveldb -I/usr/include/hiredis -I/tmp/minetest/src -I/usr/include/irrlicht -I/tmp/minetest/src/Release -I/usr/include/luajit-2.0 -I/tmp/minetest/src/jsoncpp -I/tmp/minetest/src/script -I/usr/include/freetype2 -I/tmp/minetest/src/cguittfont  -DNDEBUG -Wall   -ffast-math -Wall -pipe -funroll-loops -O3 -fomit-frame-pointer   -o CMakeFiles/minetestserver.dir/noise.cpp.o -c
22:16 sfan5     /tmp/minetest/src/noise.cpp
22:17 sfan5     ( gcc-Version 6.2.1 20160830 (GCC) )
22:19 sfan5     here's the asm http://paste.ubuntu.com/23783912/
22:19 est31     no debuginfo?
22:20 est31     also, what are the inputs
22:20 sfan5     missed that line
22:20 sfan5     no i used Release
22:20 est31     that doesnt mean you have to miss debuginfo
22:20 est31     optimisations are separate from debuginfo
22:20 est31     also, do -M intel on objdump
22:21 est31     att syntax is horrible
22:21 sfan5     i know
22:21 sfan5     it's however crashing in PcgRandom::range(int, int) not PcgRandom::range(unsigned int)
22:22 sfan5     this is a whole different issue
22:22 sfan5     it doesn't crash in the function where bound is used
22:26 sfan5     oh that's inlined somewhere i guess(?)
22:30 paramat   no hmmmm again, i feel lost
22:31 est31     oh great
22:32 est31     seems when I extract the stuff into a minimal program it works
22:33 kaeza     Thread 1 "minetest" received signal SIGFPE, Arithmetic exception.
22:33 kaeza     0x0000000000894958 in PcgRandom::range (bound=0, this=0x7fffffffc890)
22:33 kaeza     at /home/kaeza/src/minetest/src/noise.cpp:102
22:33 kaeza     102		u32 threshold = -bound % bound;
22:33 est31     this standalone program
22:33 est31     https://gist.github.com/est31/75e752ef22bdf1a9a0881da493a1bbe2
22:34 est31     compiled with g++ -pipe  -funroll-loops -fomit-frame-pointer  -ffast-math -O3 -o m main.cpp
22:34 paramat   any opinions on #5014 before i merge it later?
22:34 est31     doesnt work
22:34 ShadowBot https://github.com/minetest/minetest/issues/5014 -- Sky: Do not darken in dark places when above water_level by paramat
22:34 kaeza     whoa, sorry about that
22:35 sfan5     so the problem is with s32 PcgRandom::range(s32 min, s32 max)
22:36 sfan5     since the exception on (max < min) assures the compiler that max >= min
22:36 sfan5     it can rightfully assume that max - min + 1 is never 0 and removes the zero check when inlining u32 PcgRandom::range(u32 bound)
22:36 sfan5     though for some reason it is actually 0
22:37 est31     it performs the - operation before the % right?
22:37 sfan5     yes
22:38 sfan5     the only way i can see it being 0 is that max - min underflows to 0xffffffff and +1 makes it go to 0
22:38 est31     yeah
22:44 sfan5     https://github.com/minetest/minetest/commit/738fbc66d096575bb9a1694056ce2d627a70c03d
22:44 sfan5     hm
22:44 sfan5     looks like previous gcc versions were not smart enough to optimize the check away when inlining
22:48 sfan5     const static s32 RANDOM_MIN   = -0x7fffffff - 1;
22:48 sfan5     is this supposed to be the smallest number random can generate?
22:48 sfan5     or the number below that one?
22:48 est31     dunno