[C++ Patch] PR 60955

[C++ Patch] PR 60955

[Paolo Carlini]

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

Hi,

this regression, a spurious warning about taking the address of a 
register parameter, happens in C++14 mode due to the use of 
force_paren_expr, called by finish_parenthesized_expr, which ends up 
calling build_static_cast. Manuel mentioned in the audit trail that 
TREE_NO_WARNING can be used for DECLs too, and indeed I noticed today 
that we are *already* using it for EXPRs, at the beginning of 
finish_parenthesized_expr. I experimented a bit with restricting the 
setting, eg, to PARM_DECLs only, but in fact we have an identical issue 
for, eg, register VAR_DECLs. Tested x86_64-linux.

Thanks,
Paolo.

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

[-- Attachment #2: CL_60955 --]
[-- Type: text/plain, Size: 335 bytes --]

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

	PR c++/60955
	* semantics.c (finish_parenthesized_expr): Set TREE_NO_WARNING on
	non-EXPRs too.
	* typeck.c (cxx_mark_addressable): Check TREE_NO_WARNING.

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

	PR c++/60955
	* g++.dg/warn/register-parm-1.C: New.

[-- Attachment #3: patch_60955 --]
[-- Type: text/plain, Size: 1725 bytes --]

Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 218586)
+++ cp/semantics.c	(working copy)
@@ -1674,10 +1674,13 @@ force_paren_expr (tree expr)
 tree
 finish_parenthesized_expr (tree expr)
 {
-  if (EXPR_P (expr))
-    /* This inhibits warnings in c_common_truthvalue_conversion.  */
-    TREE_NO_WARNING (expr) = 1;
+  if (expr == error_mark_node)
+    return error_mark_node;
 
+  /* Inhibit warnings in c_common_truthvalue_conversion and in
+     cxx_mark_addressable.  */
+  TREE_NO_WARNING (expr) = 1;
+
   if (TREE_CODE (expr) == OFFSET_REF
       || TREE_CODE (expr) == SCOPE_REF)
     /* [expr.unary.op]/3 The qualified id of a pointer-to-member must not be
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 218586)
+++ cp/typeck.c	(working copy)
@@ -6068,7 +6068,8 @@ cxx_mark_addressable (tree exp)
 		  ("address of explicit register variable %qD requested", x);
 		return false;
 	      }
-	    else if (extra_warnings)
+	    else if (extra_warnings
+		     && !TREE_NO_WARNING (x))
 	      warning
 		(OPT_Wextra, "address requested for %qD, which is declared %<register%>", x);
 	  }
Index: testsuite/g++.dg/warn/register-parm-1.C
===================================================================
--- testsuite/g++.dg/warn/register-parm-1.C	(revision 0)
+++ testsuite/g++.dg/warn/register-parm-1.C	(working copy)
@@ -0,0 +1,9 @@
+// PR c++/60955
+// { dg-options "-Wextra" }
+
+unsigned int erroneous_warning(register int a) {
+    if ((a) & 0xff) return 1; else return 0;
+}
+unsigned int no_erroneous_warning(register int a) {
+    if (a & 0xff) return 1; else return 0;
+}

[Ping][C++ Patch] PR 60955

[Paolo Carlini]

Hi,

gently pinging this....

     https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00961.html

On 12/10/2014 08:24 PM, Paolo Carlini wrote:
> Hi,
>
> this regression, a spurious warning about taking the address of a 
> register parameter, happens in C++14 mode due to the use of 
> force_paren_expr, called by finish_parenthesized_expr, which ends up 
> calling build_static_cast. Manuel mentioned in the audit trail that 
> TREE_NO_WARNING can be used for DECLs too, and indeed I noticed today 
> that we are *already* using it for EXPRs, at the beginning of 
> finish_parenthesized_expr. I experimented a bit with restricting the 
> setting, eg, to PARM_DECLs only, but in fact we have an identical 
> issue for, eg, register VAR_DECLs. Tested x86_64-linux.
Thanks,
Paolo.

Re: [C++ Patch] PR 60955

[Jason Merrill]

I'm uncomfortable with setting TREE_NO_WARNING on a decl just because we 
don't want a warning for one particular use of it.  How about 
suppressing warnings across the call to build_static_cast?

Jason

Re: [C++ Patch] PR 60955

[Paolo Carlini]

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

Hi,

On 12/17/2014 09:37 PM, Jason Merrill wrote:
> I'm uncomfortable with setting TREE_NO_WARNING on a decl just because 
> we don't want a warning for one particular use of it.  How about 
> suppressing warnings across the call to build_static_cast?
Sure. The below uses the c_inhibit_evaluation_warnings mechanism and 
passes testing. I wondered if in such cases we could alternately use the 
warning_sentinel mechanism (moved to cp-tree.h of course) ?

Thanks,
Paolo.

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

[-- Attachment #2: patch_60955_2 --]
[-- Type: text/plain, Size: 1555 bytes --]

Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 218857)
+++ cp/semantics.c	(working copy)
@@ -1660,7 +1660,9 @@ force_paren_expr (tree expr)
 	  tree type = unlowered_expr_type (expr);
 	  bool rval = !!(kind & clk_rvalueref);
 	  type = cp_build_reference_type (type, rval);
+	  ++c_inhibit_evaluation_warnings;
 	  expr = build_static_cast (type, expr, tf_error);
+	  --c_inhibit_evaluation_warnings;
 	  if (expr != error_mark_node)
 	    REF_PARENTHESIZED_P (expr) = true;
 	}
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 218857)
+++ cp/typeck.c	(working copy)
@@ -6047,7 +6047,8 @@ cxx_mark_addressable (tree exp)
 		  ("address of explicit register variable %qD requested", x);
 		return false;
 	      }
-	    else if (extra_warnings)
+	    else if (extra_warnings
+		     && c_inhibit_evaluation_warnings == 0)
 	      warning
 		(OPT_Wextra, "address requested for %qD, which is declared %<register%>", x);
 	  }
Index: testsuite/g++.dg/warn/register-parm-1.C
===================================================================
--- testsuite/g++.dg/warn/register-parm-1.C	(revision 0)
+++ testsuite/g++.dg/warn/register-parm-1.C	(working copy)
@@ -0,0 +1,9 @@
+// PR c++/60955
+// { dg-options "-Wextra" }
+
+unsigned int erroneous_warning(register int a) {
+    if ((a) & 0xff) return 1; else return 0;
+}
+unsigned int no_erroneous_warning(register int a) {
+    if (a & 0xff) return 1; else return 0;
+}

Re: [C++ Patch] PR 60955

[Jason Merrill]

On 12/18/2014 06:17 AM, Paolo Carlini wrote:
> Sure. The below uses the c_inhibit_evaluation_warnings mechanism and
> passes testing. I wondered if in such cases we could alternately use the
> warning_sentinel mechanism (moved to cp-tree.h of course) ?

That would make sense to me.

Jason

Re: [C++ Patch] PR 60955

[Paolo Carlini]

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

Hi,

On 12/18/2014 03:20 PM, Jason Merrill wrote:
> On 12/18/2014 06:17 AM, Paolo Carlini wrote:
>> Sure. The below uses the c_inhibit_evaluation_warnings mechanism and
>> passes testing. I wondered if in such cases we could alternately use the
>> warning_sentinel mechanism (moved to cp-tree.h of course) ?
> That would make sense to me.
Good. Then I'm finishing testing the below.

Thanks,
Paolo.

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

[-- Attachment #2: patch_60955_3 --]
[-- Type: text/plain, Size: 2299 bytes --]

Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 218857)
+++ cp/cp-tree.h	(working copy)
@@ -1149,6 +1149,18 @@ struct processing_template_decl_sentinel
   }
 };
 
+/* RAII sentinel to disable certain warnings during template substitution
+   and elsewhere.  */
+
+struct warning_sentinel
+{
+  int &flag;
+  int val;
+  warning_sentinel(int& flag, bool suppress=true)
+    : flag(flag), val(flag) { if (suppress) flag = 0; }
+  ~warning_sentinel() { flag = val; }
+};
+
 /* The cached class binding level, from the most recently exited
    class, or NULL if none.  */
 
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 218857)
+++ cp/pt.c	(working copy)
@@ -14438,16 +14438,6 @@ tsubst_non_call_postfix_expression (tree t, tree a
   return t;
 }
 
-/* Sentinel to disable certain warnings during template substitution.  */
-
-struct warning_sentinel {
-  int &flag;
-  int val;
-  warning_sentinel(int& flag, bool suppress=true)
-    : flag(flag), val(flag) { if (suppress) flag = 0; }
-  ~warning_sentinel() { flag = val; }
-};
-
 /* Like tsubst but deals with expressions and performs semantic
    analysis.  FUNCTION_P is true if T is the "F" in "F (ARGS)".  */
 
Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 218857)
+++ cp/semantics.c	(working copy)
@@ -1660,6 +1660,7 @@ force_paren_expr (tree expr)
 	  tree type = unlowered_expr_type (expr);
 	  bool rval = !!(kind & clk_rvalueref);
 	  type = cp_build_reference_type (type, rval);
+	  warning_sentinel s (extra_warnings);
 	  expr = build_static_cast (type, expr, tf_error);
 	  if (expr != error_mark_node)
 	    REF_PARENTHESIZED_P (expr) = true;
Index: testsuite/g++.dg/warn/register-parm-1.C
===================================================================
--- testsuite/g++.dg/warn/register-parm-1.C	(revision 0)
+++ testsuite/g++.dg/warn/register-parm-1.C	(working copy)
@@ -0,0 +1,9 @@
+// PR c++/60955
+// { dg-options "-Wextra" }
+
+unsigned int erroneous_warning(register int a) {
+    if ((a) & 0xff) return 1; else return 0;
+}
+unsigned int no_erroneous_warning(register int a) {
+    if (a & 0xff) return 1; else return 0;
+}

Re: [C++ Patch] PR 60955

[Jason Merrill]

On 12/18/2014 11:31 AM, Paolo Carlini wrote:
> +	  warning_sentinel s (extra_warnings);

Let's add a comment about which warning we're avoiding here.  OK with 
that change.

Jason

Re: [C++ Patch] PR 60955

[Paolo Carlini]

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

Hi,

On 12/18/2014 06:21 PM, Jason Merrill wrote:
> On 12/18/2014 11:31 AM, Paolo Carlini wrote:
>> +      warning_sentinel s (extra_warnings);
>
> Let's add a comment about which warning we're avoiding here.  OK with 
> that change.
Thanks. I'm attaching what I just committed. This is a regression, I 
suppose the patch is ok for 4_9-branch too, right?

Thanks,
Paolo.

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

[-- Attachment #2: CL_60955_3 --]
[-- Type: text/plain, Size: 306 bytes --]

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

	PR c++/60955
	* pt.c (struct warning_sentinel): Move it...
	* cp-tree.h: ... here.
	* semantics.c (force_paren_expr): Use it.

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

	PR c++/60955
	* g++.dg/warn/register-parm-1.C: New.

[-- Attachment #3: patch_60955_3b --]
[-- Type: text/plain, Size: 2382 bytes --]

Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 218870)
+++ cp/cp-tree.h	(working copy)
@@ -1149,6 +1149,18 @@ struct processing_template_decl_sentinel
   }
 };
 
+/* RAII sentinel to disable certain warnings during template substitution
+   and elsewhere.  */
+
+struct warning_sentinel
+{
+  int &flag;
+  int val;
+  warning_sentinel(int& flag, bool suppress=true)
+    : flag(flag), val(flag) { if (suppress) flag = 0; }
+  ~warning_sentinel() { flag = val; }
+};
+
 /* The cached class binding level, from the most recently exited
    class, or NULL if none.  */
 
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 218870)
+++ cp/pt.c	(working copy)
@@ -14438,16 +14438,6 @@ tsubst_non_call_postfix_expression (tree t, tree a
   return t;
 }
 
-/* Sentinel to disable certain warnings during template substitution.  */
-
-struct warning_sentinel {
-  int &flag;
-  int val;
-  warning_sentinel(int& flag, bool suppress=true)
-    : flag(flag), val(flag) { if (suppress) flag = 0; }
-  ~warning_sentinel() { flag = val; }
-};
-
 /* Like tsubst but deals with expressions and performs semantic
    analysis.  FUNCTION_P is true if T is the "F" in "F (ARGS)".  */
 
Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 218870)
+++ cp/semantics.c	(working copy)
@@ -1660,6 +1660,9 @@ force_paren_expr (tree expr)
 	  tree type = unlowered_expr_type (expr);
 	  bool rval = !!(kind & clk_rvalueref);
 	  type = cp_build_reference_type (type, rval);
+	  /* This inhibits warnings in, eg, cxx_mark_addressable
+	     (c++/60955).  */
+	  warning_sentinel s (extra_warnings);
 	  expr = build_static_cast (type, expr, tf_error);
 	  if (expr != error_mark_node)
 	    REF_PARENTHESIZED_P (expr) = true;
Index: testsuite/g++.dg/warn/register-parm-1.C
===================================================================
--- testsuite/g++.dg/warn/register-parm-1.C	(revision 0)
+++ testsuite/g++.dg/warn/register-parm-1.C	(working copy)
@@ -0,0 +1,9 @@
+// PR c++/60955
+// { dg-options "-Wextra" }
+
+unsigned int erroneous_warning(register int a) {
+    if ((a) & 0xff) return 1; else return 0;
+}
+unsigned int no_erroneous_warning(register int a) {
+    if (a & 0xff) return 1; else return 0;
+}

Re: [C++ Patch] PR 60955

[Jason Merrill]

On 12/18/2014 12:55 PM, Paolo Carlini wrote:
> Thanks. I'm attaching what I just committed. This is a regression, I
> suppose the patch is ok for 4_9-branch too, right?

Yes.

Jason