Patchwork Better .debug_aranges fix (PR debug/48253)

login
register
mail settings
Submitter Jakub Jelinek
Date March 23, 2011, 8:08 p.m.
Message ID <20110323200829.GS18914@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/88111/
State New
Headers show

Comments

Jakub Jelinek - March 23, 2011, 8:08 p.m.
Hi!

As the testcases in the PR show (I think they are hardly suitable
for gcc testsuite though, better for gdb or elfutils testsuite),
we still don't cover all code in the CU with .debug_aranges (regression from
4.5) and with -freorder-blocks-and-partition we do even more bad things.

The problem are the new .text.startup/.text.exit sections, also
that __attribute__((cold)) functions can be placed as whole into
.text.unlikely, etc.

I think if we are emitting ranges for DW_TAG_compile_unit, then
.debug_ranges should cover exactly the same ranges, there is IMHO no need
to use a separate data structure to track that.  For the two
standard sections (text_section and cold_text_section) we just mark the
whole section contribution, for the rest we add each individual range.

There was another bug, in that DW_TAG_subprogram's DW_AT_ranges
could contain two ranges covering the same partition and none
covering the other partition, because fde->in_std_section was
a wrong check for the "was the section switch from hot to cold",
as say with -ffunction-section hot partition is say .text.f1
and thus not text_section.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk and 4.6.1?

2011-03-23  Jakub Jelinek  <jakub@redhat.com>

	PR debug/48253
	* dwarf2out.c (dwarf2out_switch_text_section): Update
	fde->in_std_section when switching from cold to hot section.
	(arange_table, arange_table_allocated, arange_table_in_use,
	ARANGE_TABLE_INCREMENT, add_arange): Removed.
	(size_of_aranges): Count !in_std_section and !cold_in_std_section
	hunks in fdes, instead of looking at arange_table_in_use.
	(output_aranges): Add aranges_length argument, don't call
	size_of_aranges here.  Instead of using aranges_table*
	emit ranges for fdes when !in_std_section resp.
	!cold_in_std_section.
	(gen_subprogram_die): Don't call add_arange.  Use
	!fde->dw_fde_switched_cold_to_hot instead of fde->in_std_section.
	(dwarf2out_begin_function): Initialize cold_text_section even
	when function_section () isn't text_section.
	(prune_unused_types): Don't walk arange_table.
	(dwarf2out_finish): Don't needlessly test
	flag_reorder_blocks_and_partition when testing cold_text_section_used.
	If info_section_emitted, call size_of_aranges and if it indicates
	non-empty .debug_aranges, call output_aranges with the computed
	size.


	Jakub
Jason Merrill - March 24, 2011, 5:08 p.m.
On 03/23/2011 09:08 PM, Jakub Jelinek wrote:
>   const char *dw_fde_begin;
>   const char *dw_fde_end;
>   const char *dw_fde_hot_section_label;
>   const char *dw_fde_hot_section_end_label;
>   const char *dw_fde_unlikely_section_label;
>   const char *dw_fde_unlikely_section_end_label;
>   dw_cfi_ref dw_fde_switch_cfi; /* Last CFI before switching sections.  */
>   /* True iff dw_fde_begin label is in text_section or cold_text_section.  */
>   unsigned in_std_section : 1;
>   /* True iff dw_fde_unlikely_section_label is in text_section or
>      cold_text_section.  */
>   unsigned cold_in_std_section : 1;
>   /* True iff switched sections.  */
>   unsigned dw_fde_switched_sections : 1;
>   /* True iff switching from cold to hot section.  */
>   unsigned dw_fde_switched_cold_to_hot : 1;

Looking at this patch, without having previously paid much attention to 
the hot/cold partitioning code, I find these flags very confusing; it 
took me a while to figure out what they really mean.

It seems that we are sometimes splitting functions into hot and cold, 
and those partitions can either be in standard sections or 
function-specific ones.  So it seems like the information we want is:

Primary start/end label (dw_fde_begin/end)
Primary in standard section (in_std_section or cold_in_std_section)
Secondary section used (dw_fde_switched_sections)
Secondary start/end label (dw_fde_{hot,unlikely}_section_{,end_}label)
Secondary in standard section (in_std_section or cold_in_std_section)

It seems to me that we don't need to have the labels for both hot and 
cold sections in addition to begin/end, and we could replace 
dw_fde_switched_sections with just checking whether the secondary labels 
are NULL.

I think we could drop dw_fde_switched_cold_to_hot in favor of the 
primary/secondary model; for debugging purposes it doesn't matter 
whether the primary section is hot or cold.

This ought to cut down on the number of cases we have to handle in all 
the different places in dwarf2out that deal with this stuff.

Does that make sense to you?

Jason
Jakub Jelinek - March 24, 2011, 5:13 p.m.
On Thu, Mar 24, 2011 at 06:08:01PM +0100, Jason Merrill wrote:
> This ought to cut down on the number of cases we have to handle in
> all the different places in dwarf2out that deal with this stuff.
> 
> Does that make sense to you?

I'll try to implement it tomorrow, currently don't see why
your idea wouldn't work.  I haven't actually been able to write
a testcase that switches from cold to hot btw., not sure if that
is because I just didn't try very hard or because gcc never emits
it.  -freorder-blocks-and-partition is certainly terribly undertested
(though I wonder if anybody actually uses it, e.g. given that
EH for it has been broken for years).

	Jakub

Patch

--- gcc/dwarf2out.c.jj	2011-03-23 09:34:39.000000000 +0100
+++ gcc/dwarf2out.c	2011-03-23 13:04:37.000000000 +0100
@@ -4291,6 +4291,7 @@  dwarf2out_note_section_used (void)
 void
 dwarf2out_switch_text_section (void)
 {
+  section *sect;
   dw_fde_ref fde = current_fde ();
 
   gcc_assert (cfun && fde && !fde->dw_fde_switched_sections);
@@ -4316,7 +4317,13 @@  dwarf2out_switch_text_section (void)
     fprintf (asm_out_file, "\t.cfi_endproc\n");
 
   /* Now do the real section switch.  */
-  switch_to_section (current_function_section ());
+  sect = current_function_section ();
+  switch_to_section (sect);
+
+  if (fde->dw_fde_switched_cold_to_hot)
+    fde->in_std_section
+      = (sect == text_section
+	 || (cold_text_section && sect == cold_text_section));
 
   if (dwarf2out_do_cfi_asm ())
     {
@@ -6212,19 +6219,6 @@  static GTY (()) VEC (pubname_entry, gc) 
    defines/undefines (and file start/end markers).  */
 static GTY (()) VEC (macinfo_entry, gc) * macinfo_table;
 
-/* Array of dies for which we should generate .debug_arange info.  */
-static GTY((length ("arange_table_allocated"))) dw_die_ref *arange_table;
-
-/* Number of elements currently allocated for arange_table.  */
-static GTY(()) unsigned arange_table_allocated;
-
-/* Number of elements in arange_table currently in use.  */
-static GTY(()) unsigned arange_table_in_use;
-
-/* Size (in elements) of increments by which we may expand the
-   arange_table.  */
-#define ARANGE_TABLE_INCREMENT 64
-
 /* Array of dies for which we should generate .debug_ranges info.  */
 static GTY ((length ("ranges_table_allocated"))) dw_ranges_ref ranges_table;
 
@@ -6432,8 +6426,7 @@  static void add_pubname (tree, dw_die_re
 static void add_pubname_string (const char *, dw_die_ref);
 static void add_pubtype (tree, dw_die_ref);
 static void output_pubnames (VEC (pubname_entry,gc) *);
-static void add_arange (tree, dw_die_ref);
-static void output_aranges (void);
+static void output_aranges (unsigned long);
 static unsigned int add_ranges_num (int);
 static unsigned int add_ranges (const_tree);
 static void add_ranges_by_labels (dw_die_ref, const char *, const char *,
@@ -10881,7 +10874,20 @@  size_of_aranges (void)
     size += 2 * DWARF2_ADDR_SIZE;
   if (cold_text_section_used)
     size += 2 * DWARF2_ADDR_SIZE;
-  size += 2 * DWARF2_ADDR_SIZE * arange_table_in_use;
+  if (have_multiple_function_sections)
+    {
+      unsigned fde_idx = 0;
+
+      for (fde_idx = 0; fde_idx < fde_table_in_use; fde_idx++)
+	{
+	  dw_fde_ref fde = &fde_table[fde_idx];
+
+	  if (!fde->in_std_section)
+	    size += 2 * DWARF2_ADDR_SIZE;
+	  if (fde->dw_fde_switched_sections && !fde->cold_in_std_section)
+	    size += 2 * DWARF2_ADDR_SIZE;
+	}
+    }
 
   /* Count the two zero words used to terminated the address range table.  */
   size += 2 * DWARF2_ADDR_SIZE;
@@ -11709,35 +11715,14 @@  output_pubnames (VEC (pubname_entry, gc)
   dw2_asm_output_data (DWARF_OFFSET_SIZE, 0, NULL);
 }
 
-/* Add a new entry to .debug_aranges if appropriate.  */
-
-static void
-add_arange (tree decl, dw_die_ref die)
-{
-  if (! DECL_SECTION_NAME (decl))
-    return;
-
-  if (arange_table_in_use == arange_table_allocated)
-    {
-      arange_table_allocated += ARANGE_TABLE_INCREMENT;
-      arange_table = GGC_RESIZEVEC (dw_die_ref, arange_table,
-				    arange_table_allocated);
-      memset (arange_table + arange_table_in_use, 0,
-	      ARANGE_TABLE_INCREMENT * sizeof (dw_die_ref));
-    }
-
-  arange_table[arange_table_in_use++] = die;
-}
-
 /* Output the information that goes into the .debug_aranges table.
    Namely, define the beginning and ending address range of the
    text section generated for this compilation unit.  */
 
 static void
-output_aranges (void)
+output_aranges (unsigned long aranges_length)
 {
   unsigned i;
-  unsigned long aranges_length = size_of_aranges ();
 
   if (DWARF_INITIAL_LENGTH_SIZE - DWARF_OFFSET_SIZE == 4)
     dw2_asm_output_data (4, 0xffffffff,
@@ -11782,38 +11767,44 @@  output_aranges (void)
 			    cold_text_section_label, "Length");
     }
 
-  for (i = 0; i < arange_table_in_use; i++)
+  if (have_multiple_function_sections)
     {
-      dw_die_ref die = arange_table[i];
-
-      /* We shouldn't see aranges for DIEs outside of the main CU.  */
-      gcc_assert (die->die_mark);
+      unsigned fde_idx = 0;
 
-      if (die->die_tag == DW_TAG_subprogram)
-	{
-	  dw2_asm_output_addr (DWARF2_ADDR_SIZE, get_AT_low_pc (die),
-			       "Address");
-	  dw2_asm_output_delta (DWARF2_ADDR_SIZE, get_AT_hi_pc (die),
-				get_AT_low_pc (die), "Length");
-	}
-      else
+      for (fde_idx = 0; fde_idx < fde_table_in_use; fde_idx++)
 	{
-	  /* A static variable; extract the symbol from DW_AT_location.
-	     Note that this code isn't currently hit, as we only emit
-	     aranges for functions (jason 9/23/99).  */
-	  dw_attr_ref a = get_AT (die, DW_AT_location);
-	  dw_loc_descr_ref loc;
-
-	  gcc_assert (a && AT_class (a) == dw_val_class_loc);
-
-	  loc = AT_loc (a);
-	  gcc_assert (loc->dw_loc_opc == DW_OP_addr);
-
-	  dw2_asm_output_addr_rtx (DWARF2_ADDR_SIZE,
-				   loc->dw_loc_oprnd1.v.val_addr, "Address");
-	  dw2_asm_output_data (DWARF2_ADDR_SIZE,
-			       get_AT_unsigned (die, DW_AT_byte_size),
-			       "Length");
+	  dw_fde_ref fde = &fde_table[fde_idx];
+
+	  if (fde->dw_fde_switched_sections)
+	    {
+	      if (!fde->in_std_section)
+		{
+		  dw2_asm_output_addr (DWARF2_ADDR_SIZE,
+				       fde->dw_fde_hot_section_label,
+				       "Address");
+		  dw2_asm_output_delta (DWARF2_ADDR_SIZE,
+					fde->dw_fde_hot_section_end_label,
+					fde->dw_fde_hot_section_label,
+					"Length");
+		}
+	      if (!fde->cold_in_std_section)
+		{
+		  dw2_asm_output_addr (DWARF2_ADDR_SIZE,
+				       fde->dw_fde_unlikely_section_label,
+				       "Address");
+		  dw2_asm_output_delta (DWARF2_ADDR_SIZE,
+					fde->dw_fde_unlikely_section_end_label,
+					fde->dw_fde_unlikely_section_label,
+					"Length");
+		}
+	    }
+	  else if (!fde->in_std_section)
+	    {
+	      dw2_asm_output_addr (DWARF2_ADDR_SIZE, fde->dw_fde_begin,
+				   "Address");
+	      dw2_asm_output_delta (DWARF2_ADDR_SIZE, fde->dw_fde_end,
+				    fde->dw_fde_begin, "Length");
+	    }
 	}
     }
 
@@ -19193,7 +19184,6 @@  gen_subprogram_die (tree decl, dw_die_re
 #endif
 
 	  add_pubname (decl, subr_die);
-	  add_arange (decl, subr_die);
 	}
       else
 	{  /* Generate pubnames entries for the split function code
@@ -19209,7 +19199,7 @@  gen_subprogram_die (tree decl, dw_die_re
 		     section, since the HOT/COLD labels might precede an 
 		     alignment offset.  */
 		  bool range_list_added = false;
-		  if (fde->in_std_section)
+		  if (!fde->dw_fde_switched_cold_to_hot)
 		    {
 		      add_ranges_by_labels (subr_die,
 					    fde->dw_fde_begin,
@@ -19255,7 +19245,6 @@  gen_subprogram_die (tree decl, dw_die_re
 				 fde->dw_fde_end);
 		  /* Add it.   */
 		  add_pubname (decl, subr_die);
-		  add_arange (decl, subr_die);
 
 		  /* Build a minimal DIE for the secondary section.  */
 		  seg_die = new_die (DW_TAG_subprogram,
@@ -19278,7 +19267,7 @@  gen_subprogram_die (tree decl, dw_die_re
 		  if (DECL_ARTIFICIAL (decl))
 		    add_AT_flag (seg_die, DW_AT_artificial, 1);
 
-		  if (fde->in_std_section)
+		  if (!fde->dw_fde_switched_cold_to_hot)
 		    {
 		      name = concat ("__cold_sect_of_", name, NULL); 
 		      add_AT_lbl_id (seg_die, DW_AT_low_pc,
@@ -19296,7 +19285,6 @@  gen_subprogram_die (tree decl, dw_die_re
 		    }
 		  add_name_attribute (seg_die, name);
 		  add_pubname_string (name, seg_die);
-		  add_arange (decl, seg_die);
 		}
 	    }
 	  else
@@ -19304,7 +19292,6 @@  gen_subprogram_die (tree decl, dw_die_re
 	      add_AT_lbl_id (subr_die, DW_AT_low_pc, fde->dw_fde_begin);
 	      add_AT_lbl_id (subr_die, DW_AT_high_pc, fde->dw_fde_end);
 	      add_pubname (decl, subr_die);
-	      add_arange (decl, subr_die);
 	    }
 	}
 
@@ -22041,7 +22028,7 @@  dwarf2out_begin_function (tree fun)
 {
   if (function_section (fun) != text_section)
     have_multiple_function_sections = true;
-  else if (flag_reorder_blocks_and_partition && !cold_text_section)
+  if (flag_reorder_blocks_and_partition && !cold_text_section)
     {
       gcc_assert (current_function_decl == fun);
       cold_text_section = unlikely_text_section ();
@@ -22751,11 +22738,9 @@  prune_unused_types (void)
     }
 
   /* Also set the mark on nodes referenced from the
-     pubname_table or arange_table.  */
+     pubname_table.  */
   FOR_EACH_VEC_ELT (pubname_entry, pubname_table, i, pub)
     prune_unused_types_mark (pub->die, 1);
-  for (i = 0; i < arange_table_in_use; i++)
-    prune_unused_types_mark (arange_table[i], 1);
 
   /* Get rid of nodes that aren't marked; and update the string counts.  */
   if (debug_str_hash && debug_str_hash_forced)
@@ -23553,7 +23538,7 @@  dwarf2out_finish (const char *filename)
       if (text_section_used)
 	add_ranges_by_labels (comp_unit_die (), text_section_label,
 			      text_end_label, &range_list_added);
-      if (flag_reorder_blocks_and_partition && cold_text_section_used)
+      if (cold_text_section_used)
 	add_ranges_by_labels (comp_unit_die (), cold_text_section_label,
 			      cold_end_label, &range_list_added);
 
@@ -23676,13 +23661,21 @@  dwarf2out_finish (const char *filename)
 	}
     }
 
-  /* Output the address range information.  We only put functions in the arange
-     table, so don't write it out if we don't have any.  */
-  if ((text_section_used || cold_text_section_used || arange_table_in_use)
-      && info_section_emitted)
-    {
-      switch_to_section (debug_aranges_section);
-      output_aranges ();
+  /* Output the address range information.  We only put functions in the
+     arange table, so don't write it out if we don't have any.  */
+  if (info_section_emitted)
+    {
+      unsigned long aranges_length = size_of_aranges ();
+
+      /* Empty .debug_aranges would contain just header and
+	 terminating 0,0.  */
+      if (aranges_length
+	  != (unsigned long) (DWARF_ARANGES_HEADER_SIZE
+			      + 2 * DWARF2_ADDR_SIZE))
+	{
+	  switch_to_section (debug_aranges_section);
+	  output_aranges (aranges_length);
+	}
     }
 
   /* Output ranges section if necessary.  */