Message ID | alpine.DEB.2.21.1902062135410.7523@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
Series | Avoid some left-shifts of negative constants | expand |
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.
diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c index 02c886c9cd..c179c533a5 100644 --- a/sysdeps/x86/cacheinfo.c +++ b/sysdeps/x86/cacheinfo.c @@ -619,7 +619,7 @@ init_cacheinfo (void) /* Compute count mask. */ asm ("bsr %1, %0" : "=r" (count_mask) : "g" (threads_l2)); - count_mask = ~(-1 << (count_mask + 1)); + count_mask = ~(-1U << (count_mask + 1)); threads_l2 = (shipped - 1) & count_mask; count &= ~0x1; } @@ -636,7 +636,7 @@ init_cacheinfo (void) /* Compute count mask. */ asm ("bsr %1, %0" : "=r" (count_mask) : "g" (threads_core)); - count_mask = ~(-1 << (count_mask + 1)); + count_mask = ~(-1U << (count_mask + 1)); threads_core = (shipped - 1) & count_mask; if (level == 2) threads_l2 = threads_core;