Re: [PATCH] Fix -gsplit-dwarf ICE (PR debug/85302)

[Richard Biener <rguenther at suse dot de>]

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
> 
> 

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