[C++ Patch] PR 63265

[C++ Patch] PR 63265

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

Hi,

as far as I can see this 4.9/5 regression, where we spuriously warn 
about the left shifts in the templates, has to do with r208183, where 
Jason replaced c_inhibit_evaluation_warnings fiddling in 
tsubst_copy_and_build with two warning_sentinels, on warn_type_limits 
and warn_div_by_zero. In the present testcase, the triggering warning is 
the one about left shift count >= width of type, which currently doesn't 
have a name. Thus I think we can easily solve the issue by giving the 
warning a name (likewise for the negative count warning) and using again 
warning_sentinel. The names I picked are the same already used by clang.

Tested x86_64-linux. In case, what about 4.9?

Thanks,
Paolo.

//////////////////////////



[-- Attachment #2: CL_63265 --]
[-- Type: text/plain, Size: 808 bytes --]

2014-11-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/63265
	* c-family/c.opt ([Wshift-count-negative, Wshift-count-overflow]): Add.
	* doc/invoke.texi: Document the latter.

/cp
2014-11-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/63265
	* pt.c (tsubst_copy_and_build, case LSHIFT_EXPR): Use warning_sentinels
	on warn_shift_count_negative and warn_shift_count_overflow.
	* typeck.c (cp_build_binary_op): Use OPT_Wshift_count_negative and
	OPT_Wshift_count_overflow in the warnings.

/c
2014-11-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/63265
	* c-typeck.c (build_binary_op): Use OPT_Wshift_count_negative and
	OPT_Wshift_count_overflow in the warnings.

/testsuite
2014-11-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/63265
	* g++.dg/cpp0x/constexpr-63265.C: New.

[-- Attachment #3: patch_63265 --]
[-- Type: text/plain, Size: 5387 bytes --]

Index: c/c-typeck.c
===================================================================
--- c/c-typeck.c	(revision 217282)
+++ c/c-typeck.c	(working copy)
@@ -10545,7 +10545,8 @@ build_binary_op (location_t location, enum tree_co
 		{
 		  int_const = false;
 		  if (c_inhibit_evaluation_warnings == 0)
-		    warning_at (location, 0, "left shift count is negative");
+		    warning_at (location, OPT_Wshift_count_negative,
+				"left shift count is negative");
 		}
 
 	      else if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0)
@@ -10552,8 +10553,8 @@ build_binary_op (location_t location, enum tree_co
 		{
 		  int_const = false;
 		  if (c_inhibit_evaluation_warnings == 0)
-		    warning_at (location, 0, "left shift count >= width of "
-				"type");
+		    warning_at (location, OPT_Wshift_count_overflow,
+				"left shift count >= width of type");
 		}
 	    }
 
Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 217282)
+++ c-family/c.opt	(working copy)
@@ -760,14 +760,22 @@ Wselector
 ObjC ObjC++ Var(warn_selector) Warning
 Warn if a selector has multiple methods
 
+Wsequence-point
+C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about possible violations of sequence point rules
+
 Wshadow-ivar
 ObjC ObjC++ Var(warn_shadow_ivar) EnabledBy(Wshadow) Init(1) Warning
 Warn if a local declaration hides an instance variable
 
-Wsequence-point
-C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
-Warn about possible violations of sequence point rules
+Wshift-count-negative
+C ObjC C++ ObjC++ Var(warn_shift_count_negative) Init(1) Warning
+Warn if left shift count is negative
 
+Wshift-count-overflow
+C ObjC C++ ObjC++ Var(warn_shift_count_overflow) Init(1) Warning
+Warn if left shift count >= width of type
+
 Wsign-compare
 C ObjC C++ ObjC++ Var(warn_sign_compare) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about signed-unsigned comparisons
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 217282)
+++ cp/pt.c	(working copy)
@@ -14680,6 +14680,8 @@ tsubst_copy_and_build (tree t,
       {
 	warning_sentinel s1(warn_type_limits);
 	warning_sentinel s2(warn_div_by_zero);
+	warning_sentinel s3(warn_shift_count_negative);
+	warning_sentinel s4(warn_shift_count_overflow);
 	tree op0 = RECUR (TREE_OPERAND (t, 0));
 	tree op1 = RECUR (TREE_OPERAND (t, 1));
 	tree r = build_x_binary_op
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 217282)
+++ cp/typeck.c	(working copy)
@@ -4328,7 +4328,8 @@ cp_build_binary_op (location_t location,
 		{
 		  if ((complain & tf_warning)
 		      && c_inhibit_evaluation_warnings == 0)
-		    warning (0, "left shift count is negative");
+		    warning (OPT_Wshift_count_negative,
+			     "left shift count is negative");
 		}
 	      else if (compare_tree_int (const_op1,
 					 TYPE_PRECISION (type0)) >= 0)
@@ -4335,7 +4336,8 @@ cp_build_binary_op (location_t location,
 		{
 		  if ((complain & tf_warning)
 		      && c_inhibit_evaluation_warnings == 0)
-		    warning (0, "left shift count >= width of type");
+		    warning (OPT_Wshift_count_overflow,
+			     "left shift count >= width of type");
 		}
 	    }
 	  /* Convert the shift-count to an integer, regardless of
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 217282)
+++ doc/invoke.texi	(working copy)
@@ -269,6 +269,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wpointer-arith  -Wno-pointer-to-int-cast @gol
 -Wredundant-decls  -Wno-return-local-addr @gol
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
+-Wshift-count-negative -Wshift-count-overflow @gol
 -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
 -Wsizeof-pointer-memaccess  -Wsizeof-array-argument @gol
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
@@ -3825,6 +3826,16 @@ exceptions are @samp{main} and functions defined i
 
 This warning is enabled by @option{-Wall}.
 
+@item -Wshift-count-negative
+@opindex Wshift-count-negative
+@opindex Wno-shift-count-negative
+Warn if left shift count is negative. This warning is enabled by default.
+
+@item -Wshift-count-overflow
+@opindex Wshift-count-overflow
+@opindex Wno-shift-count-overflow
+Warn if left shift count >= width of type. This warning is enabled by default.
+
 @item -Wswitch
 @opindex Wswitch
 @opindex Wno-switch
Index: testsuite/g++.dg/cpp0x/constexpr-63265.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-63265.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/constexpr-63265.C	(working copy)
@@ -0,0 +1,19 @@
+// PR c++/63265
+// { dg-do compile { target c++11 } }
+
+#define LSHIFT (sizeof(unsigned int) * __CHAR_BIT__)
+
+template <int lshift>
+struct SpuriouslyWarns1 {
+    static constexpr unsigned int v = lshift < LSHIFT ? 1U << lshift : 0;
+};
+
+static_assert(SpuriouslyWarns1<LSHIFT>::v == 0, "Impossible occurred");
+
+template <int lshift>
+struct SpuriouslyWarns2 {
+    static constexpr bool okay = lshift < LSHIFT;
+    static constexpr unsigned int v = okay ? 1U << lshift : 0;
+};
+
+static_assert(SpuriouslyWarns2<LSHIFT>::v == 0, "Impossible occurred");

Re: [C++ Patch] PR 63265

[Jakub Jelinek]

On Mon, Nov 10, 2014 at 05:32:49PM +0100, Paolo Carlini wrote:
> 	PR c++/63265
> 	* c-family/c.opt ([Wshift-count-negative, Wshift-count-overflow]): Add.

Note, c-family/ has its own ChangeLog.

	Jakub

Re: [C++ Patch] PR 63265

[Jason Merrill]

I don't think we want to suppress this warning in general.  The problem 
in this PR is that the warning code is failing to recognize that the 
first operand is constant false.

Jason

Re: [C++ Patch] PR 63265

[Jason Merrill]

On 11/10/2014 12:16 PM, Jason Merrill wrote:
> I don't think we want to suppress this warning in general.  The problem
> in this PR is that the warning code is failing to recognize that the
> first operand is constant false.

But adding the warning options is OK.

Jason

Re: [C++ Patch] PR 63265

[Paolo Carlini]

Hi,

On 11/10/2014 06:16 PM, Jason Merrill wrote:
> On 11/10/2014 12:16 PM, Jason Merrill wrote:
>> I don't think we want to suppress this warning in general.  The problem
>> in this PR is that the warning code is failing to recognize that the
>> first operand is constant false.
>
> But adding the warning options is OK.
Thanks, but I just noticed that the patch is incomplete about that, 
doesn't handle right shifts. Let's say that for time being I'm going to 
resubmit a complete patch for the warnings and I unassign myself from 
the regression proper.

Paolo.

Re: [C++ Patch] PR 63265

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

Hi,

On 11/10/2014 06:16 PM, Jason Merrill wrote:
> I don't think we want to suppress this warning in general.  The 
> problem in this PR is that the warning code is failing to recognize 
> that the first operand is constant false.
Thanks. Then, shall we do something like the below? Passes testing.

Thanks,
Paolo.

//////////////////

[-- Attachment #2: CL_63265_2 --]
[-- Type: text/plain, Size: 279 bytes --]

/cp
2014-11-11  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/63265
	* pt.c (tsubst_copy_and_build, case COND_EXPR): Maybe fold to
	const the condition.

/testsuite
2014-11-11  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/63265
	* g++.dg/cpp0x/constexpr-63265.C: New.

[-- Attachment #3: patch_63265_2 --]
[-- Type: text/plain, Size: 1264 bytes --]

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 217342)
+++ cp/pt.c	(working copy)
@@ -15137,7 +15137,9 @@ tsubst_copy_and_build (tree t,
 
     case COND_EXPR:
       {
-	tree cond = RECUR (TREE_OPERAND (t, 0));
+	tree cond
+	  = maybe_constant_value (fold_non_dependent_expr_sfinae
+				  (RECUR (TREE_OPERAND (t, 0)), tf_none));
 	tree exp1, exp2;
 
 	if (TREE_CODE (cond) == INTEGER_CST)
Index: testsuite/g++.dg/cpp0x/constexpr-63265.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-63265.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/constexpr-63265.C	(working copy)
@@ -0,0 +1,19 @@
+// PR c++/63265
+// { dg-do compile { target c++11 } }
+
+#define LSHIFT (sizeof(unsigned int) * __CHAR_BIT__)
+
+template <int lshift>
+struct SpuriouslyWarns1 {
+    static constexpr unsigned int v = lshift < LSHIFT ? 1U << lshift : 0;
+};
+
+static_assert(SpuriouslyWarns1<LSHIFT>::v == 0, "Impossible occurred");
+
+template <int lshift>
+struct SpuriouslyWarns2 {
+    static constexpr bool okay = lshift < LSHIFT;
+    static constexpr unsigned int v = okay ? 1U << lshift : 0;
+};
+
+static_assert(SpuriouslyWarns2<LSHIFT>::v == 0, "Impossible occurred");

Re: [C++ Patch] PR 63265

[Jason Merrill]

On 11/11/2014 08:04 AM, Paolo Carlini wrote:
> -	tree cond = RECUR (TREE_OPERAND (t, 0));
> +	tree cond
> +	  = maybe_constant_value (fold_non_dependent_expr_sfinae
> +				  (RECUR (TREE_OPERAND (t, 0)), tf_none));

I like this approach, but if the result of maybe_constant_value doesn't 
turn out to be an INTEGER_CST, we want to end up with the result of 
RECUR rather than the result of fold_non_dependent_expr, as the latter 
might not be suitable for subsequent tsubsting.

Jason

Re: [C++ Patch] PR 63265

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

Hi,

On 11/11/2014 02:19 PM, Jason Merrill wrote:
> On 11/11/2014 08:04 AM, Paolo Carlini wrote:
>> -    tree cond = RECUR (TREE_OPERAND (t, 0));
>> +    tree cond
>> +      = maybe_constant_value (fold_non_dependent_expr_sfinae
>> +                  (RECUR (TREE_OPERAND (t, 0)), tf_none));
>
> I like this approach, but if the result of maybe_constant_value 
> doesn't turn out to be an INTEGER_CST, we want to end up with the 
> result of RECUR rather than the result of fold_non_dependent_expr, as 
> the latter might not be suitable for subsequent tsubsting.
I see. Something like the below, then?

Thanks,
Paolo.

/////////////////

[-- Attachment #2: patch_63265_3 --]
[-- Type: text/plain, Size: 1640 bytes --]

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 217342)
+++ cp/pt.c	(working copy)
@@ -15138,11 +15138,13 @@ tsubst_copy_and_build (tree t,
     case COND_EXPR:
       {
 	tree cond = RECUR (TREE_OPERAND (t, 0));
+	tree folded_cond = (maybe_constant_value
+			    (fold_non_dependent_expr_sfinae (cond, tf_none)));
 	tree exp1, exp2;
 
-	if (TREE_CODE (cond) == INTEGER_CST)
+	if (TREE_CODE (folded_cond) == INTEGER_CST)
 	  {
-	    if (integer_zerop (cond))
+	    if (integer_zerop (folded_cond))
 	      {
 		++c_inhibit_evaluation_warnings;
 		exp1 = RECUR (TREE_OPERAND (t, 1));
@@ -15156,6 +15158,7 @@ tsubst_copy_and_build (tree t,
 		exp2 = RECUR (TREE_OPERAND (t, 2));
 		--c_inhibit_evaluation_warnings;
 	      }
+	    cond = folded_cond;
 	  }
 	else
 	  {
Index: testsuite/g++.dg/cpp0x/constexpr-63265.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-63265.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/constexpr-63265.C	(working copy)
@@ -0,0 +1,19 @@
+// PR c++/63265
+// { dg-do compile { target c++11 } }
+
+#define LSHIFT (sizeof(unsigned int) * __CHAR_BIT__)
+
+template <int lshift>
+struct SpuriouslyWarns1 {
+    static constexpr unsigned int v = lshift < LSHIFT ? 1U << lshift : 0;
+};
+
+static_assert(SpuriouslyWarns1<LSHIFT>::v == 0, "Impossible occurred");
+
+template <int lshift>
+struct SpuriouslyWarns2 {
+    static constexpr bool okay = lshift < LSHIFT;
+    static constexpr unsigned int v = okay ? 1U << lshift : 0;
+};
+
+static_assert(SpuriouslyWarns2<LSHIFT>::v == 0, "Impossible occurred");

Re: [C++ Patch] PR 63265

[Jason Merrill]

OK.

Jason

Re: [C++ Patch] PR 63265

[Paolo Carlini]

Hi,

On 11/11/2014 04:03 PM, Jason Merrill wrote:
> OK.
Committed. What about 4.9? Technically the issue is a regression, but in 
my opinion the fix isn't completely trivial for a release branch..

Paolo.

Re: [C++ Patch] PR 63265

[Jason Merrill]

On 11/11/2014 12:37 PM, Paolo Carlini wrote:
> Committed. What about 4.9? Technically the issue is a regression, but in
> my opinion the fix isn't completely trivial for a release branch..

I think it's safe enough for a regression fix.

Jason