Bug 12963

Summary: Wrong and misleading warning encourages writing non-portable code
Product: gcc Reporter: bagnara
Component: cAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: enhancement CC: eggert, gcc-bugs, giovannibajo, manu, qrczak, sthoenna, vincent-gcc
Priority: P2 Keywords: diagnostic
Version: 3.4.0   
Target Milestone: 4.3.0   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2005-12-28 06:05:58
Attachments: Shows how GCC (3.4.2) is inconsistent concerning the mentioned warning

Description bagnara 2003-11-08 13:42:20 UTC
For the C++ function

unsigned char
foo(unsigned char c) {
  if (c <= 255)
    return 255;
  else
    return c;
}

g++ 3.4 20031102 (and also 3.3.2, but NOT 3.3.1) generates,
without any -W option, the following warning:

g++ -c bug.cc
bug.cc: In function `unsigned char foo(unsigned char)':
bug.cc:3: warning: comparison is always true due to limited range of data type

I believe this warning should not be issued, since unsigned chars may
be more than 8 bits wide.  The fact that in the particular implementation
of ISO C++ I am using right now unsigned chars are 8 bits wide should be
irrelevant (I certainly do not want to write unportable code to avoid that warning).
Comment 1 Andrew Pinski 2003-11-08 21:02:37 UTC
The warning is correct for this implementation and since you are compiling with this 
implementation, you can recieve warnings (yes it would be better if it was enabled by a -
W* but I think it is good warning no matter what).  To disable the warning use -w (notice 
the lower case w).
Comment 2 bagnara 2003-11-08 22:44:49 UTC
(In reply to comment #1)
> The warning is correct for this implementation and since you are compiling
with this 
> implementation, you can recieve warnings (yes it would be better if it was
enabled by a -
> W* but I think it is good warning no matter what).  To disable the warning use
-w (notice 
> the lower case w).

If I understand correctly, -w inhibits all warning messages, which is
certainly not what I call for.  The fact that, when producing code for
platforms where CHAR_BIT == 8, the test is redundant is a matter for
the optimizer (who can omit the test), not something the user should
be warned about.  In general, I believe target-dependent warnings
should default to disabled.  By the way, are there other target-dependent
warnings in GCC?  I mean that this is the first time I meet one (unless
I am missing something).

Now try with

void*
foo(void* c) {
  if (c <= (void*) 0xFFFFFFFF)
    return 0;
  else
    return c;
}

and you will see that (notice -W -Wall)

g++ -W -Wall -c t.cc -O3 -fomit-frame-pointer -S

does not produce any warning.  Yet, the produced code is,

.globl _Z3fooPv
	.type	_Z3fooPv, @function
_Z3fooPv:
.LFB3:
	xorl	%eax, %eax
	ret

as expected.
Would you advocate a warning also in this case?
Comment 3 Andrew Pinski 2003-11-15 08:55:23 UTC
There are a large number of target-dependent (and sizeof dependent) warnings in GCC. GCC also 
warns about "integer overflow in expression" and fp.
Comment 4 Andrew Pinski 2003-11-21 15:58:31 UTC
Since GCC is warning based on the target you are using, I am closing as will not fix.
Comment 5 bagnara 2003-11-21 21:13:48 UTC
(In reply to comment #4)
> Since GCC is warning based on the target you are using, I am closing as will
not fix.

In my opinion, this is a mistake: people wishing to write 100% portable code
should not be forced to switch all warnings off in order to get rid
of this useless warning (yes, useless, because there is nothing sensible the
sensible programmer can do about it).
Warnings such as this one should be issued only when specifically requested.
At the very least, it should be possible to selectively switch off this
kind of warnings.
But closing the bug report as you are doing (without even addressing my
question "why then there is no such a warning for void*, not even with
-W -Wall?") will not make the problem go away.  The problem is there.
I don't think this anxiety to close bug reports before the issue has
been studied and discussed in depth is going to help.
Comment 6 Wolfgang Bangerth 2003-11-21 21:52:04 UTC
I agree with Robert on this point: we shouldn't get this warning
_when not giving any flag_. IMHO, the warning is quite valid and tells
the programmer about something that he might not have considered, but
in case he has it is pointless to issue a warning that is issued
even without any of the -W... flags and without the possibility to
selectively switch it off.

Let's keep this open until we reach some kind of consensus whether it
is possible to implement this or not.

W.
Comment 7 Gabriel Dos Reis 2003-11-21 23:15:59 UTC
Subject: Re:  Wrong and misleading warning encourages writing non-portable code

"bangerth at dealii dot org" <gcc-bugzilla@gcc.gnu.org> writes:

| Let's keep this open until we reach some kind of consensus whether it
| is possible to implement this or not.

I would suggest "enhancement request".

-- Gaby
Comment 8 Joseph S. Myers 2003-11-21 23:52:43 UTC
Subject: Re:  Wrong and misleading warning encourages writing
 non-portable code

On Fri, 21 Nov 2003, bangerth at dealii dot org wrote:

> I agree with Robert on this point: we shouldn't get this warning
> _when not giving any flag_. IMHO, the warning is quite valid and tells

As I previously pointed out
<http://gcc.gnu.org/ml/gcc-patches/2003-03/msg01954.html>.  Given this
sort of portability issue, a separate option (maybe in -Wextra, but
probably not in -Wall), such as -Wcomparison-fixed, is appropriate for
these warnings and those already in -Wextra I mentioned in that message.

Comment 9 bagnara 2004-09-20 13:12:13 UTC
To my claim that the warning "comparison is always false due to limited range of
data type" should be disabled by default, I would like to add the fact that GCC
is highly inconsistent in this respect.  In fact, as the attached file v.cc
shows, the warning is given only for char and short whereas it is not given for
int, long and long long.
Comment 10 bagnara 2004-09-20 13:16:37 UTC
Created attachment 7177 [details]
Shows how GCC (3.4.2) is inconsistent concerning the mentioned warning

Lines 56 and 57 cause the instantiations with char and short: warning given.
Lines 58, 59 and 60, for which no warning is given, cause the instantiations
with int, long and long long.
Here is what happens with GCC 3.4.2:

$ g++ -c v.cc
v.cc: In function `int assign_c(To&, From) [with To = char, From = char]':
v.cc:56:   instantiated from here
v.cc:39: warning: comparison is always false due to limited range of data type
v.cc:41: warning: comparison is always false due to limited range of data type
v.cc: In function `int assign_c(To&, From) [with To = short int, From = short
int]':
v.cc:57:   instantiated from here
v.cc:39: warning: comparison is always false due to limited range of data type
v.cc:41: warning: comparison is always false due to limited range of data type
Comment 11 Giovanni Bajo 2004-09-20 16:29:03 UTC
Besides the consistency issue, which is a bug and should be reported elsewhere, 
I do not understand how this encourage writing non-portable code. On the 
contrary, it does *help* it.

Say that you have a program which assumes that char is 16bits, and you port to 
an architecture which has chars with 8bits. Instead of silently failing, you 
will get warnings like this one which would help you catching problems.

I do not think you provided a good rationale of why we should disable such a 
warning. Could you please describe your real-life problem about portability and 
this warning?
Comment 12 Joseph S. Myers 2004-09-20 23:08:28 UTC
Subject: Re:  Wrong and misleading warning encourages writing
 non-portable code

On Mon, 20 Sep 2004, giovannibajo at libero dot it wrote:

> Besides the consistency issue, which is a bug and should be reported elsewhere, 
> I do not understand how this encourage writing non-portable code. On the 
> contrary, it does *help* it.

A program has some data in a type such as char or uid_t which needs to be 
stored in an externally defined structure, or transmitted by an externally 
defined protocol.  This external definition imposes a limit of (say 255) 
on the value held in (say) unsigned char.  A portable program checks that 
the data is within the defined range before storing or transmitting it.  
Some checks are redundant on a particular system, so receive these 
warnings; if the warnings are to be silenced, that means removing checks 
that are not dead code on other systems.  I have encountered such problems 
(warnings that cannot effectively be silenced) in real code, both with 
system typedefs such as uid_t and user typedefs that may be configured to 
be a type such as unsigned char or a wider type.  My comment #8 still 
applies: this does not even belong in -Wall.

Comment 13 Abramo Bagnara 2004-09-21 07:24:39 UTC
(In reply to comment #1)
> The warning is correct for this implementation and since you are compiling
with this 
> implementation, you can recieve warnings (yes it would be better if it was
enabled by a -
> W* but I think it is good warning no matter what).  To disable the warning use
-w (notice 
> the lower case w).

I hope that there is a general consensus about the fact that the presence of
expected warnings not avoidable on compilation log is a bad thing.

This lower the attention threshold of programmers and contributors about true
problems and reduce the readability of compilation log. I suppose that the
availability of -Werror is derived from this same reasoning.

Taken for granted that, I think that the presence of warning options like
-wWARNING-NAME to disable specific warnings could solve the problem in a per
file fashion.

Better than that the availability of something like
#pragma expected-warning line WARNING-NAME
might remove the warning generated by the following line labeling it as checked,
expected and/or unavoidable.
Comment 14 Joseph S. Myers 2004-09-22 22:24:15 UTC
Subject: Re:  Wrong and misleading warning encourages writing
 non-portable code

On Tue, 21 Sep 2004, abramobagnara at tin dot it wrote:

> Better than that the availability of something like
> #pragma expected-warning line WARNING-NAME
> might remove the warning generated by the following line labeling it as checked,
> expected and/or unavoidable.

That this is desired is generally agreed.  It's just that no-one has yet 
produced a design for it and still had time left to implement it after all 
the arguing over that design.

DJ produced a design <http://gcc.gnu.org/ml/gcc/2004-06/msg01188.html> 
(and thread).  This led to a mechanical patch adding a parameter 0 to 
every call to warning() 
<http://gcc.gnu.org/ml/gcc-patches/2004-07/msg02229.html>; it wasn't 
applied, nor was there any followup.  Previously, Stan Shebs gave a 
proposal <http://gcc.gnu.org/ml/gcc/2003-01/msg01065.html> (and thread).  
There have been various other such discussions.  
<http://gcc.gnu.org/ml/gcc/2000-06/msg00639.html> has a different 
proposal.  <http://gcc.gnu.org/ml/gcc/1998-09/msg00353.html> included, I 
think uniquely, a patch, but there were objections and it was reverted 
within 24 hours.

Implementing such a feature is not hard.  I could probably do a plausible 
implementation - that did not require all calls to diagnostic functions to 
be converted at once, rather allowing the conversion to having individual 
warnings controllable to be a gradual one - in a few days (having 
enable-mapped-location unconditionally on would help though), as could 
many other people.  Producing a consensus that any given approach is 
right, or even that any given approach is not unacceptable, is the hard 
part.

Comment 15 Andrew Pinski 2005-03-20 17:50:31 UTC
*** Bug 20550 has been marked as a duplicate of this bug. ***
Comment 16 Marcin 'Qrczak' Kowalczyk 2005-03-20 19:10:41 UTC
> Better than that the availability of something like
> #pragma expected-warning line WARNING-NAME
> might remove the warning generated by the following line labeling it as checked,
> expected and/or unavoidable.

This would not help in my case because it's a regular type-generic C macro, not
generated code. The line number of the macro definition is not stable (changes
as surrounding code evolves), and it makes no sense to mark all its invocations.

From my point of view a perfect solution would be making this obvious workaround
working:

int test(int x) {
   if ((long long)x <= 0x123456789ABCLL) return 1;
   else return 0;
}

There should be no warning if the lhs is cast to the wider type. GCC seems to be
inferring that the range of possible values of (long long)x is the range of int.
The runtime comparison is eliminated, which is good; this implies that the
warning should not be tied to the comparison being eliminated.

I'm not judging whether the warning should be emitted at all. Maybe it should be
removed altogether; after all, it's not guaranteed to be redundant on all
platforms. It definitely should not be emitted by default, especially without
any warning options, especially if the cast suggests that the programmer is
aware of it.
Comment 17 Andrew Pinski 2005-03-21 00:24:34 UTC
*** Bug 20573 has been marked as a duplicate of this bug. ***
Comment 18 Andrew Pinski 2005-06-24 23:10:23 UTC
*** Bug 22178 has been marked as a duplicate of this bug. ***
Comment 19 bagnara 2006-01-25 11:39:29 UTC
Just a small update.  On one of our projects we have now thousands of warnings on the test "x < 0" for the function below, when Type is instantiated to an unsigned integral type:

template <typename Type>
inline Result
sgn_generic(const Type& x) {
  if (x < 0)
    return V_LT;
  if (x > 0)
    return V_GT;
  return V_EQ;
}

The net result is that some of us started using "-w" or stopped paying attention to warnings, and, as a consequence bugs are creeping in at a much increased rate.  Please, give us a way to at least turn off that warning.
Comment 20 Paolo Carlini 2006-01-25 12:20:58 UTC
(In reply to comment #19)
> Just a small update.  On one of our projects we have now thousands of warnings
> on the test "x < 0" for the function below, when Type is instantiated to an
> unsigned integral type:
> 
> template <typename Type>
> inline Result
> sgn_generic(const Type& x) {
>   if (x < 0)
>     return V_LT;
>   if (x > 0)
>     return V_GT;
>   return V_EQ;
> }

A side remark, about a weird workaround I had to use in the past in the library (yes the problem is real, I agree, not sure about the best solution, however). Would boil down to something like (mosulo stupid typos ;)

  if (x > 0)
    return V_GT;
  else if (x) 
    return V_LT;
  return V_EQ;

Seems correct to me and doesn't incur in any warning.
Comment 21 Manuel López-Ibáñez 2007-02-06 13:48:14 UTC
Patch: http://gcc.gnu.org/ml/gcc-patches/2007-01/msg01933.html
Comment 22 Manuel López-Ibáñez 2007-05-20 21:31:57 UTC
Subject: Bug 12963

Author: manu
Date: Sun May 20 20:29:55 2007
New Revision: 124875

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124875
Log:
2007-05-20  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>
	
	PR middle-end/7651
	PR c++/11856
	PR c/12963
	PR c/23587
	PR other/29694
	* c.opt (Wtype-limits): New.
	* doc/invoke.texi (Wtype-limits): Document it.
	(Wextra): Enabled by -Wextra.
	* c-opts.c (c_common_post_options): Enabled by -Wextra.
	* c-common.c (shorten_compare): Warn with Wtype-limits.

testsuite/
	* gcc.dg/compare6.c: Replace Wall with Wtype-limits.
	* gcc.dg/Wtype-limits.c: New.
	* gcc.dg/Wtype-limits-Wextra.c: New.
	* gcc.dg/Wtype-limits-no.c: New.
	* g++.dg/warn/Wtype-limits.C: New.
	* g++.dg/warn/Wtype-limits-Wextra.C: New.
	* g++.dg/warn/Wtype-limits-no.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wtype-limits-Wextra.C
    trunk/gcc/testsuite/g++.dg/warn/Wtype-limits-no.C
    trunk/gcc/testsuite/g++.dg/warn/Wtype-limits.C
    trunk/gcc/testsuite/gcc.dg/Wtype-limits-Wextra.c
    trunk/gcc/testsuite/gcc.dg/Wtype-limits-no.c
    trunk/gcc/testsuite/gcc.dg/Wtype-limits.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-common.c
    trunk/gcc/c-opts.c
    trunk/gcc/c.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/compare6.c

Comment 23 Manuel López-Ibáñez 2007-05-20 21:50:57 UTC
The diverse warnings of the type "always true/false because of range of data type" have been grouped under -Wtype-limits that is enabled by -Wextra (and not by -Wall). The warning can be disabled by using -Wno-type-limits.

Thus, fixed in GCC 4.3.