Re: [PATCH v2] c++: side effect in nullptr_t conversion fix

[Dmitry Drozodv <da.drozdov85@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7096 bytes --]

[PATCH v3] c++: side effect in nullptr_t conversion fix

Hi,

> This seems to assume that a CONVERT_EXPR can't have any other
> side-effects nested in it.
>
> It seems to me a better approach is the one in keep_unused_object_arg
> and cp_gimplify_expr, basically
>
> if (TREE_THIS_VOLATILE (e))
>    e = cp_build_addr_expr (e);
>
> since the address of a volatile lvalue is safe to put on the lhs of a
> COMPOUND_EXPR.

Thank you for your review! I have implemented what you suggested with a
little modification:
I cut the conversion nodes using STRIP_NOPS (expr), since the value is not
actually converted anywhere and should be ignored.

> I don't see why ocp_convert needs to change at all; if TREE_SIDE_EFFECTS
> is set we'll eventually get to cp_convert_to_pointer and can do the
> right thing there.

Without changes in ocp_convert, it does not happen to call the
cp_convert_to_pointer function.
Due to the equality of the 'to' and 'from' types, identity conversion
occurs with preserving the side effect.

> Why not just
>
> DECL_INITIAL (dest) = nullptr_node;
>
> ?

Because it doesn't work.
This does not completely replace initialization; reading from volatile
nullptr remains.

I also added a test suite 'nullptr47.C' to check the original tree.

---
 gcc/cp/cvt.cc                          | 20 ++++++++++++++---
 gcc/cp/typeck2.cc                      |  8 +++++++
 gcc/testsuite/g++.dg/cpp0x/nullptr47.C | 30 ++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/nullptr47.C

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index cbed847b343..578c9b8ef20 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -218,8 +218,20 @@ cp_convert_to_pointer (tree type, tree expr, bool
dofold,
   ? build_int_cst_type (type, -1)
   : build_int_cst (type, 0));

-      return (TREE_SIDE_EFFECTS (expr)
-      ? build2 (COMPOUND_EXPR, type, expr, val) : val);
+      tree e = STRIP_NOPS (expr);
+      if (TREE_THIS_VOLATILE (e))
+ {
+  /* can't drop expr with side effect (e.g. function call). */
+  e = cp_build_addr_expr(e, tf_warning_or_error);
+  return build2 (COMPOUND_EXPR, type, e, val);
+ }
+
+      /* C++ [conv.lval]p3:
+ If T is cv std::nullptr_t, the result is a null pointer constant. */
+      /* expr value must be ignored. */
+      return ((TREE_SIDE_EFFECTS(e))
+       ? build2(COMPOUND_EXPR, type, e, val)
+               : val);
     }
   else if (TYPE_PTRMEM_P (type) && INTEGRAL_CODE_P (form))
     {
@@ -743,9 +755,11 @@ ocp_convert (tree type, tree expr, int convtype, int
flags,
     {
       if (complain & tf_warning)
  maybe_warn_zero_as_null_pointer_constant (e, loc);
-
       if (!TREE_SIDE_EFFECTS (e))
  return nullptr_node;
+      else
+ /* process nullptr to nullptr conversion with side effect. */
+ return cp_convert_to_pointer(type, e, dofold, complain);
     }

   if (MAYBE_CLASS_TYPE_P (type) && (convtype & CONV_FORCE_TEMP))
diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index ac0fefa24f2..cdabb32b289 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -770,6 +770,14 @@ split_nonconstant_init (tree dest, tree init)
    && array_of_runtime_bound_p (TREE_TYPE (dest)))
     code = build_vec_init (dest, NULL_TREE, init, /*value-init*/false,
    /*from array*/1, tf_warning_or_error);
+  else if (TREE_CODE (TREE_TYPE(init)) == NULLPTR_TYPE)
+    {
+      /* C++ [conv.lval]p3:
+ If T is cv std::nullptr_t, the result is a null pointer constant. */
+      tree ie = cp_build_init_expr(dest, nullptr_node);
+      // remain expr with side effect
+      code = add_stmt_to_compound(init, ie);
+    }
   else
     code = cp_build_init_expr (dest, init);

diff --git a/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
b/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
new file mode 100644
index 00000000000..f05e91465f7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
@@ -0,0 +1,30 @@
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2 -fdump-tree-original" }
+
+int* foo()
+{
+    volatile auto a_0 = nullptr;
+    int* b_0 = a_0;
+    return b_0;
+}
+/* nullptr to int pointer conversion: no load from a_0. Init b_0 by zero.
*/
+// { dg-final { scan-tree-dump-times "cleanup_point   int \\* b_0 = 0" 1
"original"} }
+
+auto __attribute__ ((noinline)) bar()
+{
+   /* side effect. Elimination of 'call bar()' will be incorrect. */
+   volatile int* b = (int *)0xff;
+   *b = 10;
+
+   volatile auto n = nullptr;
+   return n;
+}
+/* nullptr to nullptr conversion (identity): no load from n. Just return
zero. */
+// { dg-final { scan-tree-dump-times "return <retval> =\[^\\n\\r\]* 0;" 1
"original"} }
+
+void foo_2()
+{
+  volatile auto a_2 = bar();
+}
+/* non-constant init: no store bar() result to a_2. Call bar() and init
a_2 by zero. */
+// { dg-final { scan-tree-dump-times "bar \\(\\);, a_2 = 0;" 1 "original"}
}
-- 
2.34.1

On Wed, Jan 17, 2024 at 12:10 AM Jason Merrill <jason@redhat.com> wrote:

> On 1/11/24 15:34, Dmitry Drozodv wrote:
> > You are absolutely right, we can't throw all side-effects away.
> >
> > +      /* C++ [conv.lval]p3:
> > +        If T is cv std::nullptr_t, the result is a null pointer
> constant. */
> > +      return ((TREE_SIDE_EFFECTS (expr) && !CONVERT_EXPR_P (expr))
> > +              ? build2 (COMPOUND_EXPR, type, expr, val)
> > +              : val);
>
> This seems to assume that a CONVERT_EXPR can't have any other
> side-effects nested in it.
>
> It seems to me a better approach is the one in keep_unused_object_arg
> and cp_gimplify_expr, basically
>
> if (TREE_THIS_VOLATILE (e))
>    e = cp_build_addr_expr (e);
>
> since the address of a volatile lvalue is safe to put on the lhs of a
> COMPOUND_EXPR.
>
> > -      if (!TREE_SIDE_EFFECTS (e))
> > +      /* C++ [conv.lval]p3:
> > +        Convert expr from nullptr to nullptr doesn't have side effect.
> */
> > +      if (!TREE_SIDE_EFFECTS (e) || CONVERT_EXPR_P (expr))
>
> I don't see why ocp_convert needs to change at all; if TREE_SIDE_EFFECTS
> is set we'll eventually get to cp_convert_to_pointer and can do the
> right thing there.
>
> > @@ -770,6 +770,15 @@ split_nonconstant_init (tree dest, tree init)
> >             && array_of_runtime_bound_p (TREE_TYPE (dest)))
> >       code = build_vec_init (dest, NULL_TREE, init, /*value-init*/false,
> >                             /*from array*/1, tf_warning_or_error);
> > +  else if (TREE_CODE (TREE_TYPE(init)) == NULLPTR_TYPE)
> > +    {
> > +      /* C++ [conv.lval]p3:
> > +        If T is cv std::nullptr_t, the result is a null pointer
> > constant. */
> > +      tree val = build_int_cst(TREE_TYPE(dest), 0);
> > +      tree ie = cp_build_init_expr(dest, val);
>
> Why not just
>
> DECL_INITIAL (dest) = nullptr_node;
>
> ?
>
> Also, your mail client is mangling your patch so it doesn't apply
> properly.  Please see https://gcc.gnu.org/contribute.html for more
> information.
>
> Jason
>
>

[-- Attachment #2: 0001-PATCH-v3-c-side-effect-in-nullptr_t-conversion-fix.patch --]
[-- Type: application/octet-stream, Size: 8620 bytes --]

From c0e4db96cba0b46a06ecd1f81ed1ef747d9f78ba Mon Sep 17 00:00:00 2001
From: Dmitrryy <da.drozdov@mail.ru>
Date: Sun, 7 Jan 2024 15:45:40 +0300
Subject: [PATCH] [PATCH v3] c++: side effect in nullptr_t conversion fix

Hi,

> This seems to assume that a CONVERT_EXPR can't have any other
> side-effects nested in it.
>
> It seems to me a better approach is the one in keep_unused_object_arg
> and cp_gimplify_expr, basically
>
> if (TREE_THIS_VOLATILE (e))
>    e = cp_build_addr_expr (e);
>
> since the address of a volatile lvalue is safe to put on the lhs of a
> COMPOUND_EXPR.

Thank you for your review! I implemented what you suggested with a little modification:
I cut the conversion nodes using STRIP_NOPS (expr), since the value is not actually converted anywhere and should be ignored.

> I don't see why ocp_convert needs to change at all; if TREE_SIDE_EFFECTS
> is set we'll eventually get to cp_convert_to_pointer and can do the
> right thing there.

Without changes, it does not happen to call the cp_convert_to_pointer function.
Due to the equality of the 'to' and 'from' types, identity conversion occurs with preserving the side effect.

> Why not just
>
> DECL_INITIAL (dest) = nullptr_node;
>
> ?

Because it doesn't work.
This does not completely replace initialization, reading from volatile nullptr remains.

I also added a test suite 'nullptr47.C' to check the original tree.

---
 gcc/cp/cvt.cc                          | 20 ++++++++++++++---
 gcc/cp/typeck2.cc                      |  8 +++++++
 gcc/testsuite/g++.dg/cpp0x/nullptr47.C | 30 ++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/nullptr47.C

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index cbed847b343..578c9b8ef20 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -218,8 +218,20 @@ cp_convert_to_pointer (tree type, tree expr, bool dofold,
 		  ? build_int_cst_type (type, -1)
 		  : build_int_cst (type, 0));

-      return (TREE_SIDE_EFFECTS (expr)
-	      ? build2 (COMPOUND_EXPR, type, expr, val) : val);
+      tree e = STRIP_NOPS (expr);
+      if (TREE_THIS_VOLATILE (e))
+	{
+	  /* can't drop expr with side effect (e.g. function call). */
+	  e = cp_build_addr_expr(e, tf_warning_or_error);
+	  return build2 (COMPOUND_EXPR, type, e, val);
+	}
+
+      /* C++ [conv.lval]p3:
+	 If T is cv std::nullptr_t, the result is a null pointer constant. */
+      /* expr value must be ignored. */
+      return ((TREE_SIDE_EFFECTS(e))
+	       ? build2(COMPOUND_EXPR, type, e, val)
+               : val);
     }
   else if (TYPE_PTRMEM_P (type) && INTEGRAL_CODE_P (form))
     {
@@ -743,9 +755,11 @@ ocp_convert (tree type, tree expr, int convtype, int flags,
     {
       if (complain & tf_warning)
 	maybe_warn_zero_as_null_pointer_constant (e, loc);
-
       if (!TREE_SIDE_EFFECTS (e))
 	return nullptr_node;
+      else
+	/* process nullptr to nullptr conversion with side effect. */
+	return cp_convert_to_pointer(type, e, dofold, complain);
     }

   if (MAYBE_CLASS_TYPE_P (type) && (convtype & CONV_FORCE_TEMP))
diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index ac0fefa24f2..cdabb32b289 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -770,6 +770,14 @@ split_nonconstant_init (tree dest, tree init)
 	   && array_of_runtime_bound_p (TREE_TYPE (dest)))
     code = build_vec_init (dest, NULL_TREE, init, /*value-init*/false,
 			   /*from array*/1, tf_warning_or_error);
+  else if (TREE_CODE (TREE_TYPE(init)) == NULLPTR_TYPE)
+    {
+      /* C++ [conv.lval]p3:
+	 If T is cv std::nullptr_t, the result is a null pointer constant. */
+      tree ie = cp_build_init_expr(dest, nullptr_node);
+      // remain expr with side effect
+      code = add_stmt_to_compound(init, ie);
+    }
   else
     code = cp_build_init_expr (dest, init);

diff --git a/gcc/testsuite/g++.dg/cpp0x/nullptr47.C b/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
new file mode 100644
index 00000000000..f05e91465f7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
@@ -0,0 +1,30 @@
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2 -fdump-tree-original" }
+
+int* foo()
+{
+    volatile auto a_0 = nullptr;
+    int* b_0 = a_0;
+    return b_0;
+}
+/* nullptr to int pointer conversion: no load from a_0. Init b_0 by zero. */
+// { dg-final { scan-tree-dump-times "cleanup_point   int \\* b_0 = 0" 1 "original"} }
+
+auto __attribute__ ((noinline)) bar()
+{
+   /* side effect. Elimination of 'call bar()' will be incorrect. */
+   volatile int* b = (int *)0xff;
+   *b = 10;
+
+   volatile auto n = nullptr;
+   return n;
+}
+/* nullptr to nullptr conversion (identity): no load from n. Just return zero. */
+// { dg-final { scan-tree-dump-times "return <retval> =\[^\\n\\r\]* 0;" 1 "original"} }
+
+void foo_2()
+{
+  volatile auto a_2 = bar();
+}
+/* non-constant init: no store bar() result to a_2. Call bar() and init a_2 by zero. */
+// { dg-final { scan-tree-dump-times "bar \\(\\);, a_2 = 0;" 1 "original"} }
--
2.34.1
---
 gcc/cp/cvt.cc                          | 20 ++++++++++++++---
 gcc/cp/typeck2.cc                      |  8 +++++++
 gcc/testsuite/g++.dg/cpp0x/nullptr47.C | 30 ++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/nullptr47.C

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index cbed847b343..578c9b8ef20 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -218,8 +218,20 @@ cp_convert_to_pointer (tree type, tree expr, bool dofold,
 		  ? build_int_cst_type (type, -1)
 		  : build_int_cst (type, 0));
 
-      return (TREE_SIDE_EFFECTS (expr)
-	      ? build2 (COMPOUND_EXPR, type, expr, val) : val);
+      tree e = STRIP_NOPS (expr);
+      if (TREE_THIS_VOLATILE (e)) 
+	{
+	  /* can't drop expr with side effect (e.g. function call). */
+	  e = cp_build_addr_expr(e, tf_warning_or_error);
+	  return build2 (COMPOUND_EXPR, type, e, val);
+	}
+
+      /* C++ [conv.lval]p3:
+	 If T is cv std::nullptr_t, the result is a null pointer constant. */
+      /* expr value must be ignored. */
+      return ((TREE_SIDE_EFFECTS(e))
+	       ? build2(COMPOUND_EXPR, type, e, val)
+               : val);
     }
   else if (TYPE_PTRMEM_P (type) && INTEGRAL_CODE_P (form))
     {
@@ -743,9 +755,11 @@ ocp_convert (tree type, tree expr, int convtype, int flags,
     {
       if (complain & tf_warning)
 	maybe_warn_zero_as_null_pointer_constant (e, loc);
-
       if (!TREE_SIDE_EFFECTS (e))
 	return nullptr_node;
+      else
+	/* process nullptr to nullptr conversion with side effect. */
+	return cp_convert_to_pointer(type, e, dofold, complain);
     }
 
   if (MAYBE_CLASS_TYPE_P (type) && (convtype & CONV_FORCE_TEMP))
diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index ac0fefa24f2..cdabb32b289 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -770,6 +770,14 @@ split_nonconstant_init (tree dest, tree init)
 	   && array_of_runtime_bound_p (TREE_TYPE (dest)))
     code = build_vec_init (dest, NULL_TREE, init, /*value-init*/false,
 			   /*from array*/1, tf_warning_or_error);
+  else if (TREE_CODE (TREE_TYPE(init)) == NULLPTR_TYPE) 
+    {
+      /* C++ [conv.lval]p3:
+	 If T is cv std::nullptr_t, the result is a null pointer constant. */
+      tree ie = cp_build_init_expr(dest, nullptr_node);
+      // remain expr with side effect
+      code = add_stmt_to_compound(init, ie);
+    }
   else
     code = cp_build_init_expr (dest, init);
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/nullptr47.C b/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
new file mode 100644
index 00000000000..f05e91465f7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/nullptr47.C
@@ -0,0 +1,30 @@
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2 -fdump-tree-original" }
+
+int* foo()
+{
+    volatile auto a_0 = nullptr;
+    int* b_0 = a_0;
+    return b_0;
+}
+/* nullptr to int pointer conversion: no load from a_0. Init b_0 by zero. */
+// { dg-final { scan-tree-dump-times "cleanup_point   int \\* b_0 = 0" 1 "original"} }
+
+auto __attribute__ ((noinline)) bar()
+{
+   /* side effect. Elimination of 'call bar()' will be incorrect. */
+   volatile int* b = (int *)0xff;
+   *b = 10;
+
+   volatile auto n = nullptr;
+   return n;
+}
+/* nullptr to nullptr conversion (identity): no load from n. Just return zero. */
+// { dg-final { scan-tree-dump-times "return <retval> =\[^\\n\\r\]* 0;" 1 "original"} }
+
+void foo_2()
+{
+  volatile auto a_2 = bar();
+}
+/* non-constant init: no store bar() result to a_2. Call bar() and init a_2 by zero. */
+// { dg-final { scan-tree-dump-times "bar \\(\\);, a_2 = 0;" 1 "original"} }
-- 
2.34.1