[Bug c++/50012] New: C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[Bug c++/50012] New: C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

           Summary: C++ front end misses -Wsign-compare warnings when
                    extraneous parentheses are present
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: mikpe@it.uu.se


In this test case two similar functions compare an unsigned int with a signed
int, differing only in whether the unsigned sub-expression has an extra pair of
parentheses around it or not.  The C FE correctly issues a -Wsign-compare
diagnostic for both functions, but the C++ FE fails to do so for the function
with the extra parentheses.

> cat bug.c
int foo(unsigned int *a, int b)
{
    return (*a) <= b;
}

int bar(unsigned int *a, int b)
{
    return *a <= b;
}
> gcc/xgcc -Bgcc/ -O2 -Wsign-compare -S bug.c
bug.c: In function 'foo':
bug.c:3:17: warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]
bug.c: In function 'bar':
bug.c:8:15: warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]
> gcc/g++ -Bgcc/ -O2 -Wsign-compare -S /tmp/bug.c
/tmp/bug.c: In function 'int bar(unsigned int*, int)':
/tmp/bug.c:8:18: warning: comparison between signed and unsigned integer
expressions [-Wsign-compare]

This difference exists in 4.7-20110806, 4.6.1, and 4.5.3, but not in 4.4.6.

The test case is derived from ipa-inline-analysis.c, specifically Jan Hubicka's
recent change in r177484.  That change introduced a signed/unsigned comparison,
but due to this C++ FE bug and the recent "build stage2 etc with g++ by
default" change, the error went undetected.  That is, until people started
using --disable-build-poststage1-with-cxx to reduce bootstrap times, where it
ultimately breaks bootstrap, see PR50005.

[Bug c++/50012] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Mikael Pettersson <mikpe at it dot uu.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ian at gcc dot gnu.org
      Known to work|                            |4.4.6
      Known to fail|                            |4.5.3, 4.6.1, 4.7.0

--- Comment #1 from Mikael Pettersson <mikpe at it dot uu.se> 2011-08-07 19:52:15 UTC ---
It's caused by r148952:

Author: ian
Date: Thu Jun 25 19:20:59 2009
New Revision: 148952

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=148952
Log:
cp/:
    * call.c (avoid_sign_compare_warnings): New static function.
    (build_new_op): Call it.
    * typeck.c (cp_build_binary_op): Don't call warn_sign_compare if
    TREE_NO_WARNING is set on either operand.

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
   Target Milestone|---                         |4.5.4
            Summary|C++ front end misses        |[4.5/4.6/4.7 Regression]
                   |-Wsign-compare warnings     |C++ front end misses
                   |when extraneous parentheses |-Wsign-compare warnings
                   |are present                 |when extraneous parentheses
                   |                            |are present

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

--- Comment #2 from Mikael Pettersson <mikpe at it dot uu.se> 2011-08-14 12:25:12 UTC ---
The warning is lost due to this fragment of r148952 (with the rest of r148952
disabled):

--- gcc/cp/typeck.c    (revision 148951)
+++ gcc/cp/typeck.c    (revision 148952)
@@ -4018,6 +4018,8 @@ cp_build_binary_op (location_t location,

       if ((short_compare || code == MIN_EXPR || code == MAX_EXPR)
       && warn_sign_compare
+      && !TREE_NO_WARNING (orig_op0)
+      && !TREE_NO_WARNING (orig_op1)
       /* Do not warn until the template is instantiated; we cannot
          bound the ranges of the arguments until that point.  */
       && !processing_template_decl

In this test case (the foo() function) the LHS is a parenthesized expression,
and cp/semantics.c:finish_parenthesized_expr() contains the following gem:

tree
finish_parenthesized_expr (tree expr)
{
  if (EXPR_P (expr))
    /* This inhibits warnings in c_common_truthvalue_conversion.  */
    TREE_NO_WARNING (expr) = 1;

Disabling either the setting of TREE_NO_WARNING in finish_parenthesized_expr or
the && !TREE_NO_WARNING check in cp_build_binary_op restores the warning for
this test case.  Alas I'm sure something else will break in that case ...

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-08-19 07:29:07 UTC ---
The problem is that we have just a single bit for the disabling of warning, we
don't have a way to selectively disable just some warnings.

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-10-10
     Ever Confirmed|0                           |1

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-10 12:09:52 UTC ---
The problem is that the change by rev. 148952 looks bogus.  Instead of
using TREE_NO_WARNING it should simply strip some promotions at the
point we do the sign-compare warning.

Ian, please look at this regression.

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-12-19 14:43:05 UTC ---
This has been introduced with
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg01402.html
which has been written as a better way of:
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg01393.html
Perhaps the older patch is a better way than this one, which, if I understand
it right, is that ignoring of the promotion at the -Wsign-compare handling
spot?
Setting TREE_NO_WARNING, at least until we have a way to say which warnings we
want to prohibit and which should be kept, is an ugly hack and shouldn't be
taken lightly.

While Ian's patch doesn't set TREE_NO_WARNING in this testcase, it just
disables the warning if any of the operands is TREE_NO_WARNING and uses this
bit for yet another purpose.  The TREE_NO_WARNING bit in this case is set by
finish_parenthesized_expr:
1504      if (EXPR_P (expr))
1505        /* This inhibits warnings in c_common_truthvalue_conversion.  */
1506        TREE_NO_WARNING (expr) = 1;

So, what was wrong with the first approach?
And, perhaps for 4.8 we want to turn TREE_NO_WARNING bit into a bit that just
tells us whether that tree in question is in some hash table (see e.g.
DECL_HAS_VALUE_EXPR_P etc.) and that hash table would contain details what
warnings are we supposed to ignore.  This isn't the only problematic spot.

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-01-05 17:41:47 UTC ---
Ian, ping?

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[ian at airs dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

--- Comment #7 from Ian Lance Taylor <ian at airs dot com> 2012-01-07 01:31:36 UTC ---
Created attachment 26260
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26260
Possible patch

I did not like the first approach because I think that checking c_dialect_cxx
in the generic C family code is quite ugly.  But clearly the current approach
is problematic, and it's actually easy to implement Richi's suggestion.  This
patch is a possible approach, but it doesn't work as is.  It issues a warning
for

  if (TYPE_QUALS (type) == memfn_quals)

in cp;typeck.c.  Here memfn_quals is signed int but TYPE_QUALS, an expression
of enum values winds up unsigned.

[Bug c++/50012] [4.5/4.6/4.7 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[ian at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

--- Comment #8 from ian at gcc dot gnu.org <ian at gcc dot gnu.org> 2012-01-14 00:22:21 UTC ---
Author: ian
Date: Sat Jan 14 00:22:14 2012
New Revision: 183178

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183178
Log:
gcc/:
    PR c++/50012
    * tree.h (TYPE_QUALS): Add cast to int.
    (TYPE_QUALS_NO_ADDR_SPACE): Likewise.
gcc/cp:
    PR c++/50012
    * typeck.c (enum_cast_to_int): New static function.
    (cp_build_binary_op): When handling warn_sign_compare, don't test
    for TREE_NO_WARNING.  Do call enum_cast_to_int.
    * call.c (avoid_sign_compare_warnings): Remove static function.
    (build_new_op_1): Don't call avoid_sign_compare_warnings.
gcc/testsuite/:
    PR c++/50012
    * g++.dg/warn/Wsign-compare-4.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wsign-compare-4.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/cp/typeck.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree.h

[Bug c++/50012] [4.5/4.6 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[ian at airs dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Ian Lance Taylor <ian at airs dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[4.5/4.6/4.7 Regression]    |[4.5/4.6 Regression] C++
                   |C++ front end misses        |front end misses
                   |-Wsign-compare warnings     |-Wsign-compare warnings
                   |when extraneous parentheses |when extraneous parentheses
                   |are present                 |are present
      Known to fail|4.7.0                       |

--- Comment #9 from Ian Lance Taylor <ian at airs dot com> 2012-01-14 00:23:37 UTC ---
Fixed in mainline, the future 4.7.  Still open in 4.5 and 4.6.

[Bug c++/50012] [4.5/4.6 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.4                       |4.6.4

--- Comment #10 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-07-02 12:06:49 UTC ---
The 4.5 branch is being closed, adjusting target milestone.

[Bug c++/50012] [4.6 Regression] C++ front end misses -Wsign-compare warnings when extraneous parentheses are present

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|ian at airs dot com, ian at |
                   |gcc dot gnu.org             |
         Resolution|                            |FIXED

--- Comment #11 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-11-13 11:23:58 UTC ---
Fixed.