[debug-early] do not add location info/etc to abstract instances
diff mbox

Message ID 542AF9CF.2070208@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Sept. 30, 2014, 6:43 p.m. UTC
Hi Jason.

As discussed on IRC, DIEs of abstract instances of functions (those 
tagged with DW_AT_inline), cannot include information that would be 
different between an abstract inline and an out-of-line copy.  This, as 
well as (seeming) gdb snafus regarding abstract origins, was the reason 
I was seeing less guality regressions on my branch.

With the current patch, not only do we fix this oversight, but are now 
at feature *and* bug parity with mainline.  I guess that's good :(.

There were a few problems that needed fixing.  First, 
gen_formal_parameter_die() was reusing the abstract instance DIE's 
parameter, instead of creating a new die with abstract origin set. 
Also, gen_formal_parameter_die() had some old Michael code, working 
around decl_ultimate_origin hacks.  I've fixed all of these problems.

I also fixed gen_subprogram_die(), so it's more aware of a previously 
generated die.

I would appreciate if you could take a look at this patch, particularly 
at the gen_formal_parameter_die change, since I'm not sure what the 
proper way is of checking that a parameter belongs to an abstract 
instance.  Below is what I'm using:

>    bool reusing_die;
> -  if (parm_die)
> +  if (parm_die
> +      /* Make sure the function to which this parameter belongs to is
> +	 not an abstract instance.  If it is, we can't reuse anything.
> +	 We must create a new DW_TAG_formal_parameter with a
> +	 corresponding DW_AT_abstract_origin.  */
> +      && !get_AT (context_die, DW_AT_abstract_origin))
>      reusing_die = true;

Also, seeing as my changes could potentially render invalid DIEs, I 
thought it best to add, at the very least, a check for the inline 
problem described above (see check_die_inline).  For that matter, I 
wonder if we could add more checks to check_die() to make it a general 
dwarf DIE sanity checker.  It still amazes me that you dwarf hackers do 
all this magic, without any sort of checks.

And finally, making changes to _when_ we generate DIEs can sometimes 
lead to NOT generating DIEs early, and silently behaving like mainline 
(that is, generating everything at the end of compilation).  To solve 
this problem, which I'm sure I'll stumble into, I've added 
-fdump-early-debug-stats which will set the ->dumped_early bit in every 
DIE generated after parsing, and then dumping the DIEs after the late 
dwarf generation has run.  This way I can see if we have too many DIEs 
that were NOT generated early.  It is likely this will only be a 
temporary hacking tool to check I didn't do anything stupid along :).

How does this look?

Aldy
commit d9b215d7c3aeea2c601e0d983cbe424990c1beab
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Sep 30 08:20:44 2014 -0700

    	* common.opt (fdump-early-debug-stats): New.
    	* debug.h (dwarf2out_mark_early_dies): New prototype.
    	(dwarf2out_dump_early_debug_stats): New prototype.
    	* toplev.c (compile_file): Dump early debug stats if requested.
    	* dwarf2out.c (check_die): Check that DIEs containing a
    	DW_AT_inline doe not contain any invalid modifiers.
    	(gen_formal_parameter_die): Do not reuse parameters that belong to
    	an abstract instance.
    	Do not care that an abstract origin is itself.
    	(gen_subprogram_die): Handle old_die's better.
    	(print_die): Print dumped_early bit.
    	(mark_early_dies_helper): New.
    	(dwarf2out_mark_early_dies): New.
    	(dwarf2out_dump_early_debug_stats): New.
    	(check_die_inline): New.
    	(check_die): Call check_die_inline.

Comments

Jason Merrill Oct. 2, 2014, 3:53 p.m. UTC | #1
On 09/30/2014 02:43 PM, Aldy Hernandez wrote:
> +  if (parm_die
> +      /* Make sure the function to which this parameter belongs to is
> +	 not an abstract instance.  If it is, we can't reuse anything.
> +	 We must create a new DW_TAG_formal_parameter with a
> +	 corresponding DW_AT_abstract_origin.  */
> +      && !get_AT (context_die, DW_AT_abstract_origin))

Can we use the same test here that we do later in this function, namely 
origin && origin != node?

> -  if (origin != NULL && origin != decl)
> +  if (origin != NULL && (origin != decl || old_die))

Don't we want to go back to "if (origin != NULL)" here too?

Jason
Jason Merrill Oct. 2, 2014, 3:54 p.m. UTC | #2
On 10/02/2014 11:53 AM, Jason Merrill wrote:
> On 09/30/2014 02:43 PM, Aldy Hernandez wrote:
>> +  if (parm_die
>> +      /* Make sure the function to which this parameter belongs to is
>> +     not an abstract instance.  If it is, we can't reuse anything.
>> +     We must create a new DW_TAG_formal_parameter with a
>> +     corresponding DW_AT_abstract_origin.  */
>> +      && !get_AT (context_die, DW_AT_abstract_origin))
>
> Can we use the same test here that we do later in this function, namely
> origin && origin != node?

Or, rather, just "if (origin)".

Jason

Patch
diff mbox

diff --git a/gcc/common.opt b/gcc/common.opt
index 634a72b..c01f935 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1132,6 +1132,10 @@  fdump-unnumbered-links
 Common Report Var(flag_dump_unnumbered_links)
 Suppress output of previous and next insn numbers in debugging dumps
 
+fdump-early-debug-stats
+Common Report Var(flag_dump_early_debug_stats)
+Dump all dwarf DIEs, specifying if they were generated during the early debug stage
+
 fdwarf2-cfi-asm
 Common Report Var(flag_dwarf2_cfi_asm) Init(HAVE_GAS_CFI_DIRECTIVE)
 Enable CFI tables via GAS assembler directives.
diff --git a/gcc/debug.h b/gcc/debug.h
index ec387ca..7158a48 100644
--- a/gcc/debug.h
+++ b/gcc/debug.h
@@ -190,11 +190,12 @@  extern bool dwarf2out_do_frame (void);
 extern bool dwarf2out_do_cfi_asm (void);
 extern void dwarf2out_switch_text_section (void);
 
+extern void dwarf2out_mark_early_dies (void);
+extern void dwarf2out_dump_early_debug_stats (void);
+
 const char *remap_debug_filename (const char *);
 void add_debug_prefix_map (const char *);
 
-extern void dwarf2out_early_decl (tree);
-
 /* For -fdump-go-spec.  */
 
 extern const struct gcc_debug_hooks *
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index c92101f..a713435 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -5358,9 +5358,9 @@  print_die (dw_die_ref die, FILE *outfile)
   unsigned ix;
 
   print_spaces (outfile);
-  fprintf (outfile, "DIE %4ld: %s (%p)\n",
+  fprintf (outfile, "DIE %4ld: %s (%p)%s\n",
 	   die->die_offset, dwarf_tag_name (die->die_tag),
-	   (void*) die);
+	   (void*) die, die->dumped_early ? " (DUMPED EARLY)" : "");
   print_spaces (outfile);
   fprintf (outfile, "  abbrev id: %lu", die->die_abbrev);
   fprintf (outfile, " offset: %ld", die->die_offset);
@@ -5528,6 +5528,71 @@  debug_dwarf (void)
   print_die (comp_unit_die (), stderr);
 }
 
+/* Callback for htab_traverse_noresize.  Set the dumped_early bit for
+   a given DIE.  */
+
+static int
+mark_early_dies_helper (void **slot, void *info ATTRIBUTE_UNUSED)
+{
+  dw_die_ref ref = (dw_die_ref) *slot;
+
+  /* Unilaterally enabling this bit can potentially change the
+     behavior of dwarf2out_decl for DECLs that were discovered through
+     recursion of dwarf2out_decl(), and may not have the dumped_early
+     bit set.  Since this is only used for debugging the behavior of
+     dwarf2out, we should be ok-- but it's something to keep in
+     mind.  */
+  ref->dumped_early = true;
+  return 1;
+}
+
+/* Set the dumped_early bit for all DIEs.  */
+
+void
+dwarf2out_mark_early_dies (void)
+{
+  if (decl_die_table)
+    htab_traverse_noresize (decl_die_table, mark_early_dies_helper, NULL);
+}
+
+/* Dump the DIE table if available.  */
+
+void
+dwarf2out_dump_early_debug_stats (void)
+{
+  if (decl_die_table)
+    debug_dwarf ();
+}
+
+/* Sanity checks on DW_AT_inline DIEs.  */
+
+static void
+check_die_inline (dw_die_ref die)
+{
+  /* A debugging information entry that is a member of an abstract
+     instance tree [that has DW_AT_inline] should not contain any
+     attributes which describe aspects of the subroutine which vary
+     between distinct inlined expansions or distinct out-of-line
+     expansions.  */
+  unsigned ix;
+  dw_attr_ref a;
+  bool inline_found = false;
+  FOR_EACH_VEC_SAFE_ELT (die->die_attr, ix, a)
+    if (a->dw_attr == DW_AT_inline)
+      inline_found = true;
+  if (inline_found)
+    {
+      /* Catch the most common mistakes.  */
+      FOR_EACH_VEC_SAFE_ELT (die->die_attr, ix, a)
+	gcc_assert (a->dw_attr != DW_AT_low_pc
+		    && a->dw_attr != DW_AT_high_pc
+		    && a->dw_attr != DW_AT_location
+		    && a->dw_attr != DW_AT_frame_base
+		    && a->dw_attr != DW_AT_GNU_all_call_sites);
+    }
+
+}
+
 /* Perform some sanity checks on DIEs after they have been generated
    earlier in the compilation process.  */
 
@@ -5536,7 +5601,10 @@  check_die (dw_die_ref die, unsigned level)
 {
   static unsigned long mark = 1;
   dw_die_ref c, p;
-  /* Check that all our childs have their parent set to us.  */
+
+  check_die_inline (die);
+
+  /* Check that all of our children have their parent set to us.  */
   c = die->die_child;
   if (c) do {
       c = c->die_sib;
@@ -17721,7 +17789,12 @@  gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
     }
 
   bool reusing_die;
-  if (parm_die)
+  if (parm_die
+      /* Make sure the function to which this parameter belongs to is
+	 not an abstract instance.  If it is, we can't reuse anything.
+	 We must create a new DW_TAG_formal_parameter with a
+	 corresponding DW_AT_abstract_origin.  */
+      && !get_AT (context_die, DW_AT_abstract_origin))
     reusing_die = true;
   else
     {
@@ -17739,7 +17812,7 @@  gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
       if (reusing_die)
 	goto add_location;
 
-      if (origin != NULL && node != origin)
+      if (origin != NULL)
 	add_abstract_origin_attribute (parm_die, origin);
       else if (emit_name_p)
 	add_name_and_src_coords_attributes (parm_die, node);
@@ -18278,7 +18351,7 @@  gen_subprogram_die (tree decl, dw_die_ref context_die)
       && debug_info_level > DINFO_LEVEL_TERSE)
     old_die = force_decl_die (decl);
 
-  if (origin != NULL && origin != decl)
+  if (origin != NULL && (origin != decl || old_die))
     {
       gcc_assert (!declaration || local_scope_p (context_die));
 
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 9f29dac..7dca017 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -573,6 +573,10 @@  compile_file (void)
   /* Run the actual compilation process.  */
   if (!in_lto_p)
     {
+      /* Mark all DIEs generated as dumped early.  */
+      if (flag_dump_early_debug_stats)
+	dwarf2out_mark_early_dies ();
+
       timevar_start (TV_PHASE_OPT_GEN);
       symtab->finalize_compilation_unit ();
       timevar_stop (TV_PHASE_OPT_GEN);
@@ -597,6 +601,9 @@  compile_file (void)
     debug_hooks->late_global_decl (node->decl);
   timevar_stop (TV_PHASE_DBGINFO);
 
+  if (!in_lto_p && flag_dump_early_debug_stats)
+    dwarf2out_dump_early_debug_stats ();
+
   timevar_start (TV_PHASE_LATE_ASM);
 
   /* Compilation unit is finalized.  When producing non-fat LTO object, we are