Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Gerald Pfeifer]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]


> 2015-04-21  Marek Polacek  <polacek@redhat.com>
>
>	PR c/63357
>	* c-common.c (warn_logical_operator): Warn if the operands have the
>	same expressions.

This is nice!  It really helped me find an issue or two in the 
Wine project.

Unfortunately it also causes false positives:

  int report (unsigned t)
  {
    typedef int r_fun_t (int);

    static r_fun_t * text_funcs[7];
    static r_fun_t * GUI_funcs[7];

    return (t < sizeof text_funcs / sizeof text_funcs[0] &&
            t < sizeof GUI_funcs / sizeof GUI_funcs[0]);
  }

Where we now warn as follows:

  input: In function Β‘reportΒ’:
  input:8:58: warning: logical Β‘andΒ’ of equal expressions [-Wlogical-op]
     return (t < sizeof text_funcs / sizeof text_funcs[0] &&
                                                          ^

In case this example feels too contrived (even though it is an
excerpt of Wine code), we now also warn about the following where
the two types and variables are defined in different places and
the size of one is set implicitly:

  typedef int r_fun_t (int);

  r_fun_t * text_funcs[] = {0,0,0};

  int report (unsigned t)
  {
    typedef int s_fun_t (long, char);

    static s_fun_t * GUI_funcs[3];

    return (t < sizeof text_funcs / sizeof text_funcs[0] &&
            t < sizeof GUI_funcs / sizeof GUI_funcs[0]);
  }

(I also filed this as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65891
so that we keep track.)

Gerald

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Marek Polacek]

On Sat, Apr 25, 2015 at 10:18:59PM +0200, Gerald Pfeifer wrote:
> In case this example feels too contrived (even though it is an
> excerpt of Wine code), we now also warn about the following where
> the two types and variables are defined in different places and
> the size of one is set implicitly:
> 
>   typedef int r_fun_t (int);
> 
>   r_fun_t * text_funcs[] = {0,0,0};
> 
>   int report (unsigned t)
>   {
>     typedef int s_fun_t (long, char);
> 
>     static s_fun_t * GUI_funcs[3];
> 
>     return (t < sizeof text_funcs / sizeof text_funcs[0] &&
>             t < sizeof GUI_funcs / sizeof GUI_funcs[0]);
>   }
> 
> (I also filed this as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65891
> so that we keep track.)

I'm afraid there isn't an easy solution to this; the problem is that we fold
sizeof early, so the warning sees 

  t < sizeof 4 && t < 4

and warns.  Maybe using SIZEOF_EXPR would help...

	Marek

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Marek Polacek]

On Mon, Apr 27, 2015 at 04:47:12PM +0200, Marek Polacek wrote:
> On Sat, Apr 25, 2015 at 10:18:59PM +0200, Gerald Pfeifer wrote:
> > In case this example feels too contrived (even though it is an
> > excerpt of Wine code), we now also warn about the following where
> > the two types and variables are defined in different places and
> > the size of one is set implicitly:
> > 
> >   typedef int r_fun_t (int);
> > 
> >   r_fun_t * text_funcs[] = {0,0,0};
> > 
> >   int report (unsigned t)
> >   {
> >     typedef int s_fun_t (long, char);
> > 
> >     static s_fun_t * GUI_funcs[3];
> > 
> >     return (t < sizeof text_funcs / sizeof text_funcs[0] &&
> >             t < sizeof GUI_funcs / sizeof GUI_funcs[0]);
> >   }
> > 
> > (I also filed this as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65891
> > so that we keep track.)
> 
> I'm afraid there isn't an easy solution to this; the problem is that we fold
> sizeof early, so the warning sees 
> 
>   t < sizeof 4 && t < 4
> 
> and warns.  Maybe using SIZEOF_EXPR would help...

That said, this isn't something that regressed with my patch.  Consider e.g.:

struct S { int i; };
struct T { int i; };
int
foo (unsigned int t)
{
  return t < sizeof (struct S) && t > sizeof (struct T);
}

with -Wlogical-op:
w2.c: In function ‘foo’:
w2.c:6:32: warning: logical ‘and’ of mutually exclusive tests is always false [-Wlogical-op]
   return t < sizeof (struct S) && t > sizeof (struct T);
                                ^

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Jeff Law]

On 04/27/2015 08:47 AM, Marek Polacek wrote:
> On Sat, Apr 25, 2015 at 10:18:59PM +0200, Gerald Pfeifer wrote:
>> In case this example feels too contrived (even though it is an
>> excerpt of Wine code), we now also warn about the following where
>> the two types and variables are defined in different places and
>> the size of one is set implicitly:
>>
>>    typedef int r_fun_t (int);
>>
>>    r_fun_t * text_funcs[] = {0,0,0};
>>
>>    int report (unsigned t)
>>    {
>>      typedef int s_fun_t (long, char);
>>
>>      static s_fun_t * GUI_funcs[3];
>>
>>      return (t < sizeof text_funcs / sizeof text_funcs[0] &&
>>              t < sizeof GUI_funcs / sizeof GUI_funcs[0]);
>>    }
>>
>> (I also filed this as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65891
>> so that we keep track.)
>
> I'm afraid there isn't an easy solution to this; the problem is that we fold
> sizeof early, so the warning sees
>
>    t < sizeof 4 && t < 4
>
> and warns.  Maybe using SIZEOF_EXPR would help...
Can you file a bug for this for future reference?  We may not tackle 
this specific issue in the current iteration of delayed folding, but it 
certainly helps to have BZs for specific issues that we'd like to fix 
once we can delay folding.

jeff

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Marek Polacek]

On Mon, Apr 27, 2015 at 10:06:26AM -0600, Jeff Law wrote:
> On 04/27/2015 08:47 AM, Marek Polacek wrote:
> >On Sat, Apr 25, 2015 at 10:18:59PM +0200, Gerald Pfeifer wrote:
> >>In case this example feels too contrived (even though it is an
> >>excerpt of Wine code), we now also warn about the following where
> >>the two types and variables are defined in different places and
> >>the size of one is set implicitly:
> >>
> >>   typedef int r_fun_t (int);
> >>
> >>   r_fun_t * text_funcs[] = {0,0,0};
> >>
> >>   int report (unsigned t)
> >>   {
> >>     typedef int s_fun_t (long, char);
> >>
> >>     static s_fun_t * GUI_funcs[3];
> >>
> >>     return (t < sizeof text_funcs / sizeof text_funcs[0] &&
> >>             t < sizeof GUI_funcs / sizeof GUI_funcs[0]);
> >>   }
> >>
> >>(I also filed this as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65891
> >>so that we keep track.)
> >
> >I'm afraid there isn't an easy solution to this; the problem is that we fold
> >sizeof early, so the warning sees
> >
> >   t < sizeof 4 && t < 4
> >
> >and warns.  Maybe using SIZEOF_EXPR would help...
> Can you file a bug for this for future reference?  We may not tackle this
> specific issue in the current iteration of delayed folding, but it certainly
> helps to have BZs for specific issues that we'd like to fix once we can
> delay folding.

Gerald already opened PR65891 for this :).

	Marek

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Jeff Law]

On 04/21/2015 05:16 AM, Marek Polacek wrote:
> This patch improves -Wlogical-op so that it also warns about cases such as
> P && P or P || P.  I made use of what merge_ranges computes: if we have equal
> operands with the same ranges, warn -- that seems to work well.
> (-Wlogical-op still isn't enabled neither by -Wall nor by -Wextra.)
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-04-21  Marek Polacek  <polacek@redhat.com>
>
> 	PR c/63357
> 	* c-common.c (warn_logical_operator): Warn if the operands have the
> 	same expressions.
>
> 	* doc/invoke.texi: Update description of -Wlogical-op.
>
> 	* c-c++-common/Wlogical-op-1.c: New test.
OK.
jeff

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Marek Polacek]

On Wed, Apr 22, 2015 at 11:50:17AM +0200, Manuel López-Ibáñez wrote:
> So, I think your proposal is an improvement, however, it does not
> fully fix the problem. For that, we need to have locations for the
> operands of expressions (PR43486), so we can check if they are also
> come from macro expansion.

Right.  Argh.  I'll post that input.h bit nonetheless, it certainly won't make
things worse...  Thanks.

	Marek

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Manuel López-Ibáñez]

On 22 April 2015 at 11:50, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> So, I think your proposal is an improvement, however, it does not
> fully fix the problem. For that, we need to have locations for the
> operands of expressions (PR43486), so we can check if they are also
> come from macro expansion.

Or, in general, from system headers:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43486#c8

Cheers,

Manuel.

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Manuel López-Ibáñez]

On 22 April 2015 at 10:59, Marek Polacek <polacek@redhat.com> wrote:
> So the bug really must be solved first :(.  I see I proposed a solution in the
> PR, I wonder whether that works...

It will work for

extern int xxx;
#define XXX !xxx
int test (void)
{
  if (XXX && xxx)
    return 4;
  else
    return 0;
}

because the location of '!' is virtual. However, it won't work for:

extern int xxx;
#define XXX xxx
int test (void)
{
  if (!XXX && xxx)
    return 4;
  else
    return 0;
}

So, I think your proposal is an improvement, however, it does not
fully fix the problem. For that, we need to have locations for the
operands of expressions (PR43486), so we can check if they are also
come from macro expansion.

Cheers,

Manuel.

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Marek Polacek]

On Tue, Apr 21, 2015 at 04:19:26PM +0200, Manuel López-Ibáñez wrote:
> On 21/04/15 13:16, Marek Polacek wrote:
> >(-Wlogical-op still isn't enabled neither by -Wall nor by -Wextra.)
> 
> The reason is https://gcc.gnu.org/PR61534
> 
> which means we don't want to warn for:
> 
> extern int xxx;
> #define XXX xxx
> int test (void)
> {
>   if (!XXX && xxx)
>     return 4;
>   else
>     return 0;
> }
> 
> (gcc/testsuite/gcc.dg/pr40172-3.c, although it should be moved to c-c++-common)
> 
> As noted in the PR: The problem is that !XXX becomes XXX == 0, but it has
> the location of "!", which is not virtual. If we look at the argument of the
> expression, then XXX is actually a var_decl, whose location corresponds to
> the declaration and not the use, and it is not virtual either. This is
> PR43486.
> 
> 
> >Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> Does it pass bootstrap if you enable it? That is, is GCC itself -Wlogical-op clean?

No, there are many issues e.g. in config/i386/i386.md exactly because of PR61534,
that is, with this patch, we warn even for

int var;
#define TARGET_64BIT var
#define TARGET_KEEPS_VECTOR_ALIGNED_STACK TARGET_64BIT
int
main ()
{
  if (TARGET_64BIT || TARGET_KEEPS_VECTOR_ALIGNED_STACK)
    return 1;
}

So the bug really must be solved first :(.  I see I proposed a solution in the
PR, I wonder whether that works...

	Marek

Re: [C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Manuel López-Ibáñez]

On 21/04/15 13:16, Marek Polacek wrote:
> (-Wlogical-op still isn't enabled neither by -Wall nor by -Wextra.)

The reason is https://gcc.gnu.org/PR61534

which means we don't want to warn for:

extern int xxx;
#define XXX xxx
int test (void)
{
   if (!XXX && xxx)
     return 4;
   else
     return 0;
}

(gcc/testsuite/gcc.dg/pr40172-3.c, although it should be moved to c-c++-common)

As noted in the PR: The problem is that !XXX becomes XXX == 0, but it has the 
location of "!", which is not virtual. If we look at the argument of the 
expression, then XXX is actually a var_decl, whose location corresponds to the 
declaration and not the use, and it is not virtual either. This is PR43486.


> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Does it pass bootstrap if you enable it? That is, is GCC itself -Wlogical-op clean?

Cheers,

	Manuel.

[C/C++ PATCH] Improve -Wlogical-op (PR c/63357)

[Marek Polacek]

This patch improves -Wlogical-op so that it also warns about cases such as
P && P or P || P.  I made use of what merge_ranges computes: if we have equal
operands with the same ranges, warn -- that seems to work well.
(-Wlogical-op still isn't enabled neither by -Wall nor by -Wextra.)

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-04-21  Marek Polacek  <polacek@redhat.com>

	PR c/63357
	* c-common.c (warn_logical_operator): Warn if the operands have the
	same expressions.

	* doc/invoke.texi: Update description of -Wlogical-op.

	* c-c++-common/Wlogical-op-1.c: New test.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 7fe7fa6..6eecc73 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1772,22 +1772,35 @@ warn_logical_operator (location_t location, enum tree_code code, tree type,
     return;
 
   /* If both expressions have the same operand, if we can merge the
-     ranges, and if the range test is always false, then warn.  */
+     ranges, ...  */
   if (operand_equal_p (lhs, rhs, 0)
       && merge_ranges (&in_p, &low, &high, in0_p, low0, high0,
-		       in1_p, low1, high1)
-      && 0 != (tem = build_range_check (UNKNOWN_LOCATION,
-					type, lhs, in_p, low, high))
-      && integer_zerop (tem))
+		       in1_p, low1, high1))
     {
-      if (or_op)
-        warning_at (location, OPT_Wlogical_op,
-                    "logical %<or%> "
-                    "of collectively exhaustive tests is always true");
-      else
-        warning_at (location, OPT_Wlogical_op,
-                    "logical %<and%> "
-                    "of mutually exclusive tests is always false");
+      tem = build_range_check (UNKNOWN_LOCATION, type, lhs, in_p, low, high);
+      /* ... and if the range test is always false, then warn.  */
+      if (tem && integer_zerop (tem))
+	{
+	  if (or_op)
+	    warning_at (location, OPT_Wlogical_op,
+			"logical %<or%> of collectively exhaustive tests is "
+			"always true");
+	  else
+	    warning_at (location, OPT_Wlogical_op,
+			"logical %<and%> of mutually exclusive tests is "
+			"always false");
+	}
+      /* Or warn if the operands have exactly the same range, e.g.
+	 A > 0 && A > 0.  */
+      else if (low0 == low1 && high0 == high1)
+	{
+	  if (or_op)
+	    warning_at (location, OPT_Wlogical_op,
+			"logical %<or%> of equal expressions");
+	  else
+	    warning_at (location, OPT_Wlogical_op,
+			"logical %<and%> of equal expressions");
+	}
     }
 }
 
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index c20dd4d..8ce233b 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -4936,7 +4936,12 @@ programmer intended to use @code{strcmp}.  This warning is enabled by
 @opindex Wno-logical-op
 Warn about suspicious uses of logical operators in expressions.
 This includes using logical operators in contexts where a
-bit-wise operator is likely to be expected.
+bit-wise operator is likely to be expected.  Also warns when
+the operands of a logical operator are the same:
+@smallexample
+extern int a;
+if (a < 0 && a < 0) @{ @dots{} @}
+@end smallexample
 
 @item -Wlogical-not-parentheses
 @opindex Wlogical-not-parentheses
diff --git gcc/testsuite/c-c++-common/Wlogical-op-1.c gcc/testsuite/c-c++-common/Wlogical-op-1.c
index e69de29..33d4f38 100644
--- gcc/testsuite/c-c++-common/Wlogical-op-1.c
+++ gcc/testsuite/c-c++-common/Wlogical-op-1.c
@@ -0,0 +1,109 @@
+/* PR c/63357 */
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-op" } */
+
+#ifndef __cplusplus
+# define bool _Bool
+# define true 1
+# define false 0
+#endif
+
+extern int bar (void);
+extern int *p;
+struct R { int a, b; } S;
+
+void
+andfn (int a, int b)
+{
+  if (a && a) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if (!a && !a) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if (!!a && !!a) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if (a > 0 && a > 0) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if (a < 0 && a < 0) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if (a == 0 && a == 0) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if (a <= 0 && a <= 0) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if (a >= 0 && a >= 0) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if (a == 0 && !(a != 0)) {}	/* { dg-warning "logical .and. of equal expressions" } */
+
+  if (a && a && a) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if ((a + 1) && (a + 1)) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if ((10 * a) && (a * 10)) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if (!!a && a) {}		/* { dg-warning "logical .and. of equal expressions" } */
+
+  if (*p && *p) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if (p[0] && p[0]) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if (S.a && S.a) {}		/* { dg-warning "logical .and. of equal expressions" } */
+  if ((bool) a && (bool) a) {}	/* { dg-warning "logical .and. of equal expressions" } */
+  if ((unsigned) a && a) {}	/* { dg-warning "logical .and. of equal expressions" } */
+
+  /* Stay quiet here.  */
+  if (a && b) {}
+  if (!a && !b) {}
+  if (!!a && !!b) {}
+  if (a > 0 && b > 0) {}
+  if (a < 0 && b < 0) {}
+  if (a == 0 && b == 0) {}
+  if (a <= 0 && b <= 0) {}
+  if (a >= 0 && b >= 0) {}
+
+  if (a > 0 && a > 1) {}
+  if (a > -2 && a > 1) {}
+  if (a && (short) a) {}
+  if ((char) a && a) {}
+  if (++a && a) {}
+  if (++a && ++a) {}
+  if (a && --a) {}
+  if (a && a / 2) {}
+  if (bar () && bar ()) {}
+  if (p && *p) {}
+  if (p[0] && p[1]) {}
+  if (S.a && S.b) {}
+}
+
+void
+orfn (int a, int b)
+{
+  if (a || a) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if (!a || !a) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if (!!a || !!a) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if (a > 0 || a > 0) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if (a < 0 || a < 0) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if (a == 0 || a == 0) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if (a <= 0 || a <= 0) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if (a >= 0 || a >= 0) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if (a == 0 || !(a != 0)) {}	/* { dg-warning "logical .or. of equal expressions" } */
+
+  if (a || a || a) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if ((a + 1) || (a + 1)) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if ((10 * a) || (a * 10)) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if (!!a || a) {}		/* { dg-warning "logical .or. of equal expressions" } */
+
+  if (*p || *p) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if (p[0] || p[0]) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if (S.a || S.a) {}		/* { dg-warning "logical .or. of equal expressions" } */
+  if ((bool) a || (bool) a) {}	/* { dg-warning "logical .or. of equal expressions" } */
+  if ((unsigned) a || a) {}	/* { dg-warning "logical .or. of equal expressions" } */
+
+  /* Stay quiet here.  */
+  if (a || b) {}
+  if (!a || !b) {}
+  if (!!a || !!b) {}
+  if (a > 0 || b > 0) {}
+  if (a < 0 || b < 0) {}
+  if (a == 0 || b == 0) {}
+  if (a <= 0 || b <= 0) {}
+  if (a >= 0 || b >= 0) {}
+
+  if (a > 0 || a > 1) {}
+  if (a > -2 || a > 1) {}
+  if (a || (short) a) {}
+  if ((char) a || a) {}
+  if (++a || a) {}
+  if (++a || ++a) {}
+  if (a || --a) {}
+  if (a || a / 2) {}
+  if (bar () || bar ()) {}
+  if (p || *p) {}
+  if (p[0] || p[1]) {}
+  if (S.a || S.b) {}
+}

	Marek