Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva <aoliva@redhat.com>]

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

On Feb 26, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:

> So far, all the differences I looked at were caused by padding at the
> end of BBs, and by jump stmts without line numbers at the end of BBs,
> both right after the debug reset stmts the proposed patch introduces.

Further investigation showed there were other sources of spurious
differences:

- copies arising from the expansion of PHI nodes; source code
  information associated with these copies points at the source of the
  copy, which is hardly useful and sensible.

- "optimization" of single-range location lists to a location expression
  covering the entire function, which extends, often incorrectly, the
  range of the expression, and thus the coverage of the function.

By patching GCC so as to eliminate these differences (patches attached
for reference), I managed to reduce the coverage differences in
libgcc_s.so to a manageable size (about a dozen variables in 3 functions
in 2 object files), so I could manually investigate each one of them.

All remaining differences amounted to single insns, originally before
the reset debug stmt introduced by the patch, computing subexpressions
of expressions expanded after the return point of the inlined function.
Because of TER, this often causes stmts to be moved past corresponding
debug stmts.  This is a long-standing and long-known issue to me, thus
nothing particularly new brought about or worsened by the proposed
patch, and that would be fixed with stmt frontiers.

Ultimately, the issue is not so much that the insn gets emitted at a
different spot, but rather the possibility that no insn will remain at
the point where it was expected, which might make it impossible (with
current compiler and debugger) to inspect the results of that
computation.  The worst the reset debug stmts introduced by this patch
could do is to make those effects not visible at other points of the
computation, where they are not guaranteed or expected to be visible to
begin with.

That said, it might be nice if we emitted partial of full expansions of
subexpressions at their original location, relative to debug stmts
expanded to debug insns, rather than all at the point of the full
expression.  I recall having started something along these lines years
ago, but that project never got to a working state.


The differences in libstdc++.so, even after the patches intended to
minimize differences, are too many to investigate in depth, but from
what I can tell from a quick glance at the diff in dwlocstat
per-variable per-range coverage dumps, nearly all of the differences
amount to one insn in the final range, which likely means TER-introduced
differences.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-end-scopes-before-locless-insns.patch --]
[-- Type: text/x-patch, Size: 2714 bytes --]

End scopes before location-less insns

From: Alexandre Oliva <aoliva@redhat.com>


---
 gcc/final.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/gcc/final.c b/gcc/final.c
index 1fa93d9..6fab871 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -1634,13 +1634,19 @@ choose_inner_scope (tree s1, tree s2)
 /* Emit lexical block notes needed to change scope from S1 to S2.  */
 
 static void
-change_scope (rtx_insn *orig_insn, tree s1, tree s2)
+change_scope (rtx_insn *prev_insn, rtx_insn *orig_insn, tree s1, tree s2)
 {
   rtx_insn *insn = orig_insn;
+  rtx_insn *einsn;
   tree com = NULL_TREE;
   tree ts1 = s1, ts2 = s2;
   tree s;
 
+  if (!prev_insn)
+    einsn = insn;
+  else
+    einsn = NEXT_INSN (prev_insn);
+
   while (ts1 != ts2)
     {
       gcc_assert (ts1 && ts2);
@@ -1660,7 +1666,7 @@ change_scope (rtx_insn *orig_insn, tree s1, tree s2)
   s = s1;
   while (s != com)
     {
-      rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, insn);
+      rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, einsn);
       NOTE_BLOCK (note) = s;
       s = BLOCK_SUPERCONTEXT (s);
     }
@@ -1684,6 +1690,7 @@ reemit_insn_block_notes (void)
   tree cur_block = DECL_INITIAL (cfun->decl);
   rtx_insn *insn;
   rtx_note *note;
+  rtx_insn *prev_insn = NULL;
 
   insn = get_insns ();
   for (; insn; insn = NEXT_INSN (insn))
@@ -1693,10 +1700,16 @@ reemit_insn_block_notes (void)
       /* Prevent lexical blocks from straddling section boundaries.  */
       if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
         {
+	  rtx_insn *einsn;
+	  if (prev_insn)
+	    einsn = NEXT_INSN (prev_insn);
+	  else
+	    einsn = insn;
+	  prev_insn = NULL;
           for (tree s = cur_block; s != DECL_INITIAL (cfun->decl);
                s = BLOCK_SUPERCONTEXT (s))
             {
-              rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, insn);
+              rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, einsn);
               NOTE_BLOCK (note) = s;
               note = emit_note_after (NOTE_INSN_BLOCK_BEG, insn);
               NOTE_BLOCK (note) = s;
@@ -1732,14 +1745,16 @@ reemit_insn_block_notes (void)
 
       if (this_block != cur_block)
 	{
-	  change_scope (insn, cur_block, this_block);
+	  change_scope (prev_insn, insn, cur_block, this_block);
 	  cur_block = this_block;
 	}
+
+      prev_insn = insn;
     }
 
   /* change_scope emits before the insn, not after.  */
   note = emit_note (NOTE_INSN_DELETED);
-  change_scope (note, cur_block, DECL_INITIAL (cfun->decl));
+  change_scope (NULL, note, cur_block, DECL_INITIAL (cfun->decl));
   delete_insn (note);
 
   reorder_blocks ();

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: vta-no-locs-for-out-of-ssa-copies.patch --]
[-- Type: text/x-patch, Size: 1606 bytes --]

Don't assign locations to out-of-ssa copies.

From: Alexandre Oliva <aoliva@redhat.com>


---
 gcc/tree-outof-ssa.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/tree-outof-ssa.c b/gcc/tree-outof-ssa.c
index e6310cd..7d15a9a 100644
--- a/gcc/tree-outof-ssa.c
+++ b/gcc/tree-outof-ssa.c
@@ -289,6 +289,7 @@ insert_partition_copy_on_edge (edge e, int dest, int src, source_location locus)
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   var = partition_to_var (SA.map, src);
   seq = emit_partition_copy (copy_rtx (SA.partition_to_pseudo[dest]),
@@ -327,6 +328,7 @@ insert_value_copy_on_edge (edge e, int dest, tree src, source_location locus)
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   start_sequence ();
 
@@ -382,6 +384,7 @@ insert_rtx_to_part_on_edge (edge e, int dest, rtx src, int unsignedsrcp,
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   /* We give the destination as sizeexp in case src/dest are BLKmode
      mems.  Usually we give the source.  As we result from SSA names
@@ -418,6 +421,7 @@ insert_part_to_rtx_on_edge (edge e, rtx dest, int src, source_location locus)
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   var = partition_to_var (SA.map, src);
   seq = emit_partition_copy (dest,

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: vta-no-single-loc-list.patch --]
[-- Type: text/x-patch, Size: 1070 bytes --]

do not optimize single-loc list

From: Alexandre Oliva <aoliva@redhat.com>


---
 gcc/dwarf2out.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index ebf41c8..308c070 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -14209,7 +14209,7 @@ dw_loc_list (var_loc_list *loc_list, tree decl, int want_address)
      representable, we don't want to pretend a single entry that was
      applies to the entire scope in which the variable is
      available.  */
-  if (list && loc_list->first->next)
+  if (list && (1 || loc_list->first->next))
     gen_llsym (list);
 
   return list;
@@ -16049,7 +16049,7 @@ add_location_or_const_value_attribute (dw_die_ref die, tree decl, bool cache_p,
      a constant value.  That way we are better to use add_const_value_attribute
      rather than expanding constant value equivalent.  */
   loc_list = lookup_decl_loc (decl);
-  if (loc_list
+  if (0 && loc_list
       && loc_list->first
       && loc_list->first->next == NULL
       && NOTE_P (loc_list->first->loc)

[-- Attachment #5: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer