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

[Mikael Morin]

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.

Mikael