Avoid some left-shifts of negative constants

  One group of warnings seen with -Wextra is "left shift of negative
value [-Wshift-negative-value]".  These may be hard to eliminate
completely (some of them involve constants whose type ends up given by
a typedef name, rather than simply int), but it still seems worth
cleaning them up in other cases.  This patch changes two places that
trigger this warning to shift -1U instead of -1.

Tested for x86_64.

2019-02-06  Joseph Myers  <joseph@codesourcery.com>

	* sysdeps/x86/cacheinfo.c (init_cacheinfo): Left-shift -1U instead
	of -1.
  

On 2/6/19 1:37 PM, Joseph Myers wrote:
> One group of warnings seen with -Wextra is "left shift of negative
> value [-Wshift-negative-value]".

This group of warnings is make-work, right? Glibc assumes GCC, and in 
GCC the left shift of a negative value is well-defined unless it 
overflows (which can't happen in this case). So I suggest that we add 
-Wno-shift-negative-value to CFLAGS instead of worrying about pacifying 
the compiler by munging the source code.
  

On Thu, 7 Feb 2019, Paul Eggert wrote:

> On 2/6/19 1:37 PM, Joseph Myers wrote:
> > One group of warnings seen with -Wextra is "left shift of negative
> > value [-Wshift-negative-value]".
> 
> This group of warnings is make-work, right? Glibc assumes GCC, and in GCC the
> left shift of a negative value is well-defined unless it overflows (which
> can't happen in this case). So I suggest that we add -Wno-shift-negative-value
> to CFLAGS instead of worrying about pacifying the compiler by munging the
> source code.

In this particular case (constructing a mask value), using an unsigned 
constant seems cleaner to me than using a signed one.

We may well end up with -Wno-shift-negative-value because of the 
hard-to-fix cases, although I think there may well be utility in being 
able to build much of glibc with UBSAN to find bugs, which might indicate 
more generally avoiding such shifts even when in fact harmless with GCC.
  

On 2/7/19 8:52 AM, Joseph Myers wrote:
> In this particular case (constructing a mask value), using an unsigned
> constant seems cleaner to me than using a signed one.

That's a minor style preference, and the other side of it is that 
replacing -1 with -1U will break if the value being masked is later 
widened to 'long' for whatever reason, which gives a slight 
style/portability edge to leaving the code alone. (Plus, -1U is uglier 
and we shouldn't let GCC push us around. :-)

> there may well be utility in being
> able to build much of glibc with UBSAN to find bugs, which might indicate
> more generally avoiding such shifts even when in fact harmless with GCC.
There would be even greater utility in having UBSAN catch only real bugs 
instead of sending us off on wild-goose cases that result in code that's 
more fragile. Surely it would be easy to fix UBSAN to not report an 
error for a left shift of a negative value, for applications like glibc 
that are willing to rely on GCC's semantics.
  

On Thu, 7 Feb 2019, Paul Eggert wrote:

> fragile. Surely it would be easy to fix UBSAN to not report an error for a
> left shift of a negative value, for applications like glibc that are willing
> to rely on GCC's semantics.

I think such errors are like many compiler warnings - they may not 
necessarily indicate a bug, but they indicate something suspicious in the 
code, that requires extra thought to determine whether the code there is 
correct or not (compared to code doing shifts on unsigned values that can 
be more obviously correct without thinking about what happens with sign 
bits in that particular case).  If the code is doing left shifts of 
negative values, that may well indicate an unsigned type would have made 
the intended semantics clearer, even if a signed type does work in that 
code.
  

On 2/7/19 9:14 AM, Joseph Myers wrote:
> If the code is doing left shifts of
> negative values, that may well indicate an unsigned type would have made
> the intended semantics clearer, even if a signed type does work in that
> code.

Although that might be true elsewhere, there is a reasonable argument 
that the code is clearer as-is for this particular case, as it's more 
robust in the presence of integer-width changes. I'd like to see cases 
where the code is obviously clearer after being rewritten to use 
unsigned shifts, before conceding that it's worthwhile to spend time to 
pacify -Wshift-negative-value.
  

On Thu, 7 Feb 2019, Paul Eggert wrote:

> On 2/7/19 9:14 AM, Joseph Myers wrote:
> > If the code is doing left shifts of
> > negative values, that may well indicate an unsigned type would have made
> > the intended semantics clearer, even if a signed type does work in that
> > code.
> 
> Although that might be true elsewhere, there is a reasonable argument that the
> code is clearer as-is for this particular case, as it's more robust in the
> presence of integer-width changes. I'd like to see cases where the code is

Such changes (if a future processor supports 2^31 or more hardware threads 
sharing the same cache) would require the constant to change anyway, 
because they only make sense if you might be shifting by 32 or more, so -1 
would also be invalid.  They'd also require various other changes in the 
code, given that the existing CPUID interfaces used by this code use 
fields too narrow to return such large values.
  

Ping.  Does anyone else wish to comment on this patch 
<https://sourceware.org/ml/libc-alpha/2019-02/msg00177.html> as a cleanup 
in this particular case (where as discussed, in any case -1U would be 
problematic because of a change of types, -1 would be problematic as well 
because of possible shifts by 32 or more)?
  

On Feb 11 2019, Joseph Myers <joseph@codesourcery.com> wrote:

> Ping.  Does anyone else wish to comment on this patch 
> <https://sourceware.org/ml/libc-alpha/2019-02/msg00177.html> as a cleanup 
> in this particular case (where as discussed, in any case -1U would be 
> problematic because of a change of types, -1 would be problematic as well 
> because of possible shifts by 32 or more)?

A shift by 32 or more would be problematic in any case.

Andreas.