[Patch] sext_hwi: Avoid left shift of negative value undefined behaviour

[Richard Biener]

On Thu, Aug 13, 2015 at 1:08 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2015.08.13 at 12:56 +0200, Mikael Morin wrote:
>> Le 12/08/2015 22:07, Richard Sandiford a écrit :
>> > Jeff Law <law@redhat.com> writes:
>> >> On 08/12/2015 12:32 PM, Richard Biener wrote:
>> >>> On August 12, 2015 8:07:13 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>> >>>> On 08/12/2015 11:12 AM, Richard Biener wrote:
>> >>>>
>> >>>>>
>> >>>>> Prec is almost never a constant and is heavily used from wide-int.
>> >>>>>
>> >>>>> We are not exploiting this undefined ness in C so I object to making
>> >>>> this so much slower.
>> >>>>>
>> >>>>> Can we instead do what we do for abs_hwi and add a checking assert so
>> >>>> we can move the tests to the callers that misbehave instead?
>> >>>> Given that ISO C++ is moving away from making shifting 1 into the sign
>> >>>> bit undefined behaviour, maybe we should make UBSan less strict in its
>> >>>> warning.  That may eliminate the need for Mikael's patch.
>> >>>
>> >>> We can also use an logical left shift followed by an arithmetic right
>> >>> shift. Or is the latter invoking undefined behaviour as well in some
>> >>> cases we hit?
>> >> Hmm, why aren't we using logicals for the left shift to begin with?
>> >> That's the problem area.  I don't think the right shifts are an issue at
>> >> all.
>> >
>> > Well, they're implementation-defined, at least in C.  The C11 wording
>> > for E1 >> E2 is "If E1 has a signed type and a negative value, the
>> > resulting value is implementation-defined".  Is C++ different?
>> > (I don't have the standard handy.)
>> >
>> > So...
>> >
>> >> It's strange that when I was researching this, consistently I saw folks
>> >> suggesting the LEFT-SHIFT followed by RIGHT-SHIFT, then it'd get shot
>> >> down as UB, then folks went to either Mikael's approach or another that
>> >> is very similar.  Nobody suggested twidding the types to get a
>> >> left-logical-shift, then doing a right-arithmetic-shift.
>> >
>> > ...unless C++ is different, there's not a standard-level concept
>> > of arithmetic shift.
>> >
>> Still, implementation-defined behavior is a progress over undefined
>> behaviour.
>> Even if it's not set in stone, the good thing about
>> implementation-defined behaviour is that it's known to be non-random.
>> So it can be checked, either with autoconf, or with a macro.
>> Would it be acceptable to have both variants of the code and decide
>> among them with such a check?
>> It feels a bit overkill for such a little function, but it is the only
>> way that I see to achieve both need for speed and for well-defined-ness.
>> I guess it won't kill the bootstrap-ubsan errors though.
>
> For such cases you can use the ATTRIBUTE_NO_SANITIZE_UNDEFINED macro,
> that is defined in include/ansidecl.h.

Rather ubsan should not complain about implementation defined behavior (or
should separate those cases out with a different switch compared to undefined
behavior).

Richard.

> --
> Markus