diff mbox series

gimplify: Emit clobbers for TARGET_EXPR_SLOT vars later [PR103984]

Message ID YjIhXj9RZf1Fn2oO@tucnak
State New
Headers show
Series gimplify: Emit clobbers for TARGET_EXPR_SLOT vars later [PR103984] | expand

Commit Message

Jakub Jelinek March 16, 2022, 5:41 p.m. UTC 
Hi!

As mentioned in the PR, we emit a bogus uninitialized warning but
easily could emit wrong-code for it or similar testcases too.
The bug is that we emit clobber for a TARGET_EXPR_SLOT too early:
          D.2499.e = B::qux (&h); [return slot optimization]
          D.2516 = 1;
          try
            {
              B::B (&D.2498, &h);
              try
                {
                  _2 = baz (&D.2498);
                  D.2499.f = _2;
                  D.2516 = 0;
                  try
                    {
                      try
                        {
                          bar (&D.2499);
                        }
                      finally
                        {
                          C::~C (&D.2499);
                        }
                    }
                  finally
                    {
                      D.2499 = {CLOBBER(eol)};
                    }
                }
              finally
                {
                  D.2498 = {CLOBBER(eol)};
                }
            }
          catch
            {
              if (D.2516 != 0) goto <D.2517>; else goto <D.2518>;
              <D.2517>:
              A::~A (&D.2499.e);
              goto <D.2519>;
              <D.2518>:
              <D.2519>:
            }
The CLOBBER for D.2499 is essentially only emitted on the non-exceptional
path, if B::B or baz throws, then there is no CLOBBER for it but there
is a conditional destructor A::~A (&D.2499.e).  Now, ehcleanup1
sink_clobbers optimization assumes that clobbers in the EH cases are
emitted after last use and so sinks the D.2499 = {CLOBBER(eol)}; later,
so we then have
  # _3 = PHI <1(3), 0(9)>
<L2>:
  D.2499 ={v} {CLOBBER(eol)};
  D.2498 ={v} {CLOBBER(eol)};
  if (_3 != 0)
    goto <bb 11>; [INV]
  else
    goto <bb 15>; [INV]

  <bb 11> :
  _35 = D.2499.a;
  if (&D.2499.b != _35)
where that _35 = D.2499.a comes from inline expansion of the A::~A dtor,
and that is a load from a clobbered memory.

Now, what the gimplifier sees in this case is a CLEANUP_POINT_EXPR with
somewhere inside of it a TARGET_EXPR for D.2499 (with the C::~C (&D.2499)
cleanup) which in its TARGET_EXPR_INITIAL has another TARGET_EXPR for
D.2516 bool flag which has CLEANUP_EH_ONLY which performs that conditional
A::~A (&D.2499.e) call.
The following patch ensures that CLOBBERs (and asan poisoning) are emitted
after even those gimple_push_cleanup pushed cleanups from within the
TARGET_EXPR_INITIAL gimplification (i.e. the last point where the slot could
be in theory used).  In my first version of the patch I've done it by just
moving the
      /* Add a clobber for the temporary going out of scope, like
         gimplify_bind_expr.  */
      if (gimplify_ctxp->in_cleanup_point_expr
          && needs_to_live_in_memory (temp))
        {
...
        }
block earlier in gimplify_target_expr, but that regressed a couple of tests
where temp is marked TREE_ADDRESSABLE only during (well, very early during
that) the gimplification of TARGET_EXPR_INITIAL, so we didn't emit e.g. on
pr80032.C or stack2.C tests any clobbers for the slots and thus stack slot
reuse wasn't performed.
So that we don't regress those tests, this patch gimplifies
TARGET_EXPR_INITIAL as before, but doesn't emit it directly into pre_p,
emits it into a temporary sequence.  Then emits the CLOBBER cleanup
into pre_p, then asan poisoning if needed, then appends the
TARGET_EXPR_INITIAL temporary sequence and finally adds TARGET_EXPR_CLEANUP
gimple_push_cleanup.  The earlier a GIMPLE_WCE appears in the sequence, the
outer try/finally or try/catch it is.
So, with this patch the part of the testcase in gimple dump cited above
looks instead like:
          try
            {
              D.2499.e = B::qux (&h); [return slot optimization]
              D.2516 = 1;
              try
                {
                  try
                    {
                      B::B (&D.2498, &h);
                      _2 = baz (&D.2498);
                      D.2499.f = _2;
                      D.2516 = 0;
                      try
                        {
                          bar (&D.2499);
                        }
                      finally
                        {
                          C::~C (&D.2499);
                        }
                    }
                  finally
                    {
                      D.2498 = {CLOBBER(eol)};
                    }
                }
              catch
                {
                  if (D.2516 != 0) goto <D.2517>; else goto <D.2518>;
                  <D.2517>:
                  A::~A (&D.2499.e);
                  goto <D.2519>;
                  <D.2518>:
                  <D.2519>:
                }
            }
          finally
            {
              D.2499 = {CLOBBER(eol)};
            }

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

2022-03-16  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/103984
	* gimplify.cc (gimplify_target_expr): Gimplify type sizes and
	TARGET_EXPR_INITIAL into a temporary sequence, then push clobbers
	and asan unpoisioning, then append the temporary sequence and
	finally the TARGET_EXPR_CLEANUP clobbers.

	* g++.dg/opt/pr103984.C: New test.


	Jakub

Comments

Richard Biener March 17, 2022, 7:31 a.m. UTC | #1 
On Wed, 16 Mar 2022, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, we emit a bogus uninitialized warning but
> easily could emit wrong-code for it or similar testcases too.
> The bug is that we emit clobber for a TARGET_EXPR_SLOT too early:
>           D.2499.e = B::qux (&h); [return slot optimization]
>           D.2516 = 1;
>           try
>             {
>               B::B (&D.2498, &h);
>               try
>                 {
>                   _2 = baz (&D.2498);
>                   D.2499.f = _2;
>                   D.2516 = 0;
>                   try
>                     {
>                       try
>                         {
>                           bar (&D.2499);
>                         }
>                       finally
>                         {
>                           C::~C (&D.2499);
>                         }
>                     }
>                   finally
>                     {
>                       D.2499 = {CLOBBER(eol)};
>                     }
>                 }
>               finally
>                 {
>                   D.2498 = {CLOBBER(eol)};
>                 }
>             }
>           catch
>             {
>               if (D.2516 != 0) goto <D.2517>; else goto <D.2518>;
>               <D.2517>:
>               A::~A (&D.2499.e);
>               goto <D.2519>;
>               <D.2518>:
>               <D.2519>:
>             }
> The CLOBBER for D.2499 is essentially only emitted on the non-exceptional
> path, if B::B or baz throws, then there is no CLOBBER for it but there
> is a conditional destructor A::~A (&D.2499.e).  Now, ehcleanup1
> sink_clobbers optimization assumes that clobbers in the EH cases are
> emitted after last use and so sinks the D.2499 = {CLOBBER(eol)}; later,
> so we then have
>   # _3 = PHI <1(3), 0(9)>
> <L2>:
>   D.2499 ={v} {CLOBBER(eol)};
>   D.2498 ={v} {CLOBBER(eol)};
>   if (_3 != 0)
>     goto <bb 11>; [INV]
>   else
>     goto <bb 15>; [INV]
> 
>   <bb 11> :
>   _35 = D.2499.a;
>   if (&D.2499.b != _35)
> where that _35 = D.2499.a comes from inline expansion of the A::~A dtor,
> and that is a load from a clobbered memory.
> 
> Now, what the gimplifier sees in this case is a CLEANUP_POINT_EXPR with
> somewhere inside of it a TARGET_EXPR for D.2499 (with the C::~C (&D.2499)
> cleanup) which in its TARGET_EXPR_INITIAL has another TARGET_EXPR for
> D.2516 bool flag which has CLEANUP_EH_ONLY which performs that conditional
> A::~A (&D.2499.e) call.
> The following patch ensures that CLOBBERs (and asan poisoning) are emitted
> after even those gimple_push_cleanup pushed cleanups from within the
> TARGET_EXPR_INITIAL gimplification (i.e. the last point where the slot could
> be in theory used).  In my first version of the patch I've done it by just
> moving the
>       /* Add a clobber for the temporary going out of scope, like
>          gimplify_bind_expr.  */
>       if (gimplify_ctxp->in_cleanup_point_expr
>           && needs_to_live_in_memory (temp))
>         {
> ...
>         }
> block earlier in gimplify_target_expr, but that regressed a couple of tests
> where temp is marked TREE_ADDRESSABLE only during (well, very early during
> that) the gimplification of TARGET_EXPR_INITIAL, so we didn't emit e.g. on
> pr80032.C or stack2.C tests any clobbers for the slots and thus stack slot
> reuse wasn't performed.
> So that we don't regress those tests, this patch gimplifies
> TARGET_EXPR_INITIAL as before, but doesn't emit it directly into pre_p,
> emits it into a temporary sequence.  Then emits the CLOBBER cleanup
> into pre_p, then asan poisoning if needed, then appends the
> TARGET_EXPR_INITIAL temporary sequence and finally adds TARGET_EXPR_CLEANUP
> gimple_push_cleanup.  The earlier a GIMPLE_WCE appears in the sequence, the
> outer try/finally or try/catch it is.
> So, with this patch the part of the testcase in gimple dump cited above
> looks instead like:
>           try
>             {
>               D.2499.e = B::qux (&h); [return slot optimization]
>               D.2516 = 1;
>               try
>                 {
>                   try
>                     {
>                       B::B (&D.2498, &h);
>                       _2 = baz (&D.2498);
>                       D.2499.f = _2;
>                       D.2516 = 0;
>                       try
>                         {
>                           bar (&D.2499);
>                         }
>                       finally
>                         {
>                           C::~C (&D.2499);
>                         }
>                     }
>                   finally
>                     {
>                       D.2498 = {CLOBBER(eol)};
>                     }
>                 }
>               catch
>                 {
>                   if (D.2516 != 0) goto <D.2517>; else goto <D.2518>;
>                   <D.2517>:
>                   A::~A (&D.2499.e);
>                   goto <D.2519>;
>                   <D.2518>:
>                   <D.2519>:
>                 }
>             }
>           finally
>             {
>               D.2499 = {CLOBBER(eol)};
>             }
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

LGTM.

Thanks,
Richard.

> 2022-03-16  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/103984
> 	* gimplify.cc (gimplify_target_expr): Gimplify type sizes and
> 	TARGET_EXPR_INITIAL into a temporary sequence, then push clobbers
> 	and asan unpoisioning, then append the temporary sequence and
> 	finally the TARGET_EXPR_CLEANUP clobbers.
> 
> 	* g++.dg/opt/pr103984.C: New test.
> 
> --- gcc/gimplify.cc.jj	2022-03-16 10:55:58.426014897 +0100
> +++ gcc/gimplify.cc	2022-03-16 15:41:06.122913465 +0100
> @@ -6997,17 +6997,17 @@ gimplify_target_expr (tree *expr_p, gimp
>  
>    if (init)
>      {
> -      tree cleanup = NULL_TREE;
> +      gimple_seq init_pre_p = NULL;
>  
>        /* TARGET_EXPR temps aren't part of the enclosing block, so add it
>  	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
>        if (!poly_int_tree_p (DECL_SIZE (temp)))
>  	{
>  	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
> -	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> +	    gimplify_type_sizes (TREE_TYPE (temp), &init_pre_p);
>  	  /* FIXME: this is correct only when the size of the type does
>  	     not depend on expressions evaluated in init.  */
> -	  gimplify_vla_decl (temp, pre_p);
> +	  gimplify_vla_decl (temp, &init_pre_p);
>  	}
>        else
>  	{
> @@ -7022,12 +7022,14 @@ gimplify_target_expr (tree *expr_p, gimp
>        /* If TARGET_EXPR_INITIAL is void, then the mere evaluation of the
>  	 expression is supposed to initialize the slot.  */
>        if (VOID_TYPE_P (TREE_TYPE (init)))
> -	ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none);
> +	ret = gimplify_expr (&init, &init_pre_p, post_p, is_gimple_stmt,
> +			     fb_none);
>        else
>  	{
>  	  tree init_expr = build2 (INIT_EXPR, void_type_node, temp, init);
>  	  init = init_expr;
> -	  ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none);
> +	  ret = gimplify_expr (&init, &init_pre_p, post_p, is_gimple_stmt,
> +			       fb_none);
>  	  init = NULL;
>  	  ggc_free (init_expr);
>  	}
> @@ -7037,18 +7039,9 @@ gimplify_target_expr (tree *expr_p, gimp
>  	  TARGET_EXPR_INITIAL (targ) = NULL_TREE;
>  	  return GS_ERROR;
>  	}
> -      if (init)
> -	gimplify_and_add (init, pre_p);
>  
> -      /* If needed, push the cleanup for the temp.  */
> -      if (TARGET_EXPR_CLEANUP (targ))
> -	{
> -	  if (CLEANUP_EH_ONLY (targ))
> -	    gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ),
> -				 CLEANUP_EH_ONLY (targ), pre_p);
> -	  else
> -	    cleanup = TARGET_EXPR_CLEANUP (targ);
> -	}
> +      if (init)
> +	gimplify_and_add (init, &init_pre_p);
>  
>        /* Add a clobber for the temporary going out of scope, like
>  	 gimplify_bind_expr.  */
> @@ -7079,8 +7072,13 @@ gimplify_target_expr (tree *expr_p, gimp
>  		}
>  	    }
>  	}
> -      if (cleanup)
> -	gimple_push_cleanup (temp, cleanup, false, pre_p);
> +
> +      gimple_seq_add_seq (pre_p, init_pre_p);
> +
> +      /* If needed, push the cleanup for the temp.  */
> +      if (TARGET_EXPR_CLEANUP (targ))
> +	gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ),
> +			     CLEANUP_EH_ONLY (targ), pre_p);
>  
>        /* Only expand this once.  */
>        TREE_OPERAND (targ, 3) = init;
> --- gcc/testsuite/g++.dg/opt/pr103984.C.jj	2022-03-16 15:28:54.756742692 +0100
> +++ gcc/testsuite/g++.dg/opt/pr103984.C	2022-03-16 15:28:54.756742692 +0100
> @@ -0,0 +1,31 @@
> +// PR middle-end/103984
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -Wuninitialized" }
> +
> +struct A {
> +  char *a;
> +  char b[4];
> +  A ();
> +  A (const A &);
> +  A (const char *);
> +  A (const char *, const char *);
> +  [[gnu::always_inline]] ~A () { if (a != b) delete a; }
> +};
> +struct B {
> +  const char *c = nullptr;
> +  const char *d = nullptr;
> +  A qux () const { return A (c, d); }
> +  B (const char *x) : c(x), d(x) { d += __builtin_strlen (x); }
> +  B (const B &x) { c = x.c; d = x.d; }
> +};
> +struct C { A e; int f; };
> +extern int baz (B);
> +void bar (C &&);
> +
> +void
> +foo (char **x)
> +{
> +  const A g ("foo");
> +  const B h = x[0];
> +  bar (C { h.qux (), baz (h) });
> +}
> 
> 	Jakub
> 
>
diff mbox series

Patch

--- gcc/gimplify.cc.jj	2022-03-16 10:55:58.426014897 +0100
+++ gcc/gimplify.cc	2022-03-16 15:41:06.122913465 +0100
@@ -6997,17 +6997,17 @@  gimplify_target_expr (tree *expr_p, gimp
 
   if (init)
     {
-      tree cleanup = NULL_TREE;
+      gimple_seq init_pre_p = NULL;
 
       /* TARGET_EXPR temps aren't part of the enclosing block, so add it
 	 to the temps list.  Handle also variable length TARGET_EXPRs.  */
       if (!poly_int_tree_p (DECL_SIZE (temp)))
 	{
 	  if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
-	    gimplify_type_sizes (TREE_TYPE (temp), pre_p);
+	    gimplify_type_sizes (TREE_TYPE (temp), &init_pre_p);
 	  /* FIXME: this is correct only when the size of the type does
 	     not depend on expressions evaluated in init.  */
-	  gimplify_vla_decl (temp, pre_p);
+	  gimplify_vla_decl (temp, &init_pre_p);
 	}
       else
 	{
@@ -7022,12 +7022,14 @@  gimplify_target_expr (tree *expr_p, gimp
       /* If TARGET_EXPR_INITIAL is void, then the mere evaluation of the
 	 expression is supposed to initialize the slot.  */
       if (VOID_TYPE_P (TREE_TYPE (init)))
-	ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none);
+	ret = gimplify_expr (&init, &init_pre_p, post_p, is_gimple_stmt,
+			     fb_none);
       else
 	{
 	  tree init_expr = build2 (INIT_EXPR, void_type_node, temp, init);
 	  init = init_expr;
-	  ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none);
+	  ret = gimplify_expr (&init, &init_pre_p, post_p, is_gimple_stmt,
+			       fb_none);
 	  init = NULL;
 	  ggc_free (init_expr);
 	}
@@ -7037,18 +7039,9 @@  gimplify_target_expr (tree *expr_p, gimp
 	  TARGET_EXPR_INITIAL (targ) = NULL_TREE;
 	  return GS_ERROR;
 	}
-      if (init)
-	gimplify_and_add (init, pre_p);
 
-      /* If needed, push the cleanup for the temp.  */
-      if (TARGET_EXPR_CLEANUP (targ))
-	{
-	  if (CLEANUP_EH_ONLY (targ))
-	    gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ),
-				 CLEANUP_EH_ONLY (targ), pre_p);
-	  else
-	    cleanup = TARGET_EXPR_CLEANUP (targ);
-	}
+      if (init)
+	gimplify_and_add (init, &init_pre_p);
 
       /* Add a clobber for the temporary going out of scope, like
 	 gimplify_bind_expr.  */
@@ -7079,8 +7072,13 @@  gimplify_target_expr (tree *expr_p, gimp
 		}
 	    }
 	}
-      if (cleanup)
-	gimple_push_cleanup (temp, cleanup, false, pre_p);
+
+      gimple_seq_add_seq (pre_p, init_pre_p);
+
+      /* If needed, push the cleanup for the temp.  */
+      if (TARGET_EXPR_CLEANUP (targ))
+	gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ),
+			     CLEANUP_EH_ONLY (targ), pre_p);
 
       /* Only expand this once.  */
       TREE_OPERAND (targ, 3) = init;
--- gcc/testsuite/g++.dg/opt/pr103984.C.jj	2022-03-16 15:28:54.756742692 +0100
+++ gcc/testsuite/g++.dg/opt/pr103984.C	2022-03-16 15:28:54.756742692 +0100
@@ -0,0 +1,31 @@ 
+// PR middle-end/103984
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2 -Wuninitialized" }
+
+struct A {
+  char *a;
+  char b[4];
+  A ();
+  A (const A &);
+  A (const char *);
+  A (const char *, const char *);
+  [[gnu::always_inline]] ~A () { if (a != b) delete a; }
+};
+struct B {
+  const char *c = nullptr;
+  const char *d = nullptr;
+  A qux () const { return A (c, d); }
+  B (const char *x) : c(x), d(x) { d += __builtin_strlen (x); }
+  B (const B &x) { c = x.c; d = x.d; }
+};
+struct C { A e; int f; };
+extern int baz (B);
+void bar (C &&);
+
+void
+foo (char **x)
+{
+  const A g ("foo");
+  const B h = x[0];
+  bar (C { h.qux (), baz (h) });
+}