Re: RFA: PATCH to tree-nrv for debug/39086 (abort in dwarf with -fno-tree-sra)

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sun, Mar 8, 2009 at 2:39 AM, Jason Merrill <jason@redhat.com> wrote:
> Here's what's happening in this testcase:
>
> 1) The front end chooses 'x' as the NRV for f4 and rewrites the assignments.
> 2) inlining creates a temporary variable for the return value of f1. ?So we
> have
>
> <bb 3>:
> ?D.1204 = ax;
> ?<retval> = D.1204;
> ?goto <bb 5>;
>
> <bb 4>:
> ?<retval> = OBJ_TYPE_REF(*this->_vptr.C;this->0) (this);
>
> 3) tree-nrv looks at this, sees <retval> = D.1204, doesn't see the other
> assignment to retval because it's a GIMPLE_CALL rather than a GIMPLE_ASSIGN,
> and decides to make D.1204 the NRV.
>
> 4) tree-nrv copies the debugging information for D.1204 over the debugging
> information for the f4 return value, which already had the info for 'x'.
>
> 5) The f4 retval now has DECL_ABSTRACT_ORIGIN referring to f1, which causes
> chaos.
>
> It seems that there are several bugs here:
>
> * tree-nrv shouldn't do NRV after the front end already did, as it might
> miss modifications that aren't of the form it expects. ?Specifically, it
> doesn't seem to check whether anything takes the address of the RESULT_DECL.
>
> * tree-nrv needs to notice GIMPLE_CALL modifications.
>
> * tree-nrv shouldn't clobber the debug information on the RESULT_DECL in
> favor of another artificial variable.
>
> * tree-nrv shouldn't copy DECL_ABSTRACT_ORIGIN that refers to another
> function.
>
> This patch fixes all of these issues.
>
> OK for trunk?

+   /* If the front end already did something like this, we might not see all
+      possible modifications.  In particular, we don't attempt to notice
+      uses of the address of the RESULT_DECL.  */
+   if (DECL_NAME (result))
+     return 0;
+

Shouldn't this check for !DECL_ARTIFICIAL or !DECL_IGNORED_P?

*************** tree_nrv (void)
*** 173,178 ****
--- 179,187 ----
                                                TREE_TYPE (found)))
                return 0;
            }
+         else if (is_gimple_call (stmt)
+                  && gimple_call_lhs (stmt) == result)
+           return 0;
          else if (is_gimple_assign (stmt))

for completeness and simplicity this should probably be

Index: tree-nrv.c
===================================================================
--- tree-nrv.c	(revision 144693)
+++ tree-nrv.c	(working copy)
@@ -138,8 +138,8 @@ tree_nrv (void)
 	      if (ret_val)
 		gcc_assert (ret_val == result);
 	    }
-	  else if (is_gimple_assign (stmt)
-		   && gimple_assign_lhs (stmt) == result)
+	  else if (gimple_has_lhs (stmt)
+		   && gimple_get_lhs (stmt) == result)
 	    {
               tree rhs;

@@ -173,9 +173,9 @@ tree_nrv (void)
 					        TREE_TYPE (found)))
 		return 0;
 	    }
-	  else if (is_gimple_assign (stmt))
+	  else if (gimple_has_lhs (stmt))
 	    {
-	      tree addr = get_base_address (gimple_assign_lhs (stmt));
+	      tree addr = get_base_address (gimple_get_lhs (stmt));
 	       /* If there's any MODIFY of component of RESULT,
 		  then bail out.  */
 	      if (addr && addr == result)

If you change the entry predicate you should be able to remove the last
hunk.  As of the check for address-taking (you don't seem to fix that), you
could simply do

  if (!found
      || TREE_ADDRESSABLE (found))
    return 0;

which should do the trick.

The patch is ok with the second suggested change, you might want to consider
the others.

Thanks,
Richard.