Re: [lmi] Integer overflow warnings in bourn_cast with clang

[Greg Chicares]

On 2017-04-09 22:11, Vadim Zeitlin wrote:
> On Sun, 9 Apr 2017 17:39:44 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2017-04-07 13:50, Vadim Zeitlin wrote:
> GC> > On Fri, 7 Apr 2017 13:31:25 +0000 Greg Chicares <address@hidden> wrote:
> GC> [...]
> GC> > GC> Let me decompose the implementation into four separate function 
> templates
> GC> > GC> so that clang can compile it and we can then reexamine this.
> GC> > 
> GC> >  Just to be clear: the difference in test results is due solely to
> GC> > architecture, not compiler. I.e. it's not clang-specific, all versions 
> of
> GC> > g++ (4, 5, 6) also pass the tests with my original "fix" in 64 bits but
> GC> > fail them in 32 bits.
> GC> 
> GC> Does commit b87d1dde7b63482b872bffeb998f711f37d4d08a fix all the
> GC> unexpected test failures?
> 
>  Yes, the test now passes with clang (and still passes with g++ 6), thanks!

I needed to know the reason, so, with some casual local modifications,
I did a native build:

make $coefficiency build_type=native CXXFLAGS='-O0 -ggdb -m32' CFLAGS='-m32' 
LDFLAGS='-m32' unit_tests unit_test_targets=bourn_cast_test 2>&1 |less -S

(Installing 'g++-multilib' in my chroot was *much* less painful than
wrestling with 'winedbg'.]

After reverting this change in 'bourn_cast.hpp':
    // At least with i686-w64-mingw32-g++ version 4.9.1, this change:
    // - if(From(to_traits::max()) + 1 <= from)
    // + if(adj_max <= from)
    // fixes incorrect results with '-O0'.
it was pretty easy to find the asm by looking for an FLD1 instruction:

   0x0805ac58 <+344>:   fildll -0x100(%ebp)
   0x0805ac5e <+350>:   fstps  -0xec(%ebp)
   0x0805ac64 <+356>:   flds   -0xec(%ebp)
   0x0805ac6a <+362>:   fld1   
   0x0805ac6c <+364>:   faddp  %st,%st(1)
   0x0805ac6e <+366>:   flds   0x8(%ebp)
   0x0805ac71 <+369>:   fucompp 
   0x0805ac73 <+371>:   fnstsw %ax
   0x0805ac75 <+373>:   sahf   

(gdb) b *0x0805ac6a
[showing only the top two FP registers--the others are unused]
[alternating 'stepi' and 'info float' gdb commands...]

=>R7: Valid   0x401f8000000000000000 +4294967296                
  R6: Empty   0x00000000000000000000

  R7: Valid   0x401f8000000000000000 +4294967296                
=>R6: Valid   0x3fff8000000000000000 +1                         

=>R7: Valid   0x401f8000000080000000 +4294967297                
  R6: Empty   0x3fff8000000000000000

  R7: Valid   0x401f8000000080000000 +4294967297                
=>R6: Valid   0x401f8000000000000000 +4294967296                

  R7: Empty   0x401f8000000080000000
  R6: Empty   0x401f8000000000000000

So what it's really doing is:

    if(From(to_traits::max()) + 1 <= from)

   0x0805ac58 <+344>:   fildll -0x100(%ebp)

load integer 2^32

   0x0805ac5e <+350>:   fstps  -0xec(%ebp)
   0x0805ac64 <+356>:   flds   -0xec(%ebp)

cast it to float, truncating nothing of course; then push it back
on the FPU stack

   0x0805ac6a <+362>:   fld1   

add one, in extended precision, producing a value that a float
cannot represent

   0x0805ac6c <+364>:   faddp  %st,%st(1)

push bourn_cast's argument, and compare

   0x0805ac6e <+366>:   flds   0x8(%ebp)
   0x0805ac71 <+369>:   fucompp 

Now what happens if I add an explicit cast to the source:

-    if(From(to_traits::max()) + 1 <= from)
+    if(From(From(to_traits::max()) + 1) <= from)

? Answer: the asm is the same, line for line. Conclusion: the recent
commit that forces a write to a volatile float is apparently the only
way to be sure we get exactly what we want.