[Bug c/51712] New: -Wtype-limits should not trigger for types of implementation-defined signedness

[Bug c/51712] New: -Wtype-limits should not trigger for types of implementation-defined signedness

[jrnieder at gmail dot com]

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

             Bug #: 51712
           Summary: -Wtype-limits should not trigger for types of
                    implementation-defined signedness
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jrnieder@gmail.com
                CC: manu@gcc.gnu.org


Hi,

$ gcc -c -std=gnu99 -Wtype-limits -x c - <<-\EOF
enum test_enum {
    FOO,
    BAR
};

int valid(enum test_enum arg)
{
    return arg >= FOO && arg <= BAR;
}
EOF
<stdin>: In function ‘valid’:
<stdin>:8:9: warning: comparison of unsigned expression >= 0 is always true
[-Wtype-limits]
$ 

Since C99 (WG14/N1256 p105, lang.decl.typespec.enum.4) only says:

    Each enumerated type shall be compatible with char, a signed
    integer type, or an unsigned integer type. The choice of type
    is implementation-defined) but shall be capable of
    representing the values of all the members of the enumeration.

the (arg >= FOO) test is not actually redundant. It would be nice to
automatically suppress the warning in this case.

What do you think? Feasible?

Originally reported as http://bugs.debian.org/615525

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

--- Comment #1 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-03-26 10:35:24 UTC ---
My answer would be: What does Clang do in this case?

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[jrnieder at gmail dot com]

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

--- Comment #2 from Jonathan Nieder <jrnieder at gmail dot com> 2012-03-26 20:12:17 UTC ---
Clang does not consider "arg >= FOO" to be a comparison against 0.

| commit e3b159c0
| Author: Ted Kremenek <kremenek@apple.com>
| Date:   Thu Sep 23 21:43:44 2010 +0000
|
|     When warning about comparing an unsigned int to being >= 0, don't issue a
warning if the zero value was an
|     enum or was expanded from a macro.
|
|     Fixes: <rdar://problem/8414119>
|
|     git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@114695
91177308-0d34-0410-b5e6-96231b3b80d8
|
| diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
| index 790e7671..c4e86875 100644
| --- a/lib/Sema/SemaChecking.cpp
| +++ b/lib/Sema/SemaChecking.cpp
| @@ -2449,7 +2449,17 @@ bool IsSameFloatAfterCast(const APValue &value,
|  
|  void AnalyzeImplicitConversions(Sema &S, Expr *E);
|  
| -bool IsZero(Sema &S, Expr *E) {
| +static bool IsZero(Sema &S, Expr *E) {
| +  // Suppress cases where we are comparing against an enum constant.
| +  if (const DeclRefExpr *DR =
| +      dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
| +    if (isa<EnumConstantDecl>(DR->getDecl()))
| +      return false;
| +
| +  // Suppress cases where the '0' value is expanded from a macro.
| +  if (E->getLocStart().isMacroID())
| +    return false;
| +
|    llvm::APSInt Value;
|    return E->isIntegerConstantExpr(Value, S.Context) && Value == 0;
|  }
[...]

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-04-17
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |joseph at codesourcery dot
                   |                            |com
     Ever Confirmed|0                           |1

--- Comment #3 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-17 17:49:58 UTC ---
Unfortunately, I cannot see any way to know that FOO is a constant that comes
from an enum. Joseph, is this possible to do in the C FE? 

Also, constants don't have locations in GCC, so no way to know that it comes
from a macro. Sorry!

This a problem also in G++, but in this testcase it doesn't trigger because arg
is considered to be signed. I guess it can trigger in other testcases.

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[jrnieder at gmail dot com]

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

--- Comment #4 from Jonathan Nieder <jrnieder at gmail dot com> 2012-04-17 18:13:08 UTC ---
I am not convinced clang's heuristic is the right one.  For example, the
following code trips clang's warning, but the test is still not redundant. The
main advantage of the "enumerators equal to zero are not zero constants"
heuristic is that it means there is at least one idiom for checking that an
enum value is in range that clang won't warn about.

enum test_enum {
    FOO = 0,
    BAR
};

int valid(enum test_enum arg)
{
    return arg >= 0 && arg <= 1;
}

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

--- Comment #5 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-17 21:27:25 UTC ---
(In reply to comment #4)
> I am not convinced clang's heuristic is the right one.  For example, the
> following code trips clang's warning, but the test is still not redundant. The

I can see that it doesn't handle this case, but it does handle two other common
cases we don't want to warn about, namely, arg >= FOO and when the zero comes
from a macro expansion.

In any case, at the moment of the warning, GCC doesn't  know that arg is an
enum, and avoid warning. But perhaps this case is easier to fix than the one
above.

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[joseph at codesourcery dot com]

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

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> 2012-04-23 21:11:33 UTC ---
On Tue, 17 Apr 2012, manu at gcc dot gnu.org wrote:

> Unfortunately, I cannot see any way to know that FOO is a constant that comes
> from an enum. Joseph, is this possible to do in the C FE? 

The original types of expressions that came from enums are already tracked 
for use of -Wc++-compat (checking if C code is compatible with stricter 
C++ rules for enums).  (This is what c-typeck.c:build_external_ref uses 
the type pointer for: storing the original type of an enum.)  So the 
information is available in at least parts of the C front end.

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

--- Comment #7 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-26 11:59:30 UTC ---
(In reply to comment #6)
> On Tue, 17 Apr 2012, manu at gcc dot gnu.org wrote:
> 
> > Unfortunately, I cannot see any way to know that FOO is a constant that comes
> > from an enum. Joseph, is this possible to do in the C FE? 
> 
> The original types of expressions that came from enums are already tracked 
> for use of -Wc++-compat (checking if C code is compatible with stricter 
> C++ rules for enums).  (This is what c-typeck.c:build_external_ref uses 
> the type pointer for: storing the original type of an enum.)  So the 
> information is available in at least parts of the C front end.

I see two ways to implement this:

* Make build_binary_op take c_expr instead of trees. This will require adding
c_expr to the C++ FE. For now, the C++ FE will just encapsulate the trees into
c_expr before calling  build_binary_op and nothing else. If the long-term goal
is that C/C++ FEs track the original source code more faithfully, I think this
is the right step. The use of c_expr should help to achieve this.

* Pass 4 more arguments to build_binary_op, one for each c_expr and add a
wrapper to avoid changing the interface for C++ FE. This restricts the changes
to the C FE (and c-common.c) but it is really ugly.

Joseph/Jason,

what do you think?

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

--- Comment #8 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-05-03 22:37:06 UTC ---
Author: manu
Date: Thu May  3 22:37:01 2012
New Revision: 187125

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187125
Log:
2012-05-04  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    PR c/51712
c-family/
    * c-common.c (expr_original_type): New.
    (shorten_compare): Do not warn for enumeration types.
testsuite/
    * c-c++-common/pr51712.c: New.

Added:
    trunk/gcc/testsuite/c-c++-common/pr51712.c
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/testsuite/ChangeLog

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

--- Comment #9 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-05-03 23:13:22 UTC ---
The second testcase is fixed now. The original testcase is much harder, but I
have a patch that follows the idea in comment #7. Let's see how it goes...

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[jrnieder at gmail dot com]

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

--- Comment #13 from Jonathan Nieder <jrnieder at gmail dot com> 2012-10-16 07:55:56 UTC ---
Hi Kyrill,

(In reply to comment #11)
> Adding the -fno-short-enums fixes the
> extra warning generated by the arg >= 0 comparison in pr51712.c
> 
> "warning: comparison is always true due to limited range of data type
> [-Wtype-limits]
>   return arg >= 0 && arg <= BAR;"

If the -fno-short-enums option is needed here, isn't that a bug?

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

--- Comment #14 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-10-16 08:43:28 UTC ---
(In reply to comment #13)
> If the -fno-short-enums option is needed here, isn't that a bug?

Agreed. This is just hiding the bug for the testsuite but not fixing it for
users.

This is one patch fixing this:
http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00383.html

but a better fix is mentioned in the comments: 

http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00972.html

Implementing any of those options will fix this bug and add the infrastructure
needed to fix similar bugs.

[Bug c/51712] -Wtype-limits should not trigger for types of implementation-defined signedness

[manu at gcc dot gnu.org]

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

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikestump at comcast dot
                   |                            |net

--- Comment #15 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-10-29 13:25:22 UTC ---
So the patch was approved and committed, basically hiding that the testcase is
failing because there is a bug. Now, whoever fixes this bug has to also revert
revision 191241.