Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179)

[Martin Sebor <msebor at redhat dot com>]

On 04/24/2015 10:27 AM, Marek Polacek wrote:
> On Thu, Apr 23, 2015 at 09:11:39PM -0600, Martin Sebor wrote:
> > I wonder if the tests where the left shift operands are both
> > constants really do invoke undefined behavior in GCC. For
> > example, AFAICS, in (-1 << 0) and other constant expressions
> > gcc computes the shift in unsigned HOST_WIDE_INT which is well
> > defined.
>
> Yeah, two INTEGER_CSTs are computed in wide-int using uhwi.  But -1 << 0
> certainly is UB according to ISO C/C++ and I think we should follow the
> standards.
>
> > It seems the warning would be more valuable (and less likely
> > dismissed as overly pedantic) if it was issued when the second
> > operand was not constant and the computation had to be done in
> > hardware (or even better, in hardware not known to  use the
> > same instructions for positive and negative operands).
>
> What I've tried to do in the patch was to mimic the other two Wshift-*
> warnings.  While the new warning triggered a few times in the testsuite, GCC
> bootstrap itself was clean.  You raise a very good point though, we don't
> want to be overly pedantic.
>
> I suppose we could go with the patch as-is, and if users complain too much,
> warn only if the second operand is not constant or so.  But I hope that
> left-shifting a negative value is not a common thing.
>
> > The warning would also be valuable in some sort of a portability
> > mode (with -pedantic?) where the code is intended to be portable
> > to implementations that don't provide well-defined semantics for
> > left shifts of negative values.
>
> I really think -Wshift-negative-value should be kin to -Wshift-count-negative
> and -Wshift-count-overflow, those are enabled by default.

There's a significant difference between the reasons why
the behavior of the left shift is undefined when the left
operand is negative vs when the right operand is, and
between the results of such expressions computed by GCC
and common hardware.

When the right operand is negative the operation simply
has no meaning (some languages define the operation as
shifting right by the inverse number of bits but that's
by no means universal). In practice, the right operand
is sometimes limited by the hardware to a small non-
negative value (e.g., 32 for the i386 shll instruction)
so there's no way to shift a value by a negative number
of bits or by more than there are bits in the first
operand. As a result, GCC can compute a different result
than common hardware. For example, it substitutes 0 for
the result of 1 << -1 while x86 computes INT_MIN)

In contrast, shifting a negative value by a positive number
of bits does have a natural meaning (i.e., shifting the bit
pattern the same way as unsigned). The reason why it's
undefined in C and C++ is because some processors don't
shift the sign bit out and may raise an overflow when
a one bit is shifted into the sign position (typically
those that provide an arithmetic left shift). But most
processors implement a logical left shift and behave
the same way for signed operands as for unsigned.

The result of a left shift of a negative number computed
by GCC matches that of hardware that doesn't differentiate
between arithmetic and logical left shifts (which is all
the common CPUs, including ARM, MIPS, PowerPC, x86), so
the only value in diagnosing it is portability to rare
CPUs or to compilers that behave differently than GCC
(if there are any).

I checked Clang to see what it does. It generates the
same code as GCC but only issues a warning for negative
left shifts. With -Wpedantic, it does give a similar
warning for left shifts of negative values. I recommend
GCC to do the same.

Martin