[PATCH] PR c++/27574

[PATCH] PR c++/27574

[Dodji Seketeli]

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

Hello,

This patch was initially done by Ian Lance Taylor to fix PR c++/27574 in 
gcc 4.1, but never got applied.

I am interested in seeing this fixed in gcc 4.1, 4.2, 4.3 and trunk so I 
tested the patch on those branches and it works and fixes the problem.

I did regtested it on those branches for x86_64.

Assuming the patch looks good, would it be okay to commit it to 4.1, 4.2 
and 4.3 ?

This morning's trunk looked to be a bit broken (some tests were not 
passing) so I'd maybe wait a bit for trunk to stabilize before pushing 
it to trunk.

Thanks

[-- Attachment #2: PR27574-patch.txt --]
[-- Type: text/plain, Size: 1768 bytes --]

gcc/ChangeLog:
2008-09-04  Dodji Seketeli  <dodji@redhat.com>

	PR c++/27574
	* gcc/cgraph.c (cgraph_remove_node): Do not remove the body of
	  abstract functions. It might be useful to emit debugging
	  information. This is a patch from Ian Lance Taylor.

gcc/testsuite/ChangeLog:
2008-09-04  Dodji Seketeli  <dodji@gnome.org>

	PR c++/27574
	* g++.dg/debug/dwarf2/local-var-in-contructor.C: New testcase.

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 199c639..3d21190 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -479,6 +479,14 @@ cgraph_remove_node (struct cgraph_node *node)
 	kill_body = true;
     }
 
+  /* We don't release the body of abstract functions, because they may
+     be needed when emitting debugging information.  In particular
+     this will happen for C++ constructors/destructors.  FIXME:
+     Ideally we would check to see whether there are any reachable
+     functions whose DECL_ABSTRACT_ORIGIN points to this decl.  */
+  if (DECL_ABSTRACT (node->decl))
+    kill_body = false;
+
   if (kill_body && !dump_enabled_p (TDI_tree_all) && flag_unit_at_a_time)
     {
       DECL_SAVED_TREE (node->decl) = NULL;
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
new file mode 100644
index 0000000..d61d27f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
@@ -0,0 +1,30 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR27574
+// { dg-do compile }
+// { dg-options "-O0 -g" }
+// { dg-final { scan-assembler "problem" } }
+
+void f (int *)
+{
+}
+
+class A
+{
+public:
+ A(int i);
+};
+
+A::A(int i)
+{
+ int *problem = new int(i);
+ f (problem);
+}
+
+int
+main (void)
+{
+  A a (0);
+
+  return 0;
+}
+

Re: [PATCH] PR c++/27574

[Richard Guenther]

On Thu, Sep 4, 2008 at 11:45 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> Hello,
>
> This patch was initially done by Ian Lance Taylor to fix PR c++/27574 in gcc
> 4.1, but never got applied.
>
> I am interested in seeing this fixed in gcc 4.1, 4.2, 4.3 and trunk so I
> tested the patch on those branches and it works and fixes the problem.
>
> I did regtested it on those branches for x86_64.
>
> Assuming the patch looks good, would it be okay to commit it to 4.1, 4.2 and
> 4.3 ?

Note that the 4.1 branch is closed.

Richard.

Re: [PATCH] PR c++/27574

[Jason Merrill]

Richard Guenther wrote:
> Note that the 4.1 branch is closed.

In the past, I believe the policy has been that it's OK to check in 
patches on closed branches if they are appropriate for a release branch; 
closed just means there won't be any more releases.

Jason

[PING] PATCH PR c++/27574

[Dodji Seketeli]

Hello,

This is a PING for patch submission 
http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00418.html .

Thanks in advance,

Dodji.

Re: [PATCH] PR c++/27574

[Mark Mitchell]

Jason Merrill wrote:
> Richard Guenther wrote:
>> Note that the 4.1 branch is closed.
> 
> In the past, I believe the policy has been that it's OK to check in
> patches on closed branches if they are appropriate for a release branch;
> closed just means there won't be any more releases.

Yes, that's my understanding of the policy as well.  I see no harm if
people want to continue maintaining old branches under the usual
release-branch rules.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] PR c++/27574

[Jason Merrill]

Seems like the right fix would be to include clones in the inlined_to 
list of the abstract function.

Jason

Re: [PATCH] PR c++/27574

[Richard Guenther]

On Tue, Sep 23, 2008 at 8:17 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Jason Merrill wrote:
>> Richard Guenther wrote:
>>> Note that the 4.1 branch is closed.
>>
>> In the past, I believe the policy has been that it's OK to check in
>> patches on closed branches if they are appropriate for a release branch;
>> closed just means there won't be any more releases.
>
> Yes, that's my understanding of the policy as well.  I see no harm if
> people want to continue maintaining old branches under the usual
> release-branch rules.

I thought for the 4.1 branch there was the additional issue that we
do not want to make it GPLv3.  So while individual contributions under
GLPv2 are ok, backporting other peoples work may create a problem.

So I thought better really close the branch to avoid even thinking about
this issue.

Richard.

> --
> Mark Mitchell
> CodeSourcery
> mark@codesourcery.com
> (650) 331-3385 x713
>

Re: [PATCH] PR c++/27574

[Mark Mitchell]

Richard Guenther wrote:

> I thought for the 4.1 branch there was the additional issue that we
> do not want to make it GPLv3.  So while individual contributions under
> GLPv2 are ok, backporting other peoples work may create a problem.
> 
> So I thought better really close the branch to avoid even thinking about
> this issue.

That's a good point.

I suppose we could apply GPLv3 to the 4.1 branch, but I agree that we
shouldn't do that.  Of course, the worst thing would be to apply a GPLv3
patch without fully converting the license notices, which would be very
confusing.

But, as long as the patch is GPLv2, I don't see a particular problem.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] PR c++/27574

[Dodji Seketeli]

Hello,

Jason Merrill a écrit :
> Seems like the right fix would be to include clones in the inlined_to 
> list of the abstract function.
> 

After talking with Honza on IRC, it appeared that it wouldn't work 
because nodes that are added to inline_to must be actually inlined 
otherwise, later sanity check that would break.

I guess Honza can give more details about this :-)

Cheers,

Dodji.

Re: [PATCH] PR c++/27574

[Jason Merrill]

Dodji Seketeli wrote:
> Jason Merrill a écrit :
>> Seems like the right fix would be to include clones in the inlined_to 
>> list of the abstract function.
> 
> After talking with Honza on IRC, it appeared that it wouldn't work 
> because nodes that are added to inline_to must be actually inlined 
> otherwise, later sanity check that would break.

Can't the later sanity check check for inlining or cloning?

Jason

Re: [PATCH] PR c++/27574

[Jan Hubicka]

> Dodji Seketeli wrote:
> >Jason Merrill a ĂŠcrit :
> >>Seems like the right fix would be to include clones in the inlined_to 
> >>list of the abstract function.
> >
> >After talking with Honza on IRC, it appeared that it wouldn't work 
> >because nodes that are added to inline_to must be actually inlined 
> >otherwise, later sanity check that would break.
> 
> Can't the later sanity check check for inlining or cloning?

We definitly can remove the sanity check, but the inline clones are
designed for representing inline plan (i.e. producing virtual clones
that do not have actual tree representation), so using them for linking
functions that was originaly produced as clones by frontend but appear
as real functions in backend does not make that much sense.

If we want to have functions related together, we definitly can add new
lists for that, same way as we do for nested functions and such.

Honza
> 
> Jason

Re: [PATCH] PR c++/27574

[Jason Merrill]

Jan Hubicka wrote:
> If we want to have functions related together, we definitly can add new
> lists for that, same way as we do for nested functions and such.

My inclination would be to do this for trunk, and apply Ian's patch to 
the release branches.

Jason

Re: [PATCH] PR c++/27574

[Dodji Seketeli]

Hello,

Jason Merrill a ĂŠcrit :
> Jan Hubicka wrote:
>> If we want to have functions related together, we definitly can add new
>> lists for that, same way as we do for nested functions and such.
> 
> My inclination would be to do this for trunk, and apply Ian's patch to 
> the release branches.

Jan, I did post a superset of Ian's patch to make the sanity check code 
not cry when it encounters abstract functions. Could you please tell if 
it looks okay to you so that I can push it to the released branches ? 
The post is at http://gcc.gnu.org/ml/gcc-patches/2008-09/msg01815.html.

Now for the approach to use in trunk, there is something I am not sure 
about.

My understanding is that for each node which node->decl is abstract, 
we'd like to build a node->global->cloned_to list of nodes that points 
to the clones of the abstract node.
That'd allow the code of cgraph_remove_node() check for the presence of 
node->global->cloned_to list and not kill the body of the node if that 
list is present.

Now my problem is where (in which function) to build that list ?

As this is mainly targeted at abstract constructors, I could -- in 
build_clone(), gcc/cp/class.c -- add and update a list of clones to each 
abstract function that is cloned. That list of clones would then be used 
to update the node->global->cloned_to list, in 
cgraph_finalize_compilation_unit(), in cgraphunit.c. I am not sure if 
this would be the right approach though.

Cheers,

Dodji.

Re: [PATCH] PR c++/27574

[Jason Merrill]

Dodji Seketeli wrote:
> Jan, I did post a superset of Ian's patch to make the sanity check code 
> not cry when it encounters abstract functions. Could you please tell if 
> it looks okay to you so that I can push it to the released branches ? 
> The post is at http://gcc.gnu.org/ml/gcc-patches/2008-09/msg01815.html.

s/comply/complain/

> Now for the approach to use in trunk, there is something I am not sure 
> about.
> 
> My understanding is that for each node which node->decl is abstract, 
> we'd like to build a node->global->cloned_to list of nodes that points 
> to the clones of the abstract node.
> That'd allow the code of cgraph_remove_node() check for the presence of 
> node->global->cloned_to list and not kill the body of the node if that 
> list is present.

I think it would be better to do it the other way around; if a function 
we still need has DECL_ABSTRACT_ORIGIN, we need to keep the decl it 
points to.  But again, I don't know the cgraph code.

Jason

Re: [PATCH] PR c++/27574

[Dodji Seketeli]

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

Jason Merrill a ĂŠcrit :

[...]

>> Jan, I did post a superset of Ian's patch to make the sanity check 
>> code not cry when it encounters abstract functions. Could you please 
>> tell if it looks okay to you so that I can push it to the released 
>> branches ? The post is at 
>> http://gcc.gnu.org/ml/gcc-patches/2008-09/msg01815.html.
> 
> s/comply/complain/
>

Thanks. The first attached patch named 
PR27574-patch-for-released-branches.txt fixes that.

[...]

> I think it would be better to do it the other way around; if a function 
> we still need has DECL_ABSTRACT_ORIGIN, we need to keep the decl it 
> points to.

[...]

The second attached patch named patches/PR27574-patch-for-trunk.txt 
tries to implement that approach.

So would the first patch be OK to apply to the released branches and the 
second for trunk ? Both pass distcheck on x86_64.

Cheers,

Dodji.


[-- Attachment #2: PR27574-patch-for-released-branches.txt --]
[-- Type: text/plain, Size: 2467 bytes --]

gcc/Changelog:
    PR c++/27574
    * gcc/cgraph.c (cgraph_remove_node): Do not remove the body of
      abstract functions. It might be useful to emit debugging
      information. This is a patch from Ian Lance Taylor.
    * cgraphunit.c (cgraph_optimize): Do not cry when bodies of abstract
      functions are still around. They are useful to output debug info.

gcc/testsuite/ChangeLog:
    PR c++/27574
    * g++.dg/debug/dwarf2/local-var-in-contructor.C: New testcase.


diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 199c639..3d21190 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -479,6 +479,14 @@ cgraph_remove_node (struct cgraph_node *node)
 	kill_body = true;
     }
 
+  /* We don't release the body of abstract functions, because they may
+     be needed when emitting debugging information.  In particular
+     this will happen for C++ constructors/destructors.  FIXME:
+     Ideally we would check to see whether there are any reachable
+     functions whose DECL_ABSTRACT_ORIGIN points to this decl.  */
+  if (DECL_ABSTRACT (node->decl))
+    kill_body = false;
+
   if (kill_body && !dump_enabled_p (TDI_tree_all) && flag_unit_at_a_time)
     {
       DECL_SAVED_TREE (node->decl) = NULL;
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 65a8651..83b070d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1391,7 +1391,10 @@ cgraph_optimize (void)
       for (node = cgraph_nodes; node; node = node->next)
 	if (node->analyzed
 	    && (node->global.inlined_to
-	        || DECL_SAVED_TREE (node->decl)))
+	        || DECL_SAVED_TREE (node->decl))
+            /* Abstract functions are needed to output debug info,
+               so don't complain about them if they are still around.  */
+            && !DECL_ABSTRACT (node->decl))
 	  {
 	    error_found = true;
 	    dump_cgraph_node (stderr, node);
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
new file mode 100644
index 0000000..d61d27f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
@@ -0,0 +1,30 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR27574
+// { dg-do compile }
+// { dg-options "-O0 -g" }
+// { dg-final { scan-assembler "problem" } }
+
+void f (int *)
+{
+}
+
+class A
+{
+public:
+ A(int i);
+};
+
+A::A(int i)
+{
+ int *problem = new int(i);
+ f (problem);
+}
+
+int
+main (void)
+{
+  A a (0);
+
+  return 0;
+}
+

[-- Attachment #3: PR27574-patch-for-trunk.txt --]
[-- Type: text/plain, Size: 5208 bytes --]

gcc/ChangeLog:
2008-10-08  Dodji Seketeli  <dodji@redhat.com>

	PR debug/27574
	* cgraph.h: New abstract_and_needed member to struct cgraph_node.
	* cgraphunit.c (cgraph_analyze_functions): Flag abstract functions
	  - which clones are reachable - as "abstract and needed".
	  Then make sure the body of those abstract and needed functions
	  are not dropped because they are used to emit debug info.
	  (cgraph_mark_functions_to_output): Do not complain if some abstract and
	  needed functions are still around.
	  (cgraph_optimize): Likewise.
	* ipa.c (cgraph_remove_unreachable_nodes): Do not remove abstract and
	  needed functions nodes.

gcc/testsuite/ChangeLog:
2008-10-08  Dodji Seketeli  <dodji@redhat.com>

	PR debug/27574
	* g++.dg/debug/dwarf2/local-var-in-contructor.C: New test.


diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index a6018dc..7825804 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -166,6 +166,9 @@ struct cgraph_node GTY((chain_next ("%h.next"), chain_prev ("%h.previous")))
   /* Set when function must be output - it is externally visible
      or its address is taken.  */
   unsigned needed : 1;
+  /* Set when decl is an abstract function pointed to by the
+     ABSTRACT_DECL_ORIGIN of a reachable function.  */
+  unsigned abstract_and_needed : 1;
   /* Set when function is reachable by call from other function
      that is either reachable or needed.  */
   unsigned reachable : 1;
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 7c84573..754d4e7 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -896,6 +896,15 @@ cgraph_analyze_functions (void)
 	if (!edge->callee->reachable)
 	  cgraph_mark_reachable_node (edge->callee);
 
+      /* If decl is a clone of an abstract function, mark that abstract
+	 function so that we don't release its body. That abstract function body will
+	 be needed to output debug info.  */
+      if (DECL_ABSTRACT_ORIGIN (decl))
+	{
+	  struct cgraph_node *origin_node = cgraph_node (DECL_ABSTRACT_ORIGIN (decl));
+	  origin_node->abstract_and_needed = true;
+	}
+
       /* We finalize local static variables during constructing callgraph
          edges.  Process their attributes too.  */
       process_function_and_variable_attributes (first_processed,
@@ -928,7 +937,7 @@ cgraph_analyze_functions (void)
       if (node->local.finalized && !gimple_has_body_p (decl))
 	cgraph_reset_node (node);
 
-      if (!node->reachable && gimple_has_body_p (decl))
+      if (!node->reachable && gimple_has_body_p (decl) && !node->abstract_and_needed)
 	{
 	  if (cgraph_dump_file)
 	    fprintf (cgraph_dump_file, " %s", cgraph_node_name (node));
@@ -1003,7 +1012,8 @@ cgraph_mark_functions_to_output (void)
 #ifdef ENABLE_CHECKING
 	  if (!node->global.inlined_to
 	      && gimple_has_body_p (decl)
-	      && !DECL_EXTERNAL (decl))
+	      && !DECL_EXTERNAL (decl)
+	      && !node->abstract_and_needed)
 	    {
 	      dump_cgraph_node (stderr, node);
 	      internal_error ("failed to reclaim unneeded function");
@@ -1011,7 +1021,8 @@ cgraph_mark_functions_to_output (void)
 #endif
 	  gcc_assert (node->global.inlined_to
 		      || !gimple_has_body_p (decl)
-		      || DECL_EXTERNAL (decl));
+		      || DECL_EXTERNAL (decl)
+		      || node->abstract_and_needed);
 
 	}
 
@@ -1325,7 +1336,11 @@ cgraph_optimize (void)
       for (node = cgraph_nodes; node; node = node->next)
 	if (node->analyzed
 	    && (node->global.inlined_to
-		|| gimple_has_body_p (node->decl)))
+		|| gimple_has_body_p (node->decl))
+            /* Abstract functions are needed to output debug info,
+	       so don't complain about them if they are
+               still around.  */
+	    && !node->abstract_and_needed)
 	  {
 	    error_found = true;
 	    dump_cgraph_node (stderr, node);
diff --git a/gcc/ipa.c b/gcc/ipa.c
index 0e2cb2d..e5c3115 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -160,8 +160,8 @@ cgraph_remove_unreachable_nodes (bool before_inlining_p, FILE *file)
           node->global.inlined_to = NULL;
 	  if (file)
 	    fprintf (file, " %s", cgraph_node_name (node));
-	  if (!node->analyzed || !DECL_EXTERNAL (node->decl)
-	      || before_inlining_p)
+	  if ((!node->analyzed || !DECL_EXTERNAL (node->decl)
+	      || before_inlining_p) && !node->abstract_and_needed)
 	    cgraph_remove_node (node);
 	  else
 	    {
@@ -187,7 +187,7 @@ cgraph_remove_unreachable_nodes (bool before_inlining_p, FILE *file)
 		  node->analyzed = false;
 		  node->local.inlinable = false;
 		}
-	      else
+	      else if (!node->abstract_and_needed)
 		cgraph_remove_node (node);
 	    }
 	  changed = true;
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
new file mode 100644
index 0000000..d61d27f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
@@ -0,0 +1,30 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR27574
+// { dg-do compile }
+// { dg-options "-O0 -g" }
+// { dg-final { scan-assembler "problem" } }
+
+void f (int *)
+{
+}
+
+class A
+{
+public:
+ A(int i);
+};
+
+A::A(int i)
+{
+ int *problem = new int(i);
+ f (problem);
+}
+
+int
+main (void)
+{
+  A a (0);
+
+  return 0;
+}
+

Re: [PATCH] PR c++/27574

[Jason Merrill]

Dodji Seketeli wrote:
> So would the first patch be OK to apply to the released branches and the 
> second for trunk ? Both pass distcheck on x86_64.

They look fine to me, but I'd like someone who knows cgraph sign off on 
them.

Jason

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

Hello Jan (and maybe to any other callgraph maintainer out there),

This is a ping for a review of the patch I posted at 
http://gcc.gnu.org/ml/gcc-patches/2008-10/msg00367.html .

Cheers,

Dodji.

Re: [PATCH] PR c++/27574 (PING)

[Richard Guenther]

On Mon, Nov 3, 2008 at 12:25 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> Hello Jan (and maybe to any other callgraph maintainer out there),
>
> This is a ping for a review of the patch I posted at
> http://gcc.gnu.org/ml/gcc-patches/2008-10/msg00367.html .

Hm.  What is the effect on text size for C++ applications?  We do not
need the function text itself but only its debug information - the debug
information for the decl, not the text, correct?

Richard.

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

Richard Guenther a écrit :
> Hm.  What is the effect on text size for C++ applications?

Well, I don't have any hard figures for that, but I suspect that as the 
only the bodies of abstract functions which clones are reachable (and 
thus which bodies are emitted) are now being added, the text size might 
grow only very slightly.

In any case, I am rebuilding/installing a gcc tree here so that I can 
compile some c++ programs of mine to come up with some data.

>  We do not  need the function text itself but only its debug information - the debug
> information for the decl, not the text, correct?

I think for a function decl, we "only" need DECL_ARGUMENTS (decl) and 
DECL_INITIAL (decl) to be present so that we can generate debug info.


Dodji.

Re: [PATCH] PR c++/27574 (PING)

[Richard Guenther]

On Mon, Nov 3, 2008 at 2:53 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> Richard Guenther a écrit :
>>
>> Hm.  What is the effect on text size for C++ applications?
>
> Well, I don't have any hard figures for that, but I suspect that as the only
> the bodies of abstract functions which clones are reachable (and thus which
> bodies are emitted) are now being added, the text size might grow only very
> slightly.
>
> In any case, I am rebuilding/installing a gcc tree here so that I can
> compile some c++ programs of mine to come up with some data.
>
>>  We do not  need the function text itself but only its debug information -
>> the debug
>> information for the decl, not the text, correct?
>
> I think for a function decl, we "only" need DECL_ARGUMENTS (decl) and
> DECL_INITIAL (decl) to be present so that we can generate debug info.

So the only problem is the following line

  DECL_INITIAL (node->decl) = error_mark_node;

in cgraph_release_function_body?  Does it fix the PR
if you make that conditional on DECL_ABSTRACT?

Thanks,
Richard.

Re: [PATCH] PR c++/27574 (PING)

[Jan Hubicka]

> On Mon, Nov 3, 2008 at 2:53 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> > Richard Guenther a ĂŠcrit :
> >>
> >> Hm.  What is the effect on text size for C++ applications?
> >
> > Well, I don't have any hard figures for that, but I suspect that as the only
> > the bodies of abstract functions which clones are reachable (and thus which
> > bodies are emitted) are now being added, the text size might grow only very
> > slightly.
> >
> > In any case, I am rebuilding/installing a gcc tree here so that I can
> > compile some c++ programs of mine to come up with some data.
> >
> >>  We do not  need the function text itself but only its debug information -
> >> the debug
> >> information for the decl, not the text, correct?
> >
> > I think for a function decl, we "only" need DECL_ARGUMENTS (decl) and
> > DECL_INITIAL (decl) to be present so that we can generate debug info.
> 
> So the only problem is the following line
> 
>   DECL_INITIAL (node->decl) = error_mark_node;
> 
> in cgraph_release_function_body?  Does it fix the PR
> if you make that conditional on DECL_ABSTRACT?

The reason for setting DECL_INITIAL there was that BLOCK pointers used
to point to generic function body that remained in memory.
I think it is not needed at all and might not be the case anymore, so it
might be fine.  Still blocks occupy quite a lot of ram, can dwarf2out be
in charge of releasing them?

Honza
> 
> Thanks,
> Richard.

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

Richard Guenther a écrit :

> So the only problem is the following line
> 
>   DECL_INITIAL (node->decl) = error_mark_node;
> 
> in cgraph_release_function_body?  Does it fix the PR
> if you make that conditional on DECL_ABSTRACT?

Yes, it fixes the problem. And in a much more compact way than what I 
was trying to do. Thanks.

I am still running regtests (for C and C++). I will post a new patch 
when that's done.

Thanks,

Dodji.

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

Jan Hubicka a écrit :

[...]

> The reason for setting DECL_INITIAL there was that BLOCK pointers used
> to point to generic function body that remained in memory.

Okay.

> I think it is not needed at all and might not be the case anymore, so it
> might be fine.

Yeah, it looks like it's not needed.

>  Still blocks occupy quite a lot of ram, can dwarf2out be
> in charge of releasing them?

You mean, you'd want dwarf2out to set the DECL_INITIAL (decl) to 
error_mark_node after emitting the debug info ? I certainly could do 
that, assuming that does not go against the spirit of dwarf2out.

Cheers,

Dodji.

Re: [PATCH] PR c++/27574 (PING)

[Jason Merrill]

Dodji Seketeli wrote:
> Jan Hubicka a écrit :
>> Still blocks occupy quite a lot of ram, can dwarf2out be
>> in charge of releasing them?
> 
> You mean, you'd want dwarf2out to set the DECL_INITIAL (decl) to 
> error_mark_node after emitting the debug info ? I certainly could do 
> that, assuming that does not go against the spirit of dwarf2out.

This doesn't seem specific to dwarf2out; dbxout also uses the block 
structure in DECL_INITIAL.  I'd think that clearing DECL_INITIAL should 
happen in code independent of which debugging backend is used.

Jason

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

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

Okay, so here is another patch proposal.

It passes regtest on trunk for x86_64. It think I can even use this one 
for the other branches, assuming it passes regtest.

Thanks,

Dodji.

[-- Attachment #2: PR27574-patch.txt --]
[-- Type: text/plain, Size: 1849 bytes --]

gcc/ChangeLog/
2008-11-03  Dodji Seketeli  <dodji@redhat.com>

	* cgraph.c (cgraph_release_function_body): Do not kill DECL_INITIAL()
	of the function decl. It's useful to emit debug info.

gcc/testsuite/ChangeLog:
2008-11-03  Dodji Seketeli  <dodji@redhat.com>

	* g++.dg/debug/dwarf2/local-var-in-contructor.C: New test.

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 73dbfb3..6cbf87b 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -936,7 +936,6 @@ cgraph_release_function_body (struct cgraph_node *node)
       DECL_STRUCT_FUNCTION (node->decl) = NULL;
     }
   DECL_SAVED_TREE (node->decl) = NULL;
-  DECL_INITIAL (node->decl) = error_mark_node;
 }
 
 /* Remove the node from cgraph.  */
diff --git a/gcc/final.c b/gcc/final.c
index e2d9e5a..59c310f 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4186,6 +4186,9 @@ rest_of_handle_final (void)
 
   timevar_push (TV_SYMOUT);
   (*debug_hooks->function_decl) (current_function_decl);
+  /* Release the BLOCKs linked to DECL_INTIAL (current_function_decl)
+     to free up some memory.  */
+  DECL_INITIAL (current_function_decl) = error_mark_node;
   timevar_pop (TV_SYMOUT);
   if (DECL_STATIC_CONSTRUCTOR (current_function_decl)
       && targetm.have_ctors_dtors)
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
new file mode 100644
index 0000000..d61d27f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
@@ -0,0 +1,30 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR27574
+// { dg-do compile }
+// { dg-options "-O0 -g" }
+// { dg-final { scan-assembler "problem" } }
+
+void f (int *)
+{
+}
+
+class A
+{
+public:
+ A(int i);
+};
+
+A::A(int i)
+{
+ int *problem = new int(i);
+ f (problem);
+}
+
+int
+main (void)
+{
+  A a (0);
+
+  return 0;
+}
+

Re: [PATCH] PR c++/27574 (PING)

[Jason Merrill]

Seems like this won't solve the problem of wanting to discard the 
information for unreachable functions.

Jason

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

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

Jason Merrill a écrit :
> Seems like this won't solve the problem of wanting to discard the 
> information for unreachable functions.

Please find attached a reworked patch that merges the two previous 
approaches.
Basically, I flag abstracts functions which clones are live. Then, at 
function body clearing time, I just prevent the DECL_INITIAL() from 
being nuked as well.

So the bodies of non live functions are always dropped incurring no text 
size increase, but we should get the debug info for the abstract 
functions that are needed.

It passes regtest on trunk for x86_64.

Thanks,

Dodji.

[-- Attachment #2: PR27574-patch.txt --]
[-- Type: text/plain, Size: 3935 bytes --]

gcc/ChangeLog:
2008-11-04  Dodji Seketeli  <dodji@redhat.com>

	PR debug/27574
	* cgraph.h: New abstract_and_needed member to struct cgraph_node.
	* cgraphunit.c (cgraph_analyze_functions): Flag abstract functions
	- which clones are reachable - as "abstract and needed".
	* cgraph.c (cgraph_release_function_body):  If a node is "abstract and needed",
	do not release its DECL_INITIAL() content because that will be needed to emit
	debug info.

gcc/testsuite/ChangeLog:
2008-11-04  Dodji Seketeli  <dodji@redhat.com>

	PR debug/27574
	* g++.dg/debug/dwarf2/local-var-in-contructor.C: New test.

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 73dbfb3..43659cb 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -936,7 +936,11 @@ cgraph_release_function_body (struct cgraph_node *node)
       DECL_STRUCT_FUNCTION (node->decl) = NULL;
     }
   DECL_SAVED_TREE (node->decl) = NULL;
-  DECL_INITIAL (node->decl) = error_mark_node;
+  /* If the node is abstract and needed, then do not clear DECL_INITIAL
+     of its associated function function declaration because it's
+     needed to emit debug info later.  */
+  if (!node->abstract_and_needed)
+    DECL_INITIAL (node->decl) = error_mark_node;
 }
 
 /* Remove the node from cgraph.  */
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index a6018dc..7825804 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -166,6 +166,9 @@ struct cgraph_node GTY((chain_next ("%h.next"), chain_prev ("%h.previous")))
   /* Set when function must be output - it is externally visible
      or its address is taken.  */
   unsigned needed : 1;
+  /* Set when decl is an abstract function pointed to by the
+     ABSTRACT_DECL_ORIGIN of a reachable function.  */
+  unsigned abstract_and_needed : 1;
   /* Set when function is reachable by call from other function
      that is either reachable or needed.  */
   unsigned reachable : 1;
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 7c84573..cd58c2a 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -896,6 +896,15 @@ cgraph_analyze_functions (void)
 	if (!edge->callee->reachable)
 	  cgraph_mark_reachable_node (edge->callee);
 
+      /* If decl is a clone of an abstract function, mark that abstract
+	 function so that we don't release its body. The DECL_INITIAL() of that
+         abstract function declaration will be later needed to output debug info.  */
+      if (DECL_ABSTRACT_ORIGIN (decl))
+	{
+	  struct cgraph_node *origin_node = cgraph_node (DECL_ABSTRACT_ORIGIN (decl));
+	  origin_node->abstract_and_needed = true;
+	}
+
       /* We finalize local static variables during constructing callgraph
          edges.  Process their attributes too.  */
       process_function_and_variable_attributes (first_processed,
diff --git a/gcc/final.c b/gcc/final.c
index e2d9e5a..d007907 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4187,6 +4187,10 @@ rest_of_handle_final (void)
   timevar_push (TV_SYMOUT);
   (*debug_hooks->function_decl) (current_function_decl);
   timevar_pop (TV_SYMOUT);
+
+  /* Release the blocks that are linked to DECL_INITIAL() to free the memory.  */
+  DECL_INITIAL (current_function_decl) = error_mark_node;
+
   if (DECL_STATIC_CONSTRUCTOR (current_function_decl)
       && targetm.have_ctors_dtors)
     targetm.asm_out.constructor (XEXP (DECL_RTL (current_function_decl), 0),
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
new file mode 100644
index 0000000..d61d27f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
@@ -0,0 +1,30 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR27574
+// { dg-do compile }
+// { dg-options "-O0 -g" }
+// { dg-final { scan-assembler "problem" } }
+
+void f (int *)
+{
+}
+
+class A
+{
+public:
+ A(int i);
+};
+
+A::A(int i)
+{
+ int *problem = new int(i);
+ f (problem);
+}
+
+int
+main (void)
+{
+  A a (0);
+
+  return 0;
+}
+

Re: [PATCH] PR c++/27574 (PING)

[Jason Merrill]

Dodji Seketeli wrote:
> Please find attached a reworked patch that merges the two previous 
> approaches.

This looks good to me.

Jason

Re: [PATCH] PR c++/27574 (PING)

[Eric Botcazou]

> Please find attached a reworked patch that merges the two previous
> approaches.

       PR debug/27574
        * cgraph.h: New abstract_and_needed member to struct cgraph_node.
        * cgraphunit.c (cgraph_analyze_functions): Flag abstract functions
        - which clones are reachable - as "abstract and needed".
        * cgraph.c (cgraph_release_function_body):  If a node is "abstract and 
needed",
        do not release its DECL_INITIAL() content because that will be needed 
to emit
        debug info.


Lines are too long.  And never use "because" in a ChangeLog entry as it should 
only describe the "what", not the "why".  The "why" should be in the sources.


-- 
Eric Botcazou

Re: [PATCH] PR c++/27574 (PING)

[Dodji Seketeli]

Eric Botcazou a écrit :
[...]
> Lines are too long.  And never use "because" in a ChangeLog entry as it should 
> only describe the "what", not the "why".  The "why" should be in the sources.

Fixed, thanks.

Dodji.

Re: [PATCH] PR c++/27574 (PING)

[Eric Botcazou]

> Fixed, thanks.

Another nit: the ChangeLog entry is incomplete, final.c is not mentioned.

-- 
Eric Botcazou

Re: [PATCH] PR c++/27574 (PING)

[Eric Botcazou]

> Another nit: the ChangeLog entry is incomplete, final.c is not mentioned.

And the new comment line in final.c is too long, remove the superfluous ().

-- 
Eric Botcazou

Re: [PATCH] PR c++/27574

[Dodji Seketeli]

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

Ian Lance Taylor a écrit :

[...]

> Forwarding Honza's reply back to the list.

[...]

> 
> Hi,
> the patch should cause ICE on enable checking compiler in the loop
> verifying that all function bodies are removed.
> Naturally I would a lot preffer this invriant to remain (that is that we
> don't have live bodies at the end of compilation).
> I guess plan of generating info earlier that would probably make most
> sense to me is not that good for stage3, so I guess the patch is fine
> (if the verification loop is updated to skip abstract functions too).

So in that spirit, would the attached patch be any better ? assuming it 
passes distcheck, of course.

> After all we don't have that many abstract functions produced...
> 
> Do we really need all the body for debug output?  I think tree-SSA pass
> is killing most of it anyway, so if we need something like just
> DECL_INITIAL, then we can perhaps teach cgraph code to keep only that
> around.
> 

Yes, I have the impression that just having DECL_INITIAL should be 
enough. I am not sure we need DECL_SAVED_TREE() to generate the debug 
info, but I am not sure.

Cheers,

Dodji.


[-- Attachment #2: PR27574-patch-1.txt --]
[-- Type: text/plain, Size: 3390 bytes --]

commit 233d1f0161c434818099964ea09c231bb9c283b0
Author: Dodji Seketeli <dodji@redhat.com>
Date:   Fri Sep 5 12:23:33 2008 +0200

    Fix for PR c++/27574
    
    	PR c++/27574
    	* gcc/cgraph.c (cgraph_remove_node): Do not remove the body of
    	  abstract functions. It might be useful to emit debugging
    	  information. This is a patch from Ian Lance Taylor.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 92d7768..a959e56 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,12 @@
+2008-09-26  Dodji Seketeli  <dodji@redhat.com>
+
+	PR c++/27574
+	* cgraph.c (cgraph_remove_node): Do not remove the body of
+	  abstract functions. It might be useful to emit debugging
+	  information. This is a patch from Ian Lance Taylor.
+	* cgraphunit.c (cgraph_optimize): Do not cry when bodies of abstract
+	  functions are still around. They are useful to output debug info.
+
 2008-06-05  Richard Sandiford  <rdsandiford@googlemail.com>
 
 	* config/mips/mips.c (mips_emit_loadgp): Emit a blockage if
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 199c639..3d21190 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -479,6 +479,14 @@ cgraph_remove_node (struct cgraph_node *node)
 	kill_body = true;
     }
 
+  /* We don't release the body of abstract functions, because they may
+     be needed when emitting debugging information.  In particular
+     this will happen for C++ constructors/destructors.  FIXME:
+     Ideally we would check to see whether there are any reachable
+     functions whose DECL_ABSTRACT_ORIGIN points to this decl.  */
+  if (DECL_ABSTRACT (node->decl))
+    kill_body = false;
+
   if (kill_body && !dump_enabled_p (TDI_tree_all) && flag_unit_at_a_time)
     {
       DECL_SAVED_TREE (node->decl) = NULL;
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 65a8651..1fbc80d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1391,7 +1391,10 @@ cgraph_optimize (void)
       for (node = cgraph_nodes; node; node = node->next)
 	if (node->analyzed
 	    && (node->global.inlined_to
-	        || DECL_SAVED_TREE (node->decl)))
+	        || DECL_SAVED_TREE (node->decl))
+            /* Abstract functions are needed to output debug info,
+               so don't comply about them if they are still around.  */
+            && !DECL_ABSTRACT (node->decl))
 	  {
 	    error_found = true;
 	    dump_cgraph_node (stderr, node);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 47a7b2c..2154135 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2008-09-04  Dodji Seketeli  <dodji@gnome.org>
+
+	PR c++/27574
+	* g++.dg/debug/dwarf2/local-var-in-contructor.C: New testcase.
+
 2008-05-29  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* gcc.dg/nested-func-6.c: New test.
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
new file mode 100644
index 0000000..d61d27f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
@@ -0,0 +1,30 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR27574
+// { dg-do compile }
+// { dg-options "-O0 -g" }
+// { dg-final { scan-assembler "problem" } }
+
+void f (int *)
+{
+}
+
+class A
+{
+public:
+ A(int i);
+};
+
+A::A(int i)
+{
+ int *problem = new int(i);
+ f (problem);
+}
+
+int
+main (void)
+{
+  A a (0);
+
+  return 0;
+}
+

Re: [PATCH] PR c++/27574

[Ian Lance Taylor]

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

Forwarding Honza's reply back to the list.

(Although I wrote the original patch I personally never thought it
would be enough by itself.)

Ian


[-- Attachment #2: Type: message/rfc822, Size: 5459 bytes --]

From: Jan Hubicka <jh@suse.cz>
To: Ian Lance Taylor <iant@google.com>
Subject: Re: [Dodji Seketeli] [PATCH] PR c++/27574
Date: Wed, 17 Sep 2008 16:25:18 +0200
Message-ID: <20080917142518.GI27668@kam.mff.cuni.cz>

> Any thoughts on this?

Hi,
the patch should cause ICE on enable checking compiler in the loop
verifying that all function bodies are removed.
Naturally I would a lot preffer this invriant to remain (that is that we
don't have live bodies at the end of compilation).
I guess plan of generating info earlier that would probably make most
sense to me is not that good for stage3, so I guess the patch is fine
(if the verification loop is updated to skip abstract functions too).
After all we don't have that many abstract functions produced...

Do we really need all the body for debug output?  I think tree-SSA pass
is killing most of it anyway, so if we need something like just
DECL_INITIAL, then we can perhaps teach cgraph code to keep only that
around.

Honza
> 
> Ian
> 

> Date: Thu, 04 Sep 2008 23:45:07 +0200
> From: Dodji Seketeli <dodji@redhat.com>
> To: Gcc Patch List <gcc-patches@gcc.gnu.org>
> CC: ian@airs.com, Jason Merrill <jason@redhat.com>
> Subject: [PATCH] PR c++/27574
> User-Agent: Thunderbird 2.0.0.16 (X11/20080723)
> 
> Hello,
> 
> This patch was initially done by Ian Lance Taylor to fix PR c++/27574 in 
> gcc 4.1, but never got applied.
> 
> I am interested in seeing this fixed in gcc 4.1, 4.2, 4.3 and trunk so I 
> tested the patch on those branches and it works and fixes the problem.
> 
> I did regtested it on those branches for x86_64.
> 
> Assuming the patch looks good, would it be okay to commit it to 4.1, 4.2 
> and 4.3 ?
> 
> This morning's trunk looked to be a bit broken (some tests were not 
> passing) so I'd maybe wait a bit for trunk to stabilize before pushing 
> it to trunk.
> 
> Thanks

> gcc/ChangeLog:
> 2008-09-04  Dodji Seketeli  <dodji@redhat.com>
> 
> 	PR c++/27574
> 	* gcc/cgraph.c (cgraph_remove_node): Do not remove the body of
> 	  abstract functions. It might be useful to emit debugging
> 	  information. This is a patch from Ian Lance Taylor.
> 
> gcc/testsuite/ChangeLog:
> 2008-09-04  Dodji Seketeli  <dodji@gnome.org>
> 
> 	PR c++/27574
> 	* g++.dg/debug/dwarf2/local-var-in-contructor.C: New testcase.
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 199c639..3d21190 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -479,6 +479,14 @@ cgraph_remove_node (struct cgraph_node *node)
>  	kill_body = true;
>      }
>  
> +  /* We don't release the body of abstract functions, because they may
> +     be needed when emitting debugging information.  In particular
> +     this will happen for C++ constructors/destructors.  FIXME:
> +     Ideally we would check to see whether there are any reachable
> +     functions whose DECL_ABSTRACT_ORIGIN points to this decl.  */
> +  if (DECL_ABSTRACT (node->decl))
> +    kill_body = false;
> +
>    if (kill_body && !dump_enabled_p (TDI_tree_all) && flag_unit_at_a_time)
>      {
>        DECL_SAVED_TREE (node->decl) = NULL;
> diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
> new file mode 100644
> index 0000000..d61d27f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/debug/dwarf2/local-var-in-contructor.C
> @@ -0,0 +1,30 @@
> +// Contributed by Dodji Seketeli <dodji@redhat.com>
> +// Origin PR27574
> +// { dg-do compile }
> +// { dg-options "-O0 -g" }
> +// { dg-final { scan-assembler "problem" } }
> +
> +void f (int *)
> +{
> +}
> +
> +class A
> +{
> +public:
> + A(int i);
> +};
> +
> +A::A(int i)
> +{
> + int *problem = new int(i);
> + f (problem);
> +}
> +
> +int
> +main (void)
> +{
> +  A a (0);
> +
> +  return 0;
> +}
> +