Bug 33738

Summary: -Wtype-limits misses a warning when comparing enums
Product: gcc Reporter: Diego Novillo <dnovillo>
Component: c++Assignee: Diego Novillo <dnovillo>
Status: RESOLVED FIXED    
Severity: enhancement CC: gcc-bugs, manu, pinskia
Priority: P3 Keywords: diagnostic
Version: 4.3.0   
Target Milestone: 4.4.0   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2008-02-05 11:21:26
Bug Depends on:    
Bug Blocks: 33702    

Description Diego Novillo 2007-10-11 17:50:08 UTC
This was found on GCC 4.2.1.  In this test case, VRP quietly folds a comparison between an enum type and a constant value that the enum type can never take. With -Wtype-limits, this should give the warning:

comparison always false due to limited range of data type

extern void link_error (void);

enum Alpha {
 ZERO = 0, ONE, TWO, THREE
};

Alpha a2;

int m1 = -1;
int GetM1() {
 return m1;
}

int main() {
 a2 = static_cast<Alpha>(GetM1());
 if (a2 == -1) {                   <-- VRP should warn when folding this.
    link_error ();
 }
 return 0;
}

This is not warned by the front end because we promote -1 to the same type as a2.  But during VRP, we *do* fold the predicate, so warning when -Wtype-limits is given in this case would be a good QOI feature.

I have a patch in the works to make VRP warn when it linearizes this predicate.
Comment 1 Andrew Pinski 2007-10-11 18:09:33 UTC
Warnings from optimizers are semi a no-no.  Yes we have them for strict aliasing and overflow but I think those cases are a bit weird.  This is unspecified behavior no matter what and there is no flag to change the behavior here.
Comment 2 Manuel López-Ibáñez 2007-11-13 04:55:56 UTC
(In reply to comment #1)
> Warnings from optimizers are semi a no-no.  Yes we have them for strict
> aliasing and overflow but I think those cases are a bit weird.  This is
> unspecified behavior no matter what and there is no flag to change the behavior
> here.

Well, Wtype-limits is supposed to warn about this and it doesn't. Can we do it from the front-end? If not, then warning from the middle-end seems the right thing to do as long as it is not complex/intrusive.
Comment 3 Diego Novillo 2008-02-05 04:18:42 UTC
Subject: Bug 33738

Author: dnovillo
Date: Tue Feb  5 04:17:58 2008
New Revision: 132111

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132111
Log:

	http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00110.html

	PR 33738
	* tree-vrp.c (vrp_evaluate_conditional): With
	-Wtype-limits, emit a warning when comparing against a
	constant outside the natural range of OP0's type.

testsuite/ChangeLog

	PR 33738
	* testsuite/g++.dg/warn/pr33738.C: New.


Added:
    trunk/gcc/testsuite/g++.dg/warn/pr33738.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c

Comment 4 Diego Novillo 2008-02-05 04:29:38 UTC
Fixed.  http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00110.html.
Comment 5 Manuel López-Ibáñez 2008-02-05 11:21:26 UTC
You should use OPT_Wtype_limits instead of OPT_Wextra.

Also, the code could simply do:

+      tree op0 = TREE_OPERAND (cond, 0);
+      tree op1 = TREE_OPERAND (cond, 1);
+      tree type = TREE_TYPE (op0);
+      value_range_t *vr0 = get_value_range (op0);
+
+      if (vr0->type != VR_VARYING
+	  && INTEGRAL_TYPE_P (type)
+	  && vrp_val_is_min (vr0->min)
+	  && vrp_val_is_max (vr0->max)
+	  && is_gimple_min_invariant (op1))
+	{
+	  location_t locus;
+         const char *warnmsg = NULL;
+
+	  if (!EXPR_HAS_LOCATION (stmt))
+	    locus = input_location;
+	  else
+	    locus = EXPR_LOCATION (stmt);
+
+	  if (integer_zerop (ret))
+	    warnmsg = G_("comparison always false due to limited range of "
+		         "data type");
+	  else
+	    warnmsg = G_("comparison always true due to limited range of "
+			 "data type");
+
+	  warning (OPT_Wextra, "%H%s", &locus, warnmsg);
+	}


And BTW, what is G_ for?
Comment 6 dnovillo@google.com 2008-02-05 16:15:46 UTC
Subject: Re:  -Wtype-limits misses a warning when comparing enums

On 5 Feb 2008 11:21:26 -0000, manu at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:

>  You should use OPT_Wtype_limits instead of OPT_Wextra.

Ah, yes, thanks.

>  Also, the code could simply do:

Well, I explicitly wanted to separate the decision making from the
warning machinery.


>  And BTW, what is G_ for?

That's the i18n marker.  I c-n-p it from other messages in the same file.
Comment 7 Diego Novillo 2008-02-05 16:32:06 UTC
Subject: Bug 33738

Author: dnovillo
Date: Tue Feb  5 16:31:20 2008
New Revision: 132124

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132124
Log:

	http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00140.html

	PR 33738
	* tree-vrp.c (vrp_evaluate_conditional): Revert fix for
	PR 33738.

testsuite/ChangeLog

	PR 33738
	* g++.dg/warn/pr33738.C: Remove.



Removed:
    trunk/gcc/testsuite/g++.dg/warn/pr33738.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c

Comment 8 Volker Reichelt 2008-02-05 19:54:03 UTC
Reopened, since the patch was reverted.
Comment 9 Diego Novillo 2008-02-24 16:41:22 UTC
Subject: Bug 33738

Author: dnovillo
Date: Sun Feb 24 16:40:32 2008
New Revision: 132591

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132591
Log:

	http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01094.html

	PR 33738
	* tree-vrp.c (vrp_evaluate_conditional): With
	-Wtype-limits, emit a warning when comparing against a
	constant outside the natural range of OP0's type.
	* c.opt (Wtype-limits): Move ...
	* common.opt (Wtype-limits): ... here.

testsuite/ChangeLog

	PR 33738
	* g++.dg/warn/pr33738.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/pr33738.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c.opt
    trunk/gcc/common.opt
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c

Comment 10 Diego Novillo 2008-03-11 05:56:01 UTC
Fixed on mainline (4.4).