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

[Greg Chicares]

On 2017-04-06 15:53, Vadim Zeitlin wrote:
> On Thu, 6 Apr 2017 15:24:08 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2017-04-06 00:24, Vadim Zeitlin wrote:
[...]
> GC> >  The trivial patch
> GC> [...]
> GC> > -        if(From(to_traits::max()) + 1 <= from)
> GC> > +        if(From(to_traits::max()) <= from - 1)
> GC> [...]
> GC> > fixes it while allowing the tests to still pass,
> GC> 
> GC> In what sense do the tests pass?
> 
>  I wasn't being facetious, they do pass for me (with my clang-specific
> patches and this one on top of b777985b7b0102f665d1d451508d4dc7aabcb76a).
> I.e. the last line of output is "!!!! no errors detected" and all 540 tests
> succeed. But this is because I'm running tests under x86_64 Linux while
> you're probably testing under 32 bit MSW

Correct, I'm using 'i686-w64-mingw32-g++'.

> -- and I do see these failures
> there (I also see 539 and not 540). So this is clearly because int64_t can
> represent something not representable in an int32_t.

But I don't see how the errors can involve only 32-bit integers, because
with the patch above I observe this with my 32-bit build:

$make $coefficiency unit_tests unit_test_targets=bourn_cast_test.exe 2>&1 |grep 
'Cast from'
    'Cast from 18446744073709551616.000000 [float] to 0 [unsigned long long] 
would not preserve value.'
    'Cast from 9223372036854775808.000000 [float] to -9223372036854775808 [long 
long] would not preserve value.'
    'Cast from 4294967296.000000 [float] to 0 [unsigned int] would not preserve 
value.'
    'Cast from 2147483648.000000 [float] to -2147483648 [int] would not 
preserve value.'
    'Cast from 18446744073709551616.000000 [double] to 0 [unsigned long long] 
would not preserve value.'
    'Cast from 9223372036854775808.000000 [double] to -9223372036854775808 
[long long] would not preserve value.'
    'Cast from 18446744073709551616.000000 [float] to 0 [unsigned long long] 
would not preserve value.'
    'Cast from 18446744073709551616.000000 [float] to 0 [unsigned long long] 
would not preserve value.'
    'Cast from 18446744073709551616.000000 [float] to 0 [unsigned long long] 
would not preserve value.'
    'Cast from 18446744073709551616.000000 [double] to 0 [unsigned long long] 
would not preserve value.'

The third and fourth use (signed and unsigned) int, but all the others
involve (signed or unsigned) long long, which AIUI has to be at least
64 bits, and 18446744073709551616 above definitely is 2^64.

Let me decompose the implementation into four separate function templates
so that clang can compile it and we can then reexamine this.

> GC> > but I'm not sure at all if
> GC> > this doesn't introduce some other problem, I'm afraid this code has 
> become
> GC> > too complicated for me to reason about it with certainty.
> GC> 
> GC> > -        if(From(to_traits::max()) + 1 <= from)
> GC> 
> GC> Here, the integral maximum is 2^N - 1, and adding one makes it 2^N.
> GC> If the floating ("From") type has enough mantissa bits to represent the
> GC> integral type's maximum, then "+ 1" really does add one. Otherwise, "+ 1"
> GC> does nothing, but that's okay because "From(to_traits::max())" was already
> GC> rounded to nearest (if IEC 559 is respected), i.e. to 2^N. Either way, the
> GC> LHS equals 2^N.
> GC> 
> GC> > +        if(From(to_traits::max()) <= from - 1)
> GC> 
> GC> This is not equivalent. If we were to follow boost's design decision
> GC> and accept truncating conversions from floating to integral, we'd get
> GC> incorrect results.
> 
>  Would we though? AFAICS we'd still refuse to cast, but just with a "would
> not preserve value" error instead of "would transgress upper limit" one.
> So, IOW, maybe this check, and more precise error message, could be just
> removed completely?

If we were to follow boost's design in the floating-to-integral code,
then we'd remove the 'if(r != from)' block in order to let truncating
conversions pass:

        if(From(to_traits::max()) + 1 <= from)
            throw std::runtime_error("Cast would transgress upper limit.");
        To const r = static_cast<To>(from);
        if(r != from)
            {
            ...throw elaborate "preserve value" error message seen above...
            ...we'd remove this block if we wanted to permit truncation...
            }
        return r;

and then we'd have only the "would transgress upper limit" test.

Anyway, my guess is that the patch above allows execution to reach
"static_cast<To>(from)", which involves undefined behavior that
somehow causes the "r != from" test to be skipped with clang. The
compiler isn't really required to perform a static_cast that has
undefined behavior and test the outcome: it can just assume that
static_cast preserves value, and skip the value-preservation test.