Re: [PATCH][1/n] dwarf2out refactoring for early (LTO) debug

[Richard Biener <rguenther at suse dot de>]

On Wed, 19 Aug 2015, Richard Biener wrote:

> On Tue, 18 Aug 2015, Aldy Hernandez wrote:
> 
> > On 08/18/2015 07:20 AM, Richard Biener wrote:
> > > 
> > > This starts a series of patches (still in development) to refactor
> > > dwarf2out.c to better cope with early debug (and LTO debug).
> > 
> > Awesome!  Thanks.
> > 
> > > Aldyh, what other testing did you usually do for changes?  Run
> > > the gdb testsuite against the new compiler?  Anything else?
> > 
> > gdb testsuite, and make sure you test GCC with --enable-languages=all,go,ada,
> > though the latter is mostly useful while you iron out bugs initially.  I found
> > that ultimately, the best test was C++.
> 
> I see.
> 
> > Pre merge I also bootstrapped the compiler and compared .debug* section sizes
> > in object files to make sure things were within reason.
> > 
> > > +
> > > +static void
> > > +vmsdbgout_early_finish (const char *filename ATTRIBUTE_UNUSED)
> > > +{
> > > +  if (write_symbols == VMS_AND_DWARF2_DEBUG)
> > > +    (*dwarf2_debug_hooks.early_finish) (filename);
> > > +}
> > 
> > You can get rid of ATTRIBUTE_UNUSED now.
> 
> Done.  I've also refrained from moving
> 
>   gen_scheduled_generic_parms_dies ();
>   gen_remaining_tmpl_value_param_die_attribute ();
> 
> for now as that causes regressions I have to investigate.
> 
> The patch below has passed bootstrap & regtest on x86_64-unknown-linux-gnu
> as well as gdb testing.  Twice unpatched, twice patched - results seem
> to be somewhat unstable!?  I even refrained from using any -j with
> make check-gdb...  maybe it's just contrib/test_summary not coping well
> with gdb?  any hints?  Difference between unpatched run 1 & 2 is
> for example
> 
> --- results.unpatched   2015-08-19 15:08:36.152899926 +0200
> +++ results.unpatched2  2015-08-19 15:29:46.902060797 +0200
> @@ -209,7 +209,6 @@
>  WARNING: remote_expect statement without a default case?!
>  WARNING: remote_expect statement without a default case?!
>  WARNING: remote_expect statement without a default case?!
> -FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, 
> fc4)
>  FAIL: gdb.cp/inherit.exp: print g_vD
>  FAIL: gdb.cp/inherit.exp: print g_vE
>  FAIL: gdb.cp/no-dmgl-verbose.exp: setting breakpoint at 'f(std::string)'
> @@ -238,6 +237,7 @@
>  UNRESOLVED: gdb.fortran/types.exp: set print sevenbit-strings
>  FAIL: gdb.fortran/whatis_type.exp: run to MAIN__
>  WARNING: remote_expect statement without a default case?!
> +FAIL: gdb.gdb/complaints.exp: print symfile_complaints->root->fmt
>  WARNING: remote_expect statement without a default case?!
>  WARNING: remote_expect statement without a default case?!
>  WARNING: remote_expect statement without a default case?!
> @@ -362,12 +362,12 @@
>                 === gdb Summary ===
>  
> -# of expected passes           30881
> +# of expected passes           30884
>  # of unexpected failures       284
>  # of unexpected successes      2
> -# of expected failures         85
> +# of expected failures         83
>  # of unknown successes         2
> -# of known failures            60
> +# of known failures            59
>  # of unresolved testcases      6
>  # of untested testcases                32
>  # of unsupported tests         165
> 
> the sames changes randomly appear/disappear in the patched case.  
> Otherwise patched/unpatched agree.
> 
> Ok?

Jason, are you willing to review these refactoring patches or can
I invoke my middle-end maintainer powers to remove some of this noise
from the LTO parts?

Thanks,
Richard.

> Thanks,
> Richard.
> 
> 2015-08-18  Richard Biener  <rguenther@suse.de>
> 
> 	* debug.h (gcc_debug_hooks::early_finish): Add filename argument.
> 	* dbxout.c (dbx_debug_hooks): Adjust.
> 	* debug.c (do_nothing_hooks): Likewise.
> 	* sdbout.c (sdb_debug_hooks): Likewise.
> 	* vmsdbgout.c (vmsdbgout_early_finish): New function dispatching
> 	to dwarf2out variant if needed.
> 	(vmsdbg_debug_hooks): Adjust.
> 	* dwarf2out.c (dwarf2_line_hooks): Adjust.
> 	(flush_limbo_die_list): New function.
> 	(dwarf2out_finish): Call flush_limbo_die_list instead of
> 	dwarf2out_early_finish.  Assert there are no deferred asm-names.
> 	Move early stuff ...
> 	(dwarf2out_early_finish): ... here.
> 	* cgraphunit.c (symbol_table::finalize_compilation_unit):
> 	Call early_finish with main_input_filename argument.
> 
> 
> Index: gcc/cgraphunit.c
> ===================================================================
> --- gcc/cgraphunit.c	(revision 226966)
> +++ gcc/cgraphunit.c	(working copy)
> @@ -2490,7 +2490,7 @@ symbol_table::finalize_compilation_unit
>  
>    /* Clean up anything that needs cleaning up after initial debug
>       generation.  */
> -  (*debug_hooks->early_finish) ();
> +  (*debug_hooks->early_finish) (main_input_filename);
>  
>    /* Finally drive the pass manager.  */
>    compile ();
> Index: gcc/dbxout.c
> ===================================================================
> --- gcc/dbxout.c	(revision 226966)
> +++ gcc/dbxout.c	(working copy)
> @@ -354,7 +354,7 @@ const struct gcc_debug_hooks dbx_debug_h
>  {
>    dbxout_init,
>    dbxout_finish,
> -  debug_nothing_void,
> +  debug_nothing_charstar,
>    debug_nothing_void,
>    debug_nothing_int_charstar,
>    debug_nothing_int_charstar,
> Index: gcc/debug.c
> ===================================================================
> --- gcc/debug.c	(revision 226966)
> +++ gcc/debug.c	(working copy)
> @@ -28,7 +28,7 @@ const struct gcc_debug_hooks do_nothing_
>  {
>    debug_nothing_charstar,
>    debug_nothing_charstar,
> -  debug_nothing_void,			/* early_finish */
> +  debug_nothing_charstar,			/* early_finish */
>    debug_nothing_void,
>    debug_nothing_int_charstar,
>    debug_nothing_int_charstar,
> Index: gcc/debug.h
> ===================================================================
> --- gcc/debug.h	(revision 226966)
> +++ gcc/debug.h	(working copy)
> @@ -31,7 +31,7 @@ struct gcc_debug_hooks
>    void (* finish) (const char *main_filename);
>  
>    /* Run cleanups necessary after early debug generation.  */
> -  void (* early_finish) (void);
> +  void (* early_finish) (const char *main_filename);
>  
>    /* Called from cgraph_optimize before starting to assemble
>       functions/variables/toplevel asms.  */
> Index: gcc/dwarf2out.c
> ===================================================================
> --- gcc/dwarf2out.c	(revision 226966)
> +++ gcc/dwarf2out.c	(working copy)
> @@ -2420,7 +2420,7 @@ build_cfa_aligned_loc (dw_cfa_location *
>  
>  static void dwarf2out_init (const char *);
>  static void dwarf2out_finish (const char *);
> -static void dwarf2out_early_finish (void);
> +static void dwarf2out_early_finish (const char *);
>  static void dwarf2out_assembly_start (void);
>  static void dwarf2out_define (unsigned int, const char *);
>  static void dwarf2out_undef (unsigned int, const char *);
> @@ -2494,7 +2494,7 @@ const struct gcc_debug_hooks dwarf2_line
>  {
>    dwarf2out_init,
>    debug_nothing_charstar,
> -  debug_nothing_void,
> +  debug_nothing_charstar,
>    debug_nothing_void,
>    debug_nothing_int_charstar,
>    debug_nothing_int_charstar,
> @@ -25128,47 +25128,81 @@ optimize_location_lists (dw_die_ref die)
>    optimize_location_lists_1 (die, &htab);
>  }
>  
> +/* Traverse the limbo die list, and add parent/child links.  The only
> +   dies without parents that should be here are concrete instances of
> +   inline functions, and the comp_unit_die.  We can ignore the comp_unit_die.
> +   For concrete instances, we can get the parent die from the abstract
> +   instance.  */
> +
> +static void
> +flush_limbo_die_list (void)
> +{
> +  limbo_die_node *node, *next_node;
> +
> +  for (node = limbo_die_list; node; node = next_node)
> +    {
> +      dw_die_ref die = node->die;
> +      next_node = node->next;
> +
> +      if (die->die_parent == NULL)
> +	{
> +	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
> +
> +	  if (origin && origin->die_parent)
> +	    add_child_die (origin->die_parent, die);
> +	  else if (is_cu_die (die))
> +	    ;
> +	  else if (seen_error ())
> +	    /* It's OK to be confused by errors in the input.  */
> +	    add_child_die (comp_unit_die (), die);
> +	  else
> +	    {
> +	      /* In certain situations, the lexical block containing a
> +		 nested function can be optimized away, which results
> +		 in the nested function die being orphaned.  Likewise
> +		 with the return type of that nested function.  Force
> +		 this to be a child of the containing function.
> +
> +		 It may happen that even the containing function got fully
> +		 inlined and optimized out.  In that case we are lost and
> +		 assign the empty child.  This should not be big issue as
> +		 the function is likely unreachable too.  */
> +	      gcc_assert (node->created_for);
> +
> +	      if (DECL_P (node->created_for))
> +		origin = get_context_die (DECL_CONTEXT (node->created_for));
> +	      else if (TYPE_P (node->created_for))
> +		origin = scope_die_for (node->created_for, comp_unit_die ());
> +	      else
> +		origin = comp_unit_die ();
> +
> +	      add_child_die (origin, die);
> +	    }
> +	}
> +    }
> +
> +  limbo_die_list = NULL;
> +}
> +
>  /* Output stuff that dwarf requires at the end of every file,
>     and generate the DWARF-2 debugging info.  */
>  
>  static void
> -dwarf2out_finish (const char *filename)
> +dwarf2out_finish (const char *)
>  {
>    comdat_type_node *ctnode;
>    dw_die_ref main_comp_unit_die;
>  
>    /* Flush out any latecomers to the limbo party.  */
> -  dwarf2out_early_finish ();
> +  flush_limbo_die_list ();
>  
> -  /* PCH might result in DW_AT_producer string being restored from the
> -     header compilation, so always fill it with empty string initially
> -     and overwrite only here.  */
> -  dw_attr_ref producer = get_AT (comp_unit_die (), DW_AT_producer);
> -  producer_string = gen_producer_string ();
> -  producer->dw_attr_val.v.val_str->refcount--;
> -  producer->dw_attr_val.v.val_str = find_AT_string (producer_string);
> +  /* We shouldn't have any symbols with delayed asm names for
> +     DIEs generated after early finish.  */
> +  gcc_assert (deferred_asm_name == NULL);
>  
>    gen_scheduled_generic_parms_dies ();
>    gen_remaining_tmpl_value_param_die_attribute ();
>  
> -  /* Add the name for the main input file now.  We delayed this from
> -     dwarf2out_init to avoid complications with PCH.
> -     For LTO produced units use a fixed artificial name to avoid
> -     leaking tempfile names into the dwarf.  */
> -  if (!in_lto_p)
> -    add_name_attribute (comp_unit_die (), remap_debug_filename (filename));
> -  else
> -    add_name_attribute (comp_unit_die (), "<artificial>");
> -  if (!IS_ABSOLUTE_PATH (filename) || targetm.force_at_comp_dir)
> -    add_comp_dir_attribute (comp_unit_die ());
> -  else if (get_AT (comp_unit_die (), DW_AT_comp_dir) == NULL)
> -    {
> -      bool p = false;
> -      file_table->traverse<bool *, file_table_relative_p> (&p);
> -      if (p)
> -	add_comp_dir_attribute (comp_unit_die ());
> -    }
> -
>  #if ENABLE_ASSERT_CHECKING
>    {
>      dw_die_ref die = comp_unit_die (), c;
> @@ -25482,9 +25516,9 @@ dwarf2out_finish (const char *filename)
>     has run.  */
>  
>  static void
> -dwarf2out_early_finish (void)
> +dwarf2out_early_finish (const char *filename)
>  {
> -  limbo_die_node *node, *next_node;
> +  limbo_die_node *node;
>  
>    /* Add DW_AT_linkage_name for all deferred DIEs.  */
>    for (node = deferred_asm_name; node; node = node->next)
> @@ -25502,57 +25536,35 @@ dwarf2out_early_finish (void)
>      }
>    deferred_asm_name = NULL;
>  
> -  /* Traverse the limbo die list, and add parent/child links.  The only
> -     dies without parents that should be here are concrete instances of
> -     inline functions, and the comp_unit_die.  We can ignore the comp_unit_die.
> -     For concrete instances, we can get the parent die from the abstract
> -     instance.
> -
> -     The point here is to flush out the limbo list so that it is empty
> +  /* The point here is to flush out the limbo list so that it is empty
>       and we don't need to stream it for LTO.  */
> -  for (node = limbo_die_list; node; node = next_node)
> -    {
> -      dw_die_ref die = node->die;
> -      next_node = node->next;
> -
> -      if (die->die_parent == NULL)
> -	{
> -	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
> -
> -	  if (origin && origin->die_parent)
> -	    add_child_die (origin->die_parent, die);
> -	  else if (is_cu_die (die))
> -	    ;
> -	  else if (seen_error ())
> -	    /* It's OK to be confused by errors in the input.  */
> -	    add_child_die (comp_unit_die (), die);
> -	  else
> -	    {
> -	      /* In certain situations, the lexical block containing a
> -		 nested function can be optimized away, which results
> -		 in the nested function die being orphaned.  Likewise
> -		 with the return type of that nested function.  Force
> -		 this to be a child of the containing function.
> +  flush_limbo_die_list ();
>  
> -		 It may happen that even the containing function got fully
> -		 inlined and optimized out.  In that case we are lost and
> -		 assign the empty child.  This should not be big issue as
> -		 the function is likely unreachable too.  */
> -	      gcc_assert (node->created_for);
> -
> -	      if (DECL_P (node->created_for))
> -		origin = get_context_die (DECL_CONTEXT (node->created_for));
> -	      else if (TYPE_P (node->created_for))
> -		origin = scope_die_for (node->created_for, comp_unit_die ());
> -	      else
> -		origin = comp_unit_die ();
> +  /* PCH might result in DW_AT_producer string being restored from the
> +     header compilation, so always fill it with empty string initially
> +     and overwrite only here.  */
> +  dw_attr_ref producer = get_AT (comp_unit_die (), DW_AT_producer);
> +  producer_string = gen_producer_string ();
> +  producer->dw_attr_val.v.val_str->refcount--;
> +  producer->dw_attr_val.v.val_str = find_AT_string (producer_string);
>  
> -	      add_child_die (origin, die);
> -	    }
> -	}
> +  /* Add the name for the main input file now.  We delayed this from
> +     dwarf2out_init to avoid complications with PCH.
> +     For LTO produced units use a fixed artificial name to avoid
> +     leaking tempfile names into the dwarf.  */
> +  if (!in_lto_p)
> +    add_name_attribute (comp_unit_die (), remap_debug_filename (filename));
> +  else
> +    add_name_attribute (comp_unit_die (), "<artificial>");
> +  if (!IS_ABSOLUTE_PATH (filename) || targetm.force_at_comp_dir)
> +    add_comp_dir_attribute (comp_unit_die ());
> +  else if (get_AT (comp_unit_die (), DW_AT_comp_dir) == NULL)
> +    {
> +      bool p = false;
> +      file_table->traverse<bool *, file_table_relative_p> (&p);
> +      if (p)
> +	add_comp_dir_attribute (comp_unit_die ());
>      }
> -
> -  limbo_die_list = NULL;
>  }
>  
>  /* Reset all state within dwarf2out.c so that we can rerun the compiler
> Index: gcc/sdbout.c
> ===================================================================
> --- gcc/sdbout.c	(revision 226966)
> +++ gcc/sdbout.c	(working copy)
> @@ -278,7 +278,7 @@ const struct gcc_debug_hooks sdb_debug_h
>  {
>    sdbout_init,			         /* init */
>    sdbout_finish,		         /* finish */
> -  debug_nothing_void,			 /* early_finish */
> +  debug_nothing_charstar,		 /* early_finish */
>    debug_nothing_void,			 /* assembly_start */
>    debug_nothing_int_charstar,	         /* define */
>    debug_nothing_int_charstar,	         /* undef */
> Index: gcc/vmsdbgout.c
> ===================================================================
> --- gcc/vmsdbgout.c	(revision 226966)
> +++ gcc/vmsdbgout.c	(working copy)
> @@ -147,6 +147,7 @@ static int write_srccorrs (int);
>  
>  static void vmsdbgout_init (const char *);
>  static void vmsdbgout_finish (const char *);
> +static void vmsdbgout_early_finish (const char *);
>  static void vmsdbgout_assembly_start (void);
>  static void vmsdbgout_define (unsigned int, const char *);
>  static void vmsdbgout_undef (unsigned int, const char *);
> @@ -174,7 +175,7 @@ static void vmsdbgout_abstract_function
>  const struct gcc_debug_hooks vmsdbg_debug_hooks
>  = {vmsdbgout_init,
>     vmsdbgout_finish,
> -   debug_nothing_void,
> +   vmsdbgout_early_finish,
>     vmsdbgout_assembly_start,
>     vmsdbgout_define,
>     vmsdbgout_undef,
> @@ -1552,7 +1553,17 @@ vmsdbgout_abstract_function (tree decl)
>     VMS Debug debugging info.  */
>  
>  static void
> -vmsdbgout_finish (const char *filename ATTRIBUTE_UNUSED)
> +vmsdbgout_early_finish (const char *filename)
> +{
> +  if (write_symbols == VMS_AND_DWARF2_DEBUG)
> +    (*dwarf2_debug_hooks.early_finish) (filename);
> +}
> +
> +/* Output stuff that Debug requires at the end of every file and generate the
> +   VMS Debug debugging info.  */
> +
> +static void
> +vmsdbgout_finish (const char *filename)
>  {
>    unsigned int i, ifunc;
>    int totsize;
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)