C++ PATCH for c++/90473 - wrong code with nullptr in default argument

Message ID 20190809005655.GA28284@redhat.com
State New
Headers show
Series
  • C++ PATCH for c++/90473 - wrong code with nullptr in default argument
Related show

Commit Message

Marek Polacek Aug. 9, 2019, 12:56 a.m.
This is a wrong-code bug where we are throwing away the function call in
the default argument here:

  void fn1 (void* p = (f(), nullptr)) { }

and thus dropping the possible side-effects of that call to the floor.

That happens because check_default_argument returns nullptr_node when it
sees a null_ptr_cst_p argument.  In this case the argument is a COMPOUND_EXPR
"f(), nullptr" and null_ptr_cst_p returns true for it.  And so the effects of
the LHS expression of the compound expr are forgotten.  Fixed as below.

It's tempting to change null_ptr_cst_p to return false when it sees something
that has TREE_SIDE_EFFECTS, but that would be wrong I think: [conv.ptr] says
that a null pointer constant is an integer literal with value zero or a prvalue
of type std::nullptr_t.  An expression "a, b", the built-in comma expression,
is a prvalue if 'b' is an rvalue.  And so "f(), nullptr" is a prvalue of type
std::nullptr_t.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-08-08  Marek Polacek  <polacek@redhat.com>

	PR c++/90473 - wrong code with nullptr in default argument.
	* call.c (null_ptr_cst_p): Update quote from the standard.
	* decl.c (check_default_argument): Don't return nullptr when the arg
	has side-effects.

	* g++.dg/cpp0x/nullptr42.C: New test.

Comments

Jason Merrill Aug. 13, 2019, 2:49 p.m. | #1
On 8/8/19 8:56 PM, Marek Polacek wrote:
> This is a wrong-code bug where we are throwing away the function call in

> the default argument here:

> 

>    void fn1 (void* p = (f(), nullptr)) { }

> 

> and thus dropping the possible side-effects of that call to the floor.

> 

> That happens because check_default_argument returns nullptr_node when it

> sees a null_ptr_cst_p argument.  In this case the argument is a COMPOUND_EXPR

> "f(), nullptr" and null_ptr_cst_p returns true for it.  And so the effects of

> the LHS expression of the compound expr are forgotten.  Fixed as below.

> 

> It's tempting to change null_ptr_cst_p to return false when it sees something

> that has TREE_SIDE_EFFECTS, but that would be wrong I think: [conv.ptr] says

> that a null pointer constant is an integer literal with value zero or a prvalue

> of type std::nullptr_t.  An expression "a, b", the built-in comma expression,

> is a prvalue if 'b' is an rvalue.  And so "f(), nullptr" is a prvalue of type

> std::nullptr_t.

> 

> Bootstrapped/regtested on x86_64-linux, ok for trunk?

> 

> 2019-08-08  Marek Polacek  <polacek@redhat.com>

> 

> 	PR c++/90473 - wrong code with nullptr in default argument.

> 	* call.c (null_ptr_cst_p): Update quote from the standard.

> 	* decl.c (check_default_argument): Don't return nullptr when the arg

> 	has side-effects.


OK.

Jason

Patch

diff --git gcc/cp/call.c gcc/cp/call.c
index 61334a16248..01a25ad6e1e 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -529,9 +529,8 @@  null_ptr_cst_p (tree t)
 
   /* [conv.ptr]
 
-     A null pointer constant is an integral constant expression
-     (_expr.const_) rvalue of integer type that evaluates to zero or
-     an rvalue of type std::nullptr_t. */
+     A null pointer constant is an integer literal ([lex.icon]) with value
+     zero or a prvalue of type std::nullptr_t.  */
   if (NULLPTR_TYPE_P (type))
     return true;
 
diff --git gcc/cp/decl.c gcc/cp/decl.c
index b8806e4628d..d91f25183fb 100644
--- gcc/cp/decl.c
+++ gcc/cp/decl.c
@@ -13177,7 +13177,9 @@  check_default_argument (tree decl, tree arg, tsubst_flags_t complain)
   /* Avoid redundant -Wzero-as-null-pointer-constant warnings at
      the call sites.  */
   if (TYPE_PTR_OR_PTRMEM_P (decl_type)
-      && null_ptr_cst_p (arg))
+      && null_ptr_cst_p (arg)
+      /* Don't lose side-effects as in PR90473.  */
+      && !TREE_SIDE_EFFECTS (arg))
     return nullptr_node;
 
   /* [dcl.fct.default]
diff --git gcc/testsuite/g++.dg/cpp0x/nullptr42.C gcc/testsuite/g++.dg/cpp0x/nullptr42.C
new file mode 100644
index 00000000000..2fb628df6d7
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/nullptr42.C
@@ -0,0 +1,18 @@ 
+// PR c++/90473 - wrong code with nullptr in default argument.
+// { dg-do run { target c++11 } }
+
+int g;
+void f() { g++; }
+
+void fn1 (void* p = (f(), nullptr)) { }
+void fn2 (int p = (f(), 0)) { }
+
+int main()
+{
+  fn1 ();
+  if (g != 1)
+    __builtin_abort ();
+  fn2 ();
+  if (g != 2)
+    __builtin_abort ();
+}