* [PATCH] c-family: Fix up -Wsign-compare BIT_NOT_EXPR handling [PR107465]
@ 2022-11-22 9:32 Jakub Jelinek
2022-11-23 10:37 ` [PATCH] c-family: Incremental fix for " Jakub Jelinek
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2022-11-22 9:32 UTC (permalink / raw)
To: Jason Merrill, Joseph S. Myers, Marek Polacek; +Cc: gcc-patches
Hi!
The following patch fixes multiple bugs in warn_for_sign_compare related to
the BIT_NOT_EXPR related warnings.
My understanding is that what those 3 warnings are meant to warn (since 1995
apparently) is the case where we have BIT_NOT_EXPR of a zero-extended
value, so in result_type the value is something like:
0b11111111XXXXXXXX (e.g. ~ of a 8->16 bit zero extension)
0b000000000000000011111111XXXXXXXX (e.g. ~ of a 8->16 bit zero extension
then zero extended to 32 bits)
0b111111111111111111111111XXXXXXXX (e.g. ~ of a 8->16 bit zero extension
then sign extended to 32 bits)
and the intention of the warning is to warn when this is compared against
something that has some 0 bits at the place where the above has guaranteed
1 bits, either ensured through comparison against constant where we know
the bits exactly, or through zero extension from some narrower type where
again we know at least some upper bits are zero extended.
The bugs in the warning code are:
1) misunderstanding of the {,c_common_}get_narrower APIs - the unsignedp
it sets is only meaningful if the function actually returns something
narrower (in that case it says whether the narrower value is then
sign (0) or zero (1) extended to the originally passed value.
Though op0 or op1 at this point might be already narrower than
result_type, and if the function doesn't return anything narrower,
it all depends on whether the passed in op{0,1} had TYPE_UNSIGNED
type or not
2) the code didn't check at all whether the BIT_NOT_EXPR operand
was actually zero extended (i.e. that it was narrower and unsignedp
was set to 1 for it), all it did is check that unsignedp from the
call was 1. But that isn't well defined thing, if the argument
is returned as is, the function sets unsignedp to 0, but if there
is e.g. a useless cast to the same or compatible type in between,
it can return 1 if the cast is unsigned; now, if BIT_NOT_EXPR
operand is not zero extended, we know nothing at all about any bits
in the operand containing BIT_NOT_EXPR, so there is nothing to warn
about
3) the code was actually testing both operands after calling
c_common_get_narrower on them and on the one with BIT_NOT_EXPR
again for constants; I think that is just wrong in case the BIT_NOT_EXPR
operand wouldn't be fully folded, the warning makes sense only if the
other operand not having BIT_NOT_EXPR in it is constant
4) as can be seen from the above bit pattern examples, the upper bits above
(in the patch arg0) aren't always all 1s, there could be some zero extension
above it and from it one would have 0s, so that needs to be taken into
account for the choice which constant bits to test for being always set
otherwise warning is emitted, or for the zero extension guaranteed zero
bits
5) the patch also simplifies the handling, we only do it if one but not
both operands are BIT_NOT_EXPR after first {,c_common_}get_narrower,
so we can just use std::swap to ensure it is the first one
6) the code compared bits against HOST_BITS_PER_LONG, which made sense
back in 1995 when the values were stored into long, but now that they
are HOST_WIDE_INT should test HOST_BITS_PER_WIDE_INT (or we could rewrite
the stuff to wide_int, not done in the patch)
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2022-11-21 Jakub Jelinek <jakub@redhat.com>
PR c/107465
* c-warn.cc (warn_for_sign_compare): If c_common_get_narrower
doesn't return a narrower result, use TYPE_UNSIGNED to set unsignedp0
and unsignedp1. For the one BIT_NOT_EXPR case vs. one without,
only check for constant in the non-BIT_NOT_EXPR operand, use std::swap
to simplify the code, only warn if BIT_NOT_EXPR operand is extended
from narrower unsigned, fix up computation of mask for the constant
cases and for unsigned other operand case handle differently
BIT_NOT_EXPR result being sign vs. zero extended.
* c-c++-common/Wsign-compare-2.c: New test.
* c-c++-common/pr107465.c: New test.
--- gcc/c-family/c-warn.cc.jj 2022-10-28 11:00:53.738247032 +0200
+++ gcc/c-family/c-warn.cc 2022-11-21 19:29:42.351885444 +0100
@@ -2344,42 +2344,50 @@ warn_for_sign_compare (location_t locati
have all bits set that are set in the ~ operand when it is
extended. */
- op0 = c_common_get_narrower (op0, &unsignedp0);
- op1 = c_common_get_narrower (op1, &unsignedp1);
+ tree arg0 = c_common_get_narrower (op0, &unsignedp0);
+ if (TYPE_PRECISION (TREE_TYPE (arg0)) == TYPE_PRECISION (TREE_TYPE (op0)))
+ unsignedp0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+ op0 = arg0;
+ tree arg1 = c_common_get_narrower (op1, &unsignedp1);
+ if (TYPE_PRECISION (TREE_TYPE (arg1)) == TYPE_PRECISION (TREE_TYPE (op1)))
+ unsignedp1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+ op1 = arg1;
if ((TREE_CODE (op0) == BIT_NOT_EXPR)
^ (TREE_CODE (op1) == BIT_NOT_EXPR))
{
- if (TREE_CODE (op0) == BIT_NOT_EXPR)
- op0 = c_common_get_narrower (TREE_OPERAND (op0, 0), &unsignedp0);
if (TREE_CODE (op1) == BIT_NOT_EXPR)
- op1 = c_common_get_narrower (TREE_OPERAND (op1, 0), &unsignedp1);
-
- if (tree_fits_shwi_p (op0) || tree_fits_shwi_p (op1))
{
- tree primop;
- HOST_WIDE_INT constant, mask;
- int unsignedp;
- unsigned int bits;
+ std::swap (op0, op1);
+ std::swap (unsignedp0, unsignedp1);
+ }
- if (tree_fits_shwi_p (op0))
- {
- primop = op1;
- unsignedp = unsignedp1;
- constant = tree_to_shwi (op0);
- }
- else
- {
- primop = op0;
- unsignedp = unsignedp0;
- constant = tree_to_shwi (op1);
- }
+ int unsignedp;
+ arg0 = c_common_get_narrower (TREE_OPERAND (op0, 0), &unsignedp);
- bits = TYPE_PRECISION (TREE_TYPE (primop));
- if (bits < TYPE_PRECISION (result_type)
- && bits < HOST_BITS_PER_LONG && unsignedp)
+ /* For these warnings, we need BIT_NOT_EXPR operand to be
+ zero extended from narrower type to BIT_NOT_EXPR's type.
+ In that case, all those bits above the narrower's type
+ are after BIT_NOT_EXPR set to 1. */
+ if (tree_fits_shwi_p (op1))
+ {
+ HOST_WIDE_INT constant = tree_to_shwi (op1);
+ unsigned int bits = TYPE_PRECISION (TREE_TYPE (arg0));
+ if (unsignedp
+ && bits < TYPE_PRECISION (TREE_TYPE (op0))
+ && bits < HOST_BITS_PER_WIDE_INT)
{
- mask = HOST_WIDE_INT_M1U << bits;
+ HOST_WIDE_INT mask = HOST_WIDE_INT_M1U << bits;
+ if (unsignedp0)
+ {
+ bits = TYPE_PRECISION (TREE_TYPE (op0));
+ if (bits < TYPE_PRECISION (result_type)
+ && bits < HOST_BITS_PER_WIDE_INT)
+ mask &= ~(HOST_WIDE_INT_M1U << bits);
+ }
+ bits = TYPE_PRECISION (result_type);
+ if (bits < HOST_BITS_PER_WIDE_INT)
+ mask &= ~(HOST_WIDE_INT_M1U << bits);
if ((mask & constant) != mask)
{
if (constant == 0)
@@ -2393,11 +2401,28 @@ warn_for_sign_compare (location_t locati
}
}
}
- else if (unsignedp0 && unsignedp1
- && (TYPE_PRECISION (TREE_TYPE (op0))
- < TYPE_PRECISION (result_type))
+ else if ((TYPE_PRECISION (TREE_TYPE (arg0))
+ < TYPE_PRECISION (TREE_TYPE (op0)))
+ && unsignedp
+ && unsignedp1
+ /* If unsignedp0, the BIT_NOT_EXPR result is
+ zero extended, so say if op0 is unsigned char
+ variable, BIT_NOT_EXPR is unsigned short and
+ result type int and op0 has value 0x55, the
+ int value will be 0xffaa, or for op0 0xaa it
+ will be 0xff55. In these cases, warn if
+ op1 is unsigned and narrower than unsigned short.
+ While if unsignedp0 is false, the BIT_NOT_EXPR
+ result is sign extended and because of the
+ above TYPE_PRECISION comparison we know the
+ MSB of BIT_NOT_EXPR is set (perhaps with some
+ further bits below it). The sign extension will
+ then ensure all bits above BIT_NOT_EXPR up to
+ result_type's precision are set. */
&& (TYPE_PRECISION (TREE_TYPE (op1))
- < TYPE_PRECISION (result_type)))
+ < TYPE_PRECISION (unsignedp0
+ ? TREE_TYPE (op0)
+ : result_type)))
warning_at (location, OPT_Wsign_compare,
"comparison of promoted bitwise complement "
"of an unsigned value with unsigned");
--- gcc/testsuite/c-c++-common/Wsign-compare-2.c.jj 2022-11-21 19:40:24.968468682 +0100
+++ gcc/testsuite/c-c++-common/Wsign-compare-2.c 2022-11-21 20:15:04.706019401 +0100
@@ -0,0 +1,106 @@
+/* { dg-do compile { target { ilp32 || lp64 } } } */
+/* { dg-options "-Wsign-compare" } */
+
+int
+f1 (unsigned char x)
+{
+ return (unsigned short) (~(unsigned short) x) == 0; /* { dg-warning "promoted bitwise complement of an unsigned value is always nonzero" "" { target c } } */
+}
+
+int
+f2 (unsigned char x)
+{
+ return (unsigned short) (~(unsigned short) x) == 5; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with constant" "" { target c } } */
+}
+
+int
+f3 (unsigned char x)
+{
+ return (unsigned int) (~(unsigned short) x) == 0xffff0005U; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
+
+int
+f4 (unsigned char x)
+{
+ return (unsigned int) (~(unsigned short) x) == 0xffff0005ULL; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
+
+int
+f5 (unsigned char x)
+{
+ return (unsigned int) (~(unsigned short) x) == 0xffffff05U; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
+
+int
+f6 (unsigned char x)
+{
+ return (unsigned int) (~(unsigned short) x) == 0xffffff05ULL; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
+
+int
+f7 (unsigned char x)
+{
+ return (unsigned long long) (~(unsigned short) x) == 0xffffffffffffff05ULL; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
+
+typedef unsigned short T;
+
+int
+f8 (T x)
+{
+ return (unsigned short) (~(unsigned short) x) == 0; /* { dg-bogus "promoted bitwise complement of an unsigned value is always nonzero" } */
+}
+
+int
+f9 (T x)
+{
+ return (unsigned short) (~(unsigned short) x) == 5; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
+
+int
+f10 (T x, unsigned char y)
+{
+ return (unsigned short) (~(unsigned short) x) == y; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
+}
+
+int
+f11 (T x, unsigned char y)
+{
+ return (unsigned short) (~(unsigned short) x) == y; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
+}
+
+int
+f12 (unsigned char x, unsigned char y)
+{
+ return (unsigned short) (~(unsigned short) x) == y; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with unsigned" "" { target c } } */
+}
+
+int
+f13 (unsigned char x, unsigned char y)
+{
+ return (unsigned short) (~(unsigned short) x) == y; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with unsigned" "" { target c } } */
+}
+
+int
+f14 (unsigned char x, unsigned int y)
+{
+ return (unsigned long long) (~x) == y; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
+}
+
+int
+f15 (unsigned short x, unsigned int y)
+{
+ return (long long) (~x) == y; /* { dg-warning "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
+}
+
+int
+f16 (unsigned char x, unsigned short y)
+{
+ return (unsigned short) (~(unsigned short) x) == y; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
+}
+
+int
+f17 (unsigned char x, unsigned short y)
+{
+ return (unsigned short) (~(unsigned short) x) == y; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
+}
--- gcc/testsuite/c-c++-common/pr107465.c.jj 2022-11-21 16:31:01.052460446 +0100
+++ gcc/testsuite/c-c++-common/pr107465.c 2022-11-21 16:30:45.512687167 +0100
@@ -0,0 +1,22 @@
+/* PR c/107465 */
+/* { dg-do compile } */
+/* { dg-options "-Wsign-compare" } */
+
+void baz (void);
+typedef unsigned short T;
+
+#if __SIZEOF_SHORT__ * __CHAR_BIT__ == 16
+void
+foo (unsigned short x)
+{
+ if (!(x ^ 0xFFFF))
+ baz ();
+}
+
+void
+bar (T x)
+{
+ if (!(x ^ 0xFFFF)) /* { dg-bogus "promoted bitwise complement of an unsigned value is always nonzero" } */
+ baz ();
+}
+#endif
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH] c-family: Incremental fix for -Wsign-compare BIT_NOT_EXPR handling [PR107465]
2022-11-22 9:32 [PATCH] c-family: Fix up -Wsign-compare BIT_NOT_EXPR handling [PR107465] Jakub Jelinek
@ 2022-11-23 10:37 ` Jakub Jelinek
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Jelinek @ 2022-11-23 10:37 UTC (permalink / raw)
To: Jason Merrill, Joseph S. Myers, Marek Polacek; +Cc: gcc-patches
Hi!
There can be too many extensions and seems I didn't get everything right in
the previously posted patch.
The following incremental patch ought to fix that.
The code can deal with quite a few sign/zero extensions at various spots
and it is important to deal with all of them right.
On the argument that contains BIT_NOT_EXPR we have:
MSB bits#4 bits#3 BIT_NOT_EXPR bits#2 bits#1 LSB
where bits#1 is one or more bits (TYPE_PRECISION (TREE_TYPE (arg0))
at the end of the function) we don't know anything about, for the purposes
of this warning it is VARYING that is inverted with BIT_NOT_EXPR to some other
VARYING bits;
bits#2 is one or more bits (TYPE_PRECISION (TREE_TYPE (op0)) -
TYPE_PRECISION (TREE_TYPE (arg0)) at the end of the function)
which are known to be 0 before the BIT_NOT_EXPR and 1 after it.
bits#3 is zero or more bits from the TYPE_PRECISION (TREE_TYPE (op0))
at the end of function to the TYPE_PRECISION (TREE_TYPE (op0)) at the
end of the function to TYPE_PRECISION (TREE_TYPE (op0)) at the start
of the function, which are either zero extension or sign extension.
And bits#4 is zero or more bits from the TYPE_PRECISION (TREE_TYPE (op0))
at the start of the function to TYPE_PRECISION (result_type), which
again can be zero or sign extension.
Now, vanilla trunk as well as the previously posted patch mishandles the
case where bits#3 are sign extended (as bits#2 are known to be all set,
that means bits#3 are all set too) but bits#4 are zero extended and are
thus all 0.
The patch fixes it by tracking the lowest bit which is known to be clear
above the known to be set bits (if any, otherwise it is precision of
result_type).
Ok for trunk if it passes bootstrap/regtest?
2022-11-23 Jakub Jelinek <jakub@redhat.com>
PR c/107465
* c-warn.cc (warn_for_sign_compare): Don't warn for unset bits
above innermost zero extension of BIT_NOT_EXPR result.
* c-c++-common/Wsign-compare-2.c (f18): New test.
--- gcc/c-family/c-warn.cc.jj 2022-11-23 10:04:53.000000000 +0100
+++ gcc/c-family/c-warn.cc 2022-11-23 11:19:38.928113842 +0100
@@ -2344,13 +2344,33 @@ warn_for_sign_compare (location_t locati
have all bits set that are set in the ~ operand when it is
extended. */
+ /* bits0 is the bit index of op0 extended to result_type, which will
+ be always 0 and so all bits above it. If there is a BIT_NOT_EXPR
+ in that operand possibly sign or zero extended to op0 and then
+ possibly further sign or zero extended to result_type, bits0 will
+ be the precision of result type if all the extensions involved
+ if any are sign extensions, and will be the place of the innermost
+ zero extension otherwise. We warn only if BIT_NOT_EXPR's operand is
+ zero extended from some even smaller precision, in that case after
+ BIT_NOT_EXPR some bits below bits0 will be guaranteed to be set.
+ Similarly for bits1. */
+ int bits0 = TYPE_PRECISION (result_type);
+ if (TYPE_UNSIGNED (TREE_TYPE (op0)))
+ bits0 = TYPE_PRECISION (TREE_TYPE (op0));
tree arg0 = c_common_get_narrower (op0, &unsignedp0);
if (TYPE_PRECISION (TREE_TYPE (arg0)) == TYPE_PRECISION (TREE_TYPE (op0)))
unsignedp0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+ else if (unsignedp0)
+ bits0 = TYPE_PRECISION (TREE_TYPE (arg0));
op0 = arg0;
+ int bits1 = TYPE_PRECISION (result_type);
+ if (TYPE_UNSIGNED (TREE_TYPE (op1)))
+ bits1 = TYPE_PRECISION (TREE_TYPE (op1));
tree arg1 = c_common_get_narrower (op1, &unsignedp1);
if (TYPE_PRECISION (TREE_TYPE (arg1)) == TYPE_PRECISION (TREE_TYPE (op1)))
unsignedp1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+ else if (unsignedp1)
+ bits1 = TYPE_PRECISION (TREE_TYPE (arg1));
op1 = arg1;
if ((TREE_CODE (op0) == BIT_NOT_EXPR)
@@ -2360,6 +2380,7 @@ warn_for_sign_compare (location_t locati
{
std::swap (op0, op1);
std::swap (unsignedp0, unsignedp1);
+ std::swap (bits0, bits1);
}
int unsignedp;
@@ -2378,16 +2399,8 @@ warn_for_sign_compare (location_t locati
&& bits < HOST_BITS_PER_WIDE_INT)
{
HOST_WIDE_INT mask = HOST_WIDE_INT_M1U << bits;
- if (unsignedp0)
- {
- bits = TYPE_PRECISION (TREE_TYPE (op0));
- if (bits < TYPE_PRECISION (result_type)
- && bits < HOST_BITS_PER_WIDE_INT)
- mask &= ~(HOST_WIDE_INT_M1U << bits);
- }
- bits = TYPE_PRECISION (result_type);
- if (bits < HOST_BITS_PER_WIDE_INT)
- mask &= ~(HOST_WIDE_INT_M1U << bits);
+ if (bits0 < HOST_BITS_PER_WIDE_INT)
+ mask &= ~(HOST_WIDE_INT_M1U << bits0);
if ((mask & constant) != mask)
{
if (constant == 0)
@@ -2405,24 +2418,7 @@ warn_for_sign_compare (location_t locati
< TYPE_PRECISION (TREE_TYPE (op0)))
&& unsignedp
&& unsignedp1
- /* If unsignedp0, the BIT_NOT_EXPR result is
- zero extended, so say if op0 is unsigned char
- variable, BIT_NOT_EXPR is unsigned short and
- result type int and op0 has value 0x55, the
- int value will be 0xffaa, or for op0 0xaa it
- will be 0xff55. In these cases, warn if
- op1 is unsigned and narrower than unsigned short.
- While if unsignedp0 is false, the BIT_NOT_EXPR
- result is sign extended and because of the
- above TYPE_PRECISION comparison we know the
- MSB of BIT_NOT_EXPR is set (perhaps with some
- further bits below it). The sign extension will
- then ensure all bits above BIT_NOT_EXPR up to
- result_type's precision are set. */
- && (TYPE_PRECISION (TREE_TYPE (op1))
- < TYPE_PRECISION (unsignedp0
- ? TREE_TYPE (op0)
- : result_type)))
+ && TYPE_PRECISION (TREE_TYPE (op1)) < bits0)
warning_at (location, OPT_Wsign_compare,
"comparison of promoted bitwise complement "
"of an unsigned value with unsigned");
--- gcc/testsuite/c-c++-common/Wsign-compare-2.c.jj 2022-11-23 10:04:53.000000000 +0100
+++ gcc/testsuite/c-c++-common/Wsign-compare-2.c 2022-11-23 11:05:10.497862219 +0100
@@ -104,3 +104,9 @@ f17 (unsigned char x, unsigned short y)
{
return (unsigned short) (~(unsigned short) x) == y; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with unsigned" } */
}
+
+int
+f18 (unsigned char x)
+{
+ return (unsigned int) (short) (~(unsigned short) x) == 0xffffff05ULL; /* { dg-bogus "comparison of promoted bitwise complement of an unsigned value with constant" } */
+}
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-11-23 10:37 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-22 9:32 [PATCH] c-family: Fix up -Wsign-compare BIT_NOT_EXPR handling [PR107465] Jakub Jelinek
2022-11-23 10:37 ` [PATCH] c-family: Incremental fix for " Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).