[PATCH] Improve debug info for Ada (4/4)

[PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

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

Hi,

The gimplifier sometimes creates "artificial" temporaries that end up 
destroying debug info for optimized compilation, for example:

bool
is_gimple_reg_rhs (tree t)
{
  /* If the RHS of the GIMPLE_MODIFY_STMT may throw or make a nonlocal goto
     and the LHS is a user variable, then we need to introduce a formal
     temporary.  This way the optimizers can determine that the user
     variable is only modified if evaluation of the RHS does not throw.

     Don't force a temp of a non-renamable type; the copy could be
     arbitrarily expensive.  Instead we will generate a VDEF for
     the assignment.  */

  if (is_gimple_reg_type (TREE_TYPE (t))
      && ((TREE_CODE (t) == CALL_EXPR && TREE_SIDE_EFFECTS (t))
	  || tree_could_throw_p (t)))
    return false;

On the attached testcase

  types__node_id___XDLU_0__2147483647 D.693;
  types__entity_id___XDLU_0__2147483647 pouet;

  D.693 = elists__node (elmt);
  pouet = D.693;

and the optimizer eventually keeps only D.693 instead of "pouet", the former 
being marked as DECL_IGNORED_P and thus generating no debug info.

The proposed change is to attach the temporary to the user variable in this 
case by means of DECL_DEBUG_EXPR.  This will cause a location list to be 
generated for "pouet" in the debug info with -fvar-tracking.

Bootstrapped/regtested on i586-suse-linux, OK for mainline?


2007-02-12  Eric Botcazou  <ebotcazou@adacore.com>

	* gimplify.c (gimplify_modify_expr): For optimized compilation and
	during gimplification, attach a DECL on the rhs to a DECL on the lhs
	for debug info purposes if the former is ignored and not the latter.


:ADDPATCH debug:

-- 
Eric Botcazou

[-- Attachment #2: g124-028_fsf.diff --]
[-- Type: text/x-diff, Size: 890 bytes --]

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 121686)
+++ gimplify.c	(working copy)
@@ -3654,6 +3654,19 @@ gimplify_modify_expr (tree *expr_p, tree
       *to_p = make_ssa_name (*to_p, *expr_p);
     }
 
+  /* Try to alleviate the effects of the gimplification creating artificial
+     temporaries (see for example is_gimple_reg_rhs) on the debug info.  */
+  if (optimize && !gimplify_ctxp->into_ssa
+      && DECL_P (*from_p) && DECL_IGNORED_P (*from_p)
+      && DECL_P (*to_p) && !DECL_IGNORED_P (*to_p))
+    {
+      if (!DECL_NAME (*from_p) && DECL_NAME (*to_p))
+	DECL_NAME (*from_p)
+	  = create_tmp_var_name (IDENTIFIER_POINTER (DECL_NAME (*to_p)));
+      DECL_DEBUG_EXPR_IS_FROM (*from_p) = 1;
+      SET_DECL_DEBUG_EXPR (*from_p, *to_p);
+    }
+
   if (want_value)
     {
       tree_to_gimple_tuple (expr_p);

[-- Attachment #3: elists.adb --]
[-- Type: text/x-adasrc, Size: 332 bytes --]

package body Elists is

   function Node (Elmt : Node_Id) return Node_Id is
   begin
      return Elmt;
   end Node;

   procedure Next_Elmt (Elmt : in out Node_Id) is
   begin
      Elmt := Elmt + 1;
   end Next_Elmt;

   function Present (Elmt : Node_Id) return Boolean is
   begin
      return True;
   end Present;

end Elists;

[-- Attachment #4: types.ads --]
[-- Type: text/x-adasrc, Size: 95 bytes --]

package Types is

   type Node_Id is new Natural;
   subtype Entity_Id is Node_Id;

end Types;

[-- Attachment #5: sem_ch3.ads --]
[-- Type: text/x-adasrc, Size: 116 bytes --]

with Types;  use Types;

package Sem_Ch3  is

   procedure Check_Abstract_Overriding (T : Entity_Id);

end Sem_Ch3;

[-- Attachment #6: elists.ads --]
[-- Type: text/x-adasrc, Size: 210 bytes --]

with Types;  use Types;

package Elists is

   function Node (Elmt : Node_Id) return Node_Id;

   procedure Next_Elmt (Elmt : in out Node_Id);

   function Present (Elmt : Node_Id) return Boolean;

end Elists;

[-- Attachment #7: sem_ch3.adb --]
[-- Type: text/x-adasrc, Size: 395 bytes --]

with Elists;   use Elists;

package body Sem_Ch3 is

   procedure Check_Abstract_Overriding (T : Entity_Id) is
      Elmt  : Node_Id;
      Pouet : Entity_Id := Entity_Id'First;
   begin
      Elmt := 100_000;
      while Present (Elmt) loop
         Pouet := Node (Elmt);
         exit when Pouet > T;

         Next_Elmt (Elmt);
      end loop;
   end Check_Abstract_Overriding;

end Sem_Ch3;

Re: [PATCH] Improve debug info for Ada (4/4)

[Richard Henderson]

On Mon, Feb 12, 2007 at 08:29:34PM +0100, Eric Botcazou wrote:
> 	* gimplify.c (gimplify_modify_expr): For optimized compilation and
> 	during gimplification, attach a DECL on the rhs to a DECL on the lhs
> 	for debug info purposes if the former is ignored and not the latter.

Definitely the wrong place to do this, I think.  I'd much rather
do it where the temporary itself it created.  And why are you
depending on optimization enabled?


r~

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> Definitely the wrong place to do this, I think.  I'd much rather
> do it where the temporary itself it created.

The thing is that we don't have the lhs at hand.

> And why are you depending on optimization enabled?

This only makes a difference with -fvar-tracking enabled.

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Daniel Jacobowitz]

On Mon, Feb 12, 2007 at 10:43:08PM +0100, Eric Botcazou wrote:
> > And why are you depending on optimization enabled?
> 
> This only makes a difference with -fvar-tracking enabled.

Honestly, I wish we could enable it at -O0 too; the more accurate
debugging information would be useful in some cases.  For instance,
getting location lists for incoming arguments which are correct from
the very first instruction.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Improve debug info for Ada (4/4)

[Richard Henderson]

On Mon, Feb 12, 2007 at 10:43:08PM +0100, Eric Botcazou wrote:
> > Definitely the wrong place to do this, I think.  I'd much rather
> > do it where the temporary itself it created.
> 
> The thing is that we don't have the lhs at hand.

I'm thinking of create_tmp_from_val, where we do in fact
have the lhs at hand.  You'd prolly wanna strike the 
inline marker from this function at the same time.

> This only makes a difference with -fvar-tracking enabled.

Then it would make sense to depend on flag_var_tracking,
not optimization.


r~

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> I'm thinking of create_tmp_from_val, where we do in fact
> have the lhs at hand.  You'd prolly wanna strike the
> inline marker from this function at the same time.

Are we talking about the same lhs?  Mine is pouet, yours is D.693?

  D.693 = elists__node (elmt);
  pouet = D.693;

> Then it would make sense to depend on flag_var_tracking,
> not optimization.

Note that this would be somewhat inconsistent with all the other uses of 
SET_DECL_DEBUG_EXPR, which are not predicated on flag_var_tracking.

The first version of the patch did it unconditionally too, but this is a pure 
waste of time at -O0.

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> Honestly, I wish we could enable it at -O0 too; the more accurate
> debugging information would be useful in some cases.  For instance,
> getting location lists for incoming arguments which are correct from
> the very first instruction.

I'd a little wary of the bloat in the debug info, no?

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> I'd a little wary of the bloat in the debug info, no?

... be a little wary...

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Daniel Jacobowitz]

On Wed, Feb 14, 2007 at 08:04:39AM +0100, Eric Botcazou wrote:
> > Honestly, I wish we could enable it at -O0 too; the more accurate
> > debugging information would be useful in some cases.  For instance,
> > getting location lists for incoming arguments which are correct from
> > the very first instruction.
> 
> I'd a little wary of the bloat in the debug info, no?

There ought to be relatively few locations.  You could probably
compromise and not show every moment it was loaded into a register
from its assigned stack slot, and still do OK.  I think this is just a
consequence of our already overly pessimistic -O0 - compare to what
some other compilers do with "no" optimization...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Improve debug info for Ada (4/4)

[Richard Henderson]

On Wed, Feb 14, 2007 at 08:02:59AM +0100, Eric Botcazou wrote:
> Are we talking about the same lhs?  Mine is pouet, yours is D.693?
> 
>   D.693 = elists__node (elmt);
>   pouet = D.693;

Apparently not.  I guess I don't see any other way to do what
you want with that.  So I'm fine with the change.

> Note that this would be somewhat inconsistent with all the other uses of 
> SET_DECL_DEBUG_EXPR, which are not predicated on flag_var_tracking.

They aren't predicated on optimization either.


r~

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> Apparently not.  I guess I don't see any other way to do what
> you want with that.  So I'm fine with the change.

OK, thanks.

> > Note that this would be somewhat inconsistent with all the other uses of
> > SET_DECL_DEBUG_EXPR, which are not predicated on flag_var_tracking.
>
> They aren't predicated on optimization either.

Most are, indirectly.  This may seem non-obvious, but I don't really have a 
strong opinion. :-)  OK for ditching the condition altogether?

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Richard Henderson]

On Thu, Feb 15, 2007 at 08:22:48PM +0100, Eric Botcazou wrote:
> Most are, indirectly.  This may seem non-obvious, but I don't really have a 
> strong opinion. :-)  OK for ditching the condition altogether?

Yep.


r~

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> There ought to be relatively few locations.  You could probably
> compromise and not show every moment it was loaded into a register
> from its assigned stack slot, and still do OK.  I think this is just a
> consequence of our already overly pessimistic -O0 - compare to what
> some other compilers do with "no" optimization...

I'm starting to get convinced. :-)  It turns out that -fvar-tracking is 
probably necessary to get fully correct debug info for parameters that
are referenced in nested functions.

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Daniel Jacobowitz]

On Sat, Feb 17, 2007 at 08:24:20PM +0100, Eric Botcazou wrote:
> I'm starting to get convinced. :-)  It turns out that -fvar-tracking is 
> probably necessary to get fully correct debug info for parameters that
> are referenced in nested functions.

Does this mean I can sucker you into making it work at -O0? :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> Does this mean I can sucker you into making it work at -O0? :-)

So it doesn't work at -O0? ;-)  What are the problems?

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Daniel Jacobowitz]

On Sat, Feb 17, 2007 at 08:51:28PM +0100, Eric Botcazou wrote:
> > Does this mean I can sucker you into making it work at -O0? :-)
> 
> So it doesn't work at -O0? ;-)  What are the problems?

I don't know - maybe I'm wrong?  I was just going by a memory
from when it was committed.  It does appear to do _something_ at -O0.
Specifically, if I turn it on, a breakpoint on the very first
instruction of main can print argc / argv.  But if I don't, then I get
garbage until we reach their frame slots.

If it really works already, I apologize for not raising this issue
long ago: I think it should be on by default.  Someone will have to
measure the size impact for some less trivial code at -O0.  If no one
else does, I'll try to do this next week.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

[Adding Joel to the loop]

> I don't know - maybe I'm wrong?  I was just going by a memory
> from when it was committed.  It does appear to do _something_ at -O0.
> Specifically, if I turn it on, a breakpoint on the very first
> instruction of main can print argc / argv.  But if I don't, then I get
> garbage until we reach their frame slots.

Yes, we're seeing a similar effect locally with parameters referenced in 
nested functions.

> If it really works already, I apologize for not raising this issue
> long ago: I think it should be on by default.  Someone will have to
> measure the size impact for some less trivial code at -O0.  If no one
> else does, I'll try to do this next week.

Joel conducted an experiment with our 4.1-based compiler on a 1.5M SLOC code 
and he measured a 3% size increase for the final executable.

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Daniel Jacobowitz]

On Wed, Feb 21, 2007 at 10:01:02AM +0100, Eric Botcazou wrote:
> > If it really works already, I apologize for not raising this issue
> > long ago: I think it should be on by default.  Someone will have to
> > measure the size impact for some less trivial code at -O0.  If no one
> > else does, I'll try to do this next week.
> 
> Joel conducted an experiment with our 4.1-based compiler on a 1.5M SLOC code 
> and he measured a 3% size increase for the final executable.

Thanks.  Well, who gets to make this decision?  I think the time /
size impact is reasonable to improve the quality of debugging.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Improve debug info for Ada (4/4)

[Richard Henderson]

On Wed, Feb 21, 2007 at 06:47:56AM -0500, Daniel Jacobowitz wrote:
> Thanks.  Well, who gets to make this decision?  I think the time /
> size impact is reasonable to improve the quality of debugging.

I'm for it.


r~

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> > Thanks.  Well, who gets to make this decision?  I think the time /
> > size impact is reasonable to improve the quality of debugging.
>
> I'm for it.

OK.  However Joel has identified some problems triggered by enabling 
-fvar-tracking at -O0 with our compiler, let us investigate them first.

-- 
Eric Botcazou

Re: [PATCH] Improve debug info for Ada (4/4)

[Eric Botcazou]

> OK.  However Joel has identified some problems triggered by enabling
> -fvar-tracking at -O0 with our compiler, let us investigate them first.

The investigation revealed that it somewhat damages the "set" command of the 
debugger, mainly because of the MO_COPY micro-operation introduced in
  http://gcc.gnu.org/ml/gcc-patches/2006-07/msg00975.html

Disabling this MO_COPY micro-operation restores proper functioning for "set", 
at the expense of a surge for the debug info size (as reported by Alexandre).
Joel measured 16.1% and 8.2% on x86-64 and x86 respectively for the total size 
of the executable.

Moreover, even with this fat debug info, we have regressions of the form 
<value optimized out> in the debugger, what we agree with Joel to deem not 
acceptable at -O0.  And I'm also a little concerned about the increase in 
compilation time.

To sum up, I think that the downsides outweigh the advantages so I wouldn't 
recommend enabling -fvar-tracking at -O0.  I'm investigating emitting a few 
NOTE_INSN_VAR_LOCATION notes manually in the RTL when we really need them.

-- 
Eric Botcazou