[Bug c/66773] New: sign-compare warning for == and != are pretty useless

[Bug c/66773] New: sign-compare warning for == and != are pretty useless

[daniel.marjamaki at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773

            Bug ID: 66773
           Summary: sign-compare warning for == and != are pretty useless
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: daniel.marjamaki at gmail dot com
  Target Milestone: ---

I wrote a clang bug report:
https://llvm.org/bugs/show_bug.cgi?id=24036

I recommend that -Wsign-compare is not written for == and != comparisons.

For relational comparisons the sign makes a direct difference, the result of 'a
> b' can be different if you do a sign-cast of an operand.

For equality comparisons the sign does not make a direct difference. the result
of 'a == b' is the same even if you sign-cast an operand.

Code example:

void f(signed int a, unsigned int b) {
  if (a == b) {}
}

gcc writes this warning:

signcompare.c:3:19: warning: comparison between signed and unsigned integer
expressions [-Wsign-compare]

In my humble opinion the risk of a real bug here is really low. a has to be
negative. b has to be really large (unlikely). the bitpatterns of a and b has
to match. if the bitpatterns do match it might actually be the intention that
the test should succeed. but if that is not intentional then there is a bug.

The proper fix for this is to write:

  if (a >= 0 && a == b) {}

However I have seen that this is fixed wrongly by a useless cast. 

This kind of false positive is indirectly a security problem. People routinely
hide these false positives using casts or changed variable types etc. and that
cause bugs and hides other real warnings.

In my humble opinion the risk of a bug here is really low.

The proper fix for this is to write:

  if (a >= 0 && a == b) {}

However I have seen that this is fixed by a useless cast. 

This kind of false positive is indirectly a security problem. People routinely
hide these false positives using casts or changed variable types etc. and that
cause bugs and hides other real warnings.

[Bug c/66773] sign-compare warning for == and != are pretty useless

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773

Segher Boessenkool <segher at gcc dot gnu.org> changed:

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

--- Comment #1 from Segher Boessenkool <segher at gcc dot gnu.org> ---
There certainly are cases where these warnings are inconvenient, but
there also are cases where they are quite useful -- e.g.

int f(void) { return 0xffffffff == -1; }

[Bug c/66773] sign-compare warning for == and != are pretty useless

[daniel.marjamaki at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773

--- Comment #2 from Daniel Marjamäki <daniel.marjamaki at gmail dot com> ---
Thanks!

Hmm.. in my humble opinion, when I see the code:

int f(void) { return 0xffffffff == -1; }

.. I get the impression that the developer probably wants to test if the
bitpattern 0xfff.. matches -1.

I'd say an arbitrary U32 variable will rarelly have such large values unless
it's representing bitpatterns.. indexes, positions, sizes, etc are not that
large. and if you match a bitpattern against a negative value I'd say the match
is probably expected. 

Is it possible to test how much noise this generates? My feeling is that if I
run this on various open source projects I will get lots of pure noise. If I am
right, do you think such noise would be convincing?
>From gcc-bugs-return-491598-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Mon Jul 06 18:24:29 2015
Return-Path: <gcc-bugs-return-491598-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 87591 invoked by alias); 6 Jul 2015 18:24:29 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 87531 invoked by uid 48); 6 Jul 2015 18:24:26 -0000
From: "edlinger at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug target/66747] [6 Regression] The commit r225260 broke the builds of the mips-{mti,img}-linux-gnu tool chains.
Date: Mon, 06 Jul 2015 18:24:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: target
X-Bugzilla-Version: 6.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: edlinger at gcc dot gnu.org
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Resolution: FIXED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: edlinger at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 6.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_status resolution
Message-ID: <bug-66747-4-0gtezgzdK8@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66747-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66747-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-07/txt/msg00488.txt.bz2
Content-length: 430

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf747

Bernd Edlinger <edlinger at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Bernd Edlinger <edlinger at gcc dot gnu.org> ---
fixed.

[Bug c/66773] sign-compare warning for == and != are pretty useless

[daniel.marjamaki at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773

--- Comment #4 from Daniel Marjamäki <daniel.marjamaki at gmail dot com> ---
absolutely. there are often bugs in the boundaries.

well. I was hoping to get more optimistic response.

how about this.. imagine that we wrote a "possible division by zero" warning
for every integer division that uses a non-constant rhs. every warning where
rhs can't really be zero would be a false positive. That would be very noisy
imo.

imho the false positive rate should be similar here. If there is a comparison
'a == -1' and a is unsigned then this warning is useless if a can't be
0xffffffff. if a has arbitrary values then statistically it's as likely that a
is 0xffffffff and 0. So I guess the false positive rate is somewhat similar.

Maybe the message can be tweaked?

comparison between signed and unsigned integer expressions [-Wsign-compare]

I think this message is fine for relational comparisons. A sign-cast is a
reasonable solution.

For == and != I am afraid the message is somewhat misleading. A sign-cast is
not a good solution.
>From gcc-bugs-return-491611-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Tue Jul 07 00:37:47 2015
Return-Path: <gcc-bugs-return-491611-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 94939 invoked by alias); 7 Jul 2015 00:37:47 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 94907 invoked by uid 48); 7 Jul 2015 00:37:44 -0000
From: "dmalcolm at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug jit/66783] New: No error-checking for creating structs containing opaque structs
Date: Tue, 07 Jul 2015 00:37:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: new
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: jit
X-Bugzilla-Version: 6.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: dmalcolm at gcc dot gnu.org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: dmalcolm at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_id short_desc product version bug_status bug_severity priority component assigned_to reporter blocked target_milestone
Message-ID: <bug-66783-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-07/txt/msg00501.txt.bz2
Content-length: 757

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf783

            Bug ID: 66783
           Summary: No error-checking for creating structs containing
                    opaque structs
           Product: gcc
           Version: 6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: jit
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dmalcolm at gcc dot gnu.org
            Blocks: 66627
  Target Milestone: ---

Problem reported on mailing list here:
  https://gcc.gnu.org/ml/jit/2015-q3/msg00022.html

We need to reject this kind of thing at the API boundary.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf627
[Bug 66627] Tracker bug for jit bugs affecting ravi

[Bug c/66773] sign-compare warning for == and != are pretty useless

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773

--- Comment #5 from Andreas Schwab <schwab@linux-m68k.org> ---
A cast is seldom a good solution, but even equality tests have the potential to
go wrong with C's composite type rules.

[Bug c/66773] sign-compare warning for == and != are pretty useless

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773

Eric Gallager <egallager at gcc dot gnu.org> changed:

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

--- Comment #25 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #16)
> (In reply to Eric Gallager from comment #13)
> > > Yes.  You should not use casts, except in some very specific cases, and
> > > most of the uses you see "in the wild" are a bad idea.  Sometimes I
> > > wonder if we should have a -Wcast ("Warn for any cast.").
> > 
> > I get the feeling that such a warning would be extremely noisy and that
> > no one would use it.
> 
> It was not meant as a serious suggestion of course, or I would have done
> this many many years ago.

OK, but I still think some of the ideas I came up with in response to it are
good ideas, though; I'd like to amend this comment #13 in response:

> It would probably be better to go about improving existing cast-related
> warnings, or adding new ones for specific cases, rather than breaking out
> such a broad hammer. For example, the fixits that David Malcolm added for
> -Wold-style-cast are very nice; extending those to apply to more
> cast-related warnings would be a good improvement (I've been meaning to
> open a separate bug about this). These would all be better, more-specific
> warnings to add:
> * -Wcast-to-the-same-type (bug 85043)
> * -Wcast-variadic-function-type (bug 87379)
> * -Wfunctional-cast (bug 69818)
> * -Wcast-enum (bug 30357)
> * -Wold-style-cast-qual (fixit would suggest using const_cast instead)
> * -Wold-style-useless-cast
> * Any of clang's cast-related warnings that we currently don't have yet;
> grep https://clang.llvm.org/docs/DiagnosticsReference.html for the word
> "cast" to find some

To this list, I'd like to add:
* -Wold-style-cast-align (similar to -Wold-style-cast-qual)
* -Wuseless-old-style-cast (as an alternate spelling of
-Wold-style-useless-cast)