Fix -gsplit-dwarf ICE (PR debug/85302)

Message ID 20180410140102.GH8577@tucnak
State New
Headers show
Series
  • Fix -gsplit-dwarf ICE (PR debug/85302)
Related show

Commit Message

Jakub Jelinek April 10, 2018, 2:01 p.m.
Hi!

The r257510 change broke -gsplit-dwarf support by introducing a circular
dependency.  Before that revision index_location_lists used to do:
            /* Don't index an entry that has already been indexed
               or won't be output.  */
            if (curr->begin_entry != NULL
                || (strcmp (curr->begin, curr->end) == 0 && !curr->force))
              continue;
r257510 introduced a function which does that
(strcmp (curr->begin, curr->end) == 0 && !curr->force)
part extended for LVUs, but also calls size_of_locs.  In dwarf4 and earlier
we really can't emit location expressions >= 64KB in size, so we just
ignored those during output and the helper function used by the output and
now this spot uses that too.  The problem is that size_of_locs needs
(in some cases) the split dwarf address table indexes to be computed (the
indexes are uleb128s and thus need to be sized accurately), but at the point
index_location_lists is called, those aren't computed and can't easily be,
because that very function adds new address table entries and the current
logic requires that once indexes are assigned the hash table is immutable,
during output we assert we go through the same indexes as were assigned.
In theory we could introduce another hash table to hold the new table
entries that didn't have indexes assigned yet, but given that >= 64KB locs
are extremely rare, I think it is just wasted effort.

This patch just makes sure we create address table entry without computing
the size_of_locs and so rarely could add an entry nothing will really use;
it is just an address though, so not a big deal IMHO.

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

2018-04-10  Jakub Jelinek  <jakub@redhat.com>

	PR debug/85302
	* dwarf2out.c (skip_loc_list_entry): Don't call size_of_locs if
	SIZEP is NULL.
	(output_loc_list): Pass address of a dummy size variable even in the
	locview handling loop.
	(index_location_lists): Add comment on why skip_loc_list_entry can't
	call size_of_locs.

	* g++.dg/debug/dwarf2/pr85302.C: New test.


	Jakub

Comments

Richard Biener April 11, 2018, 9:57 a.m. | #1
On Tue, 10 Apr 2018, Jakub Jelinek wrote:

> Hi!
> 
> The r257510 change broke -gsplit-dwarf support by introducing a circular
> dependency.  Before that revision index_location_lists used to do:
>             /* Don't index an entry that has already been indexed
>                or won't be output.  */
>             if (curr->begin_entry != NULL
>                 || (strcmp (curr->begin, curr->end) == 0 && !curr->force))
>               continue;
> r257510 introduced a function which does that
> (strcmp (curr->begin, curr->end) == 0 && !curr->force)
> part extended for LVUs, but also calls size_of_locs.  In dwarf4 and earlier
> we really can't emit location expressions >= 64KB in size, so we just
> ignored those during output and the helper function used by the output and
> now this spot uses that too.  The problem is that size_of_locs needs
> (in some cases) the split dwarf address table indexes to be computed (the
> indexes are uleb128s and thus need to be sized accurately), but at the point
> index_location_lists is called, those aren't computed and can't easily be,
> because that very function adds new address table entries and the current
> logic requires that once indexes are assigned the hash table is immutable,
> during output we assert we go through the same indexes as were assigned.
> In theory we could introduce another hash table to hold the new table
> entries that didn't have indexes assigned yet, but given that >= 64KB locs
> are extremely rare, I think it is just wasted effort.
> 
> This patch just makes sure we create address table entry without computing
> the size_of_locs and so rarely could add an entry nothing will really use;
> it is just an address though, so not a big deal IMHO.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

> 2018-04-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/85302
> 	* dwarf2out.c (skip_loc_list_entry): Don't call size_of_locs if
> 	SIZEP is NULL.
> 	(output_loc_list): Pass address of a dummy size variable even in the
> 	locview handling loop.
> 	(index_location_lists): Add comment on why skip_loc_list_entry can't
> 	call size_of_locs.
> 
> 	* g++.dg/debug/dwarf2/pr85302.C: New test.
> 
> --- gcc/dwarf2out.c.jj	2018-04-06 19:28:24.000000000 +0200
> +++ gcc/dwarf2out.c	2018-04-10 10:21:21.352384405 +0200
> @@ -10032,18 +10032,22 @@ maybe_gen_llsym (dw_loc_list_ref list)
>    gen_llsym (list);
>  }
>  
> -/* Determine whether or not to skip loc_list entry CURR.  If we're not
> +/* Determine whether or not to skip loc_list entry CURR.  If SIZEP is
> +   NULL, don't consider size of the location expression.  If we're not
>     to skip it, and SIZEP is non-null, store the size of CURR->expr's
>     representation in *SIZEP.  */
>  
>  static bool
> -skip_loc_list_entry (dw_loc_list_ref curr, unsigned long *sizep = 0)
> +skip_loc_list_entry (dw_loc_list_ref curr, unsigned long *sizep = NULL)
>  {
>    /* Don't output an entry that starts and ends at the same address.  */
>    if (strcmp (curr->begin, curr->end) == 0
>        && curr->vbegin == curr->vend && !curr->force)
>      return true;
>  
> +  if (!sizep)
> +    return false;
> +
>    unsigned long size = size_of_locs (curr->expr);
>  
>    /* If the expression is too large, drop it on the floor.  We could
> @@ -10053,8 +10057,7 @@ skip_loc_list_entry (dw_loc_list_ref cur
>    if (dwarf_version < 5 && size > 0xffff)
>      return true;
>  
> -  if (sizep)
> -    *sizep = size;
> +  *sizep = size;
>  
>    return false;
>  }
> @@ -10121,7 +10124,9 @@ output_loc_list (dw_loc_list_ref list_he
>        for (dw_loc_list_ref curr = list_head; curr != NULL;
>  	   curr = curr->dw_loc_next)
>  	{
> -	  if (skip_loc_list_entry (curr))
> +	  unsigned long size;
> +
> +	  if (skip_loc_list_entry (curr, &size))
>  	    continue;
>  
>  	  vcount++;
> @@ -30887,7 +30892,14 @@ index_location_lists (dw_die_ref die)
>          for (curr = list; curr != NULL; curr = curr->dw_loc_next)
>            {
>              /* Don't index an entry that has already been indexed
> -               or won't be output.  */
> +	       or won't be output.  Make sure skip_loc_list_entry doesn't
> +	       call size_of_locs, because that might cause circular dependency,
> +	       index_location_lists requiring address table indexes to be
> +	       computed, but adding new indexes through add_addr_table_entry
> +	       and address table index computation requiring no new additions
> +	       to the hash table.  In the rare case of DWARF[234] >= 64KB
> +	       location expression, we'll just waste unused address table entry
> +	       for it.  */
>              if (curr->begin_entry != NULL
>                  || skip_loc_list_entry (curr))
>                continue;
> --- gcc/testsuite/g++.dg/debug/dwarf2/pr85302.C.jj	2018-04-10 10:29:22.994385218 +0200
> +++ gcc/testsuite/g++.dg/debug/dwarf2/pr85302.C	2018-04-10 10:33:00.036385099 +0200
> @@ -0,0 +1,14 @@
> +// PR debug/85302
> +// { dg-do compile }
> +// { dg-options "-std=c++11 -gsplit-dwarf -O1" }
> +// { dg-additional-options "-fPIE" { target pie } }
> +
> +struct A { const char *b; A (const char *c) : b(c) {} };
> +struct B { void foo (A); };
> +B e;
> +
> +void
> +bar ()
> +{
> +  e.foo ("");
> +}
> 
> 	Jakub
> 
>

Patch

--- gcc/dwarf2out.c.jj	2018-04-06 19:28:24.000000000 +0200
+++ gcc/dwarf2out.c	2018-04-10 10:21:21.352384405 +0200
@@ -10032,18 +10032,22 @@  maybe_gen_llsym (dw_loc_list_ref list)
   gen_llsym (list);
 }
 
-/* Determine whether or not to skip loc_list entry CURR.  If we're not
+/* Determine whether or not to skip loc_list entry CURR.  If SIZEP is
+   NULL, don't consider size of the location expression.  If we're not
    to skip it, and SIZEP is non-null, store the size of CURR->expr's
    representation in *SIZEP.  */
 
 static bool
-skip_loc_list_entry (dw_loc_list_ref curr, unsigned long *sizep = 0)
+skip_loc_list_entry (dw_loc_list_ref curr, unsigned long *sizep = NULL)
 {
   /* Don't output an entry that starts and ends at the same address.  */
   if (strcmp (curr->begin, curr->end) == 0
       && curr->vbegin == curr->vend && !curr->force)
     return true;
 
+  if (!sizep)
+    return false;
+
   unsigned long size = size_of_locs (curr->expr);
 
   /* If the expression is too large, drop it on the floor.  We could
@@ -10053,8 +10057,7 @@  skip_loc_list_entry (dw_loc_list_ref cur
   if (dwarf_version < 5 && size > 0xffff)
     return true;
 
-  if (sizep)
-    *sizep = size;
+  *sizep = size;
 
   return false;
 }
@@ -10121,7 +10124,9 @@  output_loc_list (dw_loc_list_ref list_he
       for (dw_loc_list_ref curr = list_head; curr != NULL;
 	   curr = curr->dw_loc_next)
 	{
-	  if (skip_loc_list_entry (curr))
+	  unsigned long size;
+
+	  if (skip_loc_list_entry (curr, &size))
 	    continue;
 
 	  vcount++;
@@ -30887,7 +30892,14 @@  index_location_lists (dw_die_ref die)
         for (curr = list; curr != NULL; curr = curr->dw_loc_next)
           {
             /* Don't index an entry that has already been indexed
-               or won't be output.  */
+	       or won't be output.  Make sure skip_loc_list_entry doesn't
+	       call size_of_locs, because that might cause circular dependency,
+	       index_location_lists requiring address table indexes to be
+	       computed, but adding new indexes through add_addr_table_entry
+	       and address table index computation requiring no new additions
+	       to the hash table.  In the rare case of DWARF[234] >= 64KB
+	       location expression, we'll just waste unused address table entry
+	       for it.  */
             if (curr->begin_entry != NULL
                 || skip_loc_list_entry (curr))
               continue;
--- gcc/testsuite/g++.dg/debug/dwarf2/pr85302.C.jj	2018-04-10 10:29:22.994385218 +0200
+++ gcc/testsuite/g++.dg/debug/dwarf2/pr85302.C	2018-04-10 10:33:00.036385099 +0200
@@ -0,0 +1,14 @@ 
+// PR debug/85302
+// { dg-do compile }
+// { dg-options "-std=c++11 -gsplit-dwarf -O1" }
+// { dg-additional-options "-fPIE" { target pie } }
+
+struct A { const char *b; A (const char *c) : b(c) {} };
+struct B { void foo (A); };
+B e;
+
+void
+bar ()
+{
+  e.foo ("");
+}