Good evening, On Oct 17, 2013, at 4:01 PM, Sean McBride <[email protected]> wrote:
> On Thu, 17 Oct 2013 14:47:59 -0500, Raymond Lu said: > >> Several of us including Quincey and Elena looked at the issue. Hmmm… my role was to make sure that developers look at the code :-) But I take full responsibility for this answer to the forum since our decision and how we arrived at it is not explained at all! Sean, Thank you for looking into the issue and for holding our feet to the fire! I hope we will be able to resolve the problem very soon. Elena >> We >> decided that since the algorithm is trying to detect the alignment of >> integers, ideally the flag -fcatch-undefined-behavior and other >> optimization flags shouldn't to be used for H5detect.c. In the future, >> we can separate flags for H5detect.c from the rest of the library. >> >> (For those who don't know what Issue 8147 is: CLANG compiler on mac >> machines with the options -fcatch-undefined-behavior and -ftrapv catches >> some undefined behavior in the alignment algorithm of the macro DETECT_I >> in H5detect.c.) > > Thanks for your followup! > >> Please let us know your thoughts. > > I must respectfully disagree. :) I'll try to convince you... :) > > Perhaps you misunderstand the purpose and meaning of > -fcatch-undefined-behavior: it is not an optimization flag, it is a bug > finding tool. It has found a bug in HDF5. Ignoring it is not helpful. :) > > For a good read on undefined behaviour (really worth your time!) see: > > "What Every C Programmer Should Know About Undefined Behavior" > <http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html> > > C compilers are getting very smart these days, and performing optimizations > based on the (valid!) assumption that undefined behaviour *must not* occur. > For a great example see "A Fun Case Analysis" here: > <http://blog.regehr.org/archives/213> > > HDF's own HISTORY-1.8.txt even discusses problems at higher optimization > levels: > > ---------- > * For gcc v4.3 and v4.4, with production mode, if -O3 is used, H5Tinit.c > would fail to compile. Actually bad H5Tinit.c is produced. If -O (same > as -O1) is used, H5Tinit.c compiled okay but test/dt_arith would fail. > When -O0 (no optimizatio) is used, H5Tinit.c compilete okay and all > tests passed. Therefore, -O0 is imposed for v4.3 and v4.4 of gcc. > AKC - 2009/04/20 > ---------- > > And lo and behold, dt_arith is one of the tests that fails when > -fcatch-undefined-behavior is enabled: > <http://cdash.hdfgroup.uiuc.edu/testDetails.php?test=299376&build=9516> > > I'd wager that's exactly what I'm describing: gcc's optimizer is in fact > doing nothing wrong, rather, HDF5's code invokes undefined behaviour in > several places, and so the compiler can generate whatever garbage it wants to. > > -fcatch-undefined-behavior is a tool to catch (some of) these problems in > debug mode, before the optimizer screws you. > > Consider this obvious example: > > ---------- > int main (void) > { > float big = 1e20; > unsigned char small = big; > > printf ("small is %u \n", small); > > return 0; > } > ---------- > > unsigned char (on most platforms at least) has too little range to hold that > big number. What do you expect this code to do? It's undefined behaviour, > so the compiler can do whatever it wants! Let's see: > > $ clang -O0 test.c > $ ./a.out > small is 0 > > $ clang -O3 test.c > $ ./a.out > small is 255 > > Now, let's use modern tools: > > $ clang -fsanitize=undefined-trap test.c > $ ./a.out > test.c:8:13: runtime error: value 1e+20 is outside the range of representable > values of type 'unsigned char' > small is 0 > > Bug identified! Line number included! > > IIRC, the DETECT_I macro is violating alignment rules, doing something like > this: > > ---------- > int main (void) > { > char big[] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66}; > int* foo = (int*) &(big[1]); > int bar = *foo; > > printf ("bar is %x \n", bar); > > return 0; > } > ---------- > > $ clang -fsanitize=undefined-trap test.c > $ ./a.out > test.c:9:13: runtime error: load of misaligned address 0x7fff50ab69a6 for > type 'int', which requires 4 byte alignment > 0x7fff50ab69a6: note: pointer points here > ab 50 ff 00 11 22 33 44 55 66 00 00 00 00 c8 69 ab 50 ff 7f 00 00 e1 47 39 > 89 ff 7f 00 00 e1 47 > ^ > bar is 44332211 > > You can't do that, see: > > <https://www.securecoding.cert.org/confluence/display/seccode/EXP36-C.+Do+not+convert+pointers+into+more+strictly+aligned+pointer+types> > > In other words, DETECT_I will *detect the wrong thing* depending how the > compiler decides to react to the invalid code. > > Phew! I hope I've convinced you now... :) > > Cheers, > > -- > ____________________________________________________________ > Sean McBride, B. Eng [email protected] > Rogue Research www.rogue-research.com > Mac Software Developer Montréal, Québec, Canada > > > > _______________________________________________ > Hdf-forum is for HDF software users discussion. > [email protected] > http://mail.lists.hdfgroup.org/mailman/listinfo/hdf-forum_lists.hdfgroup.org _______________________________________________ Hdf-forum is for HDF software users discussion. [email protected] http://mail.lists.hdfgroup.org/mailman/listinfo/hdf-forum_lists.hdfgroup.org
