[v2] Really fix gdb/23010: SYMBOL_LANGUAGE != DICT_LANGUAGE assertion

  This patch is another attempt at really fixing the multitude of assertions
being seen where symbols of one language are being added to symbol lists of
another language.

In short, this is happening because read_file_scope was reading DIEs without
having its language set. As a result, the default language,
language_minimal, was being passed to imported DIE trees and, in certain
specific situations, causing the SYMBOL_LANGUAGE != DICT_LANGUAGE assertion
being widely reported.

For a more thorough explanation, see the following mailing list
post:

  https://sourceware.org/ml/gdb-patches/2018-05/msg00703.html

Since a test reproducer for this has been so elusive, this patch also adds a
wrapper function around add_symbol_to_list which asserts when adding a
symbol of one language to a list containing symbols of a different language.

Differences from v1:
- Removed complaint variation
- Updated comments
- Fixed typo

gdb/ChangeLog:

	PR gdb/23010
	* dwarf2read.c (dw2_add_symbol_to_list): New function.
	(fixup_go_packaging, new_symbol): Use dw2_add_symbol_to_list
	instead of add_symbol_to_list.
	(read_file_scope): Call prepare_one_comp_unit before reading
	any other DIEs.
---
 gdb/dwarf2read.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)
  

Hi Keith,

On 2018-06-27 03:03 PM, Keith Seitz wrote:
> This patch is another attempt at really fixing the multitude of assertions
> being seen where symbols of one language are being added to symbol lists of
> another language.
> 
> In short, this is happening because read_file_scope was reading DIEs without
> having its language set. As a result, the default language,
> language_minimal, was being passed to imported DIE trees and, in certain
> specific situations, causing the SYMBOL_LANGUAGE != DICT_LANGUAGE assertion
> being widely reported.
> 
> For a more thorough explanation, see the following mailing list
> post:
> 
>   https://sourceware.org/ml/gdb-patches/2018-05/msg00703.html

I wouldn't mind if you included the information here.

> Since a test reproducer for this has been so elusive, this patch also adds a
> wrapper function around add_symbol_to_list which asserts when adding a
> symbol of one language to a list containing symbols of a different language.

I have a question about the assertion vs complaint.  Is it logically
impossible (putting aside other GDB bugs) for this assertion to fail?
Or is it possible now to feed bad debug info to GDB that will trigger
this new assert?  If it's the former, then no problem, if it's the later
then an assertion is not appropriate.

> Differences from v1:
> - Removed complaint variation
> - Updated comments
> - Fixed typo
> 
> gdb/ChangeLog:
> 
> 	PR gdb/23010
> 	* dwarf2read.c (dw2_add_symbol_to_list): New function.
> 	(fixup_go_packaging, new_symbol): Use dw2_add_symbol_to_list
> 	instead of add_symbol_to_list.
> 	(read_file_scope): Call prepare_one_comp_unit before reading
> 	any other DIEs.
> ---
>  gdb/dwarf2read.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 4ad0527406..ba70315957 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -9701,6 +9701,24 @@ compute_delayed_physnames (struct dwarf2_cu *cu)
>    cu->method_list.clear ();
>  }
>  
> +/* A wrapper for add_symbol_to_list to ensure that SYMBOL's language is
> +   the same as all other symbols in LISTHEAD.  If a new symbol is added
> +   with a different language, this function asserts.  */
> +
> +static inline void
> +dw2_add_symbol_to_list (struct symbol *symbol, struct pending **listhead)
> +{
> +  /* Only assert if LISTHEAD already contains symbols of a different
> +     language (dict_create_hashed/insert_symbol_hashed requires that all
> +     symbols in this list are of the same language).  */
> +  gdb_assert ((*listhead) == NULL
> +	      || (*listhead)->nsyms == 0
> +	      || (SYMBOL_LANGUAGE ((*listhead)->symbol[0])
> +		  == SYMBOL_LANGUAGE (symbol)));

I wonder if it's actually possible to have (*listhead)->nsyms == 0, since as
soon as a "struct pending" is allocated, there is at least one sym put into
it (in add_symbol_to_list).

It's probably a bit hard to tell for older debug formats, but is it only for
DWARF that this condition must hold?  Did you consider putting this assertion
directly in add_symbol_to_list?  I'm not saying it's the right thing to do, I'm
just asking.

Thanks,

Simon
  

On 07/10/2018 06:17 PM, Simon Marchi wrote:
> I wouldn't mind if you included the information here.

I will certainly include a redux of that analysis in the commit log.

> I have a question about the assertion vs complaint.  Is it logically
> impossible (putting aside other GDB bugs) for this assertion to fail?
> Or is it possible now to feed bad debug info to GDB that will trigger
> this new assert?  If it's the former, then no problem, if it's the later
> then an assertion is not appropriate.

IMO the former. The code is currently structured so that a dictionary can *only* have symbols of one language inside it. While it may be possible to trigger this with malicious DWARF hacking, I cannot really say for certain that it isn't possible in valid DWARF. [Sorry, that's more of a politician's answer to your question.]

>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index 4ad0527406..ba70315957 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -9701,6 +9701,24 @@ compute_delayed_physnames (struct dwarf2_cu *cu)
>>    cu->method_list.clear ();
>>  }
>>  
>> +/* A wrapper for add_symbol_to_list to ensure that SYMBOL's language is
>> +   the same as all other symbols in LISTHEAD.  If a new symbol is added
>> +   with a different language, this function asserts.  */
>> +
>> +static inline void
>> +dw2_add_symbol_to_list (struct symbol *symbol, struct pending **listhead)
>> +{
>> +  /* Only assert if LISTHEAD already contains symbols of a different
>> +     language (dict_create_hashed/insert_symbol_hashed requires that all
>> +     symbols in this list are of the same language).  */
>> +  gdb_assert ((*listhead) == NULL
>> +	      || (*listhead)->nsyms == 0
>> +	      || (SYMBOL_LANGUAGE ((*listhead)->symbol[0])
>> +		  == SYMBOL_LANGUAGE (symbol)));
> 
> I wonder if it's actually possible to have (*listhead)->nsyms == 0, since as
> soon as a "struct pending" is allocated, there is at least one sym put into
> it (in add_symbol_to_list).

Ha! Yes, that's true. I didn't even look. I just erred on the defensive side. I'll remove that.

> It's probably a bit hard to tell for older debug formats, but is it only for
> DWARF that this condition must hold?  Did you consider putting this assertion
> directly in add_symbol_to_list?  I'm not saying it's the right thing to do, I'm
> just asking.

The assertion holds for any debug reader that uses hashed dictionaries, and that list isn't a straightforward list to compose. That's probably why I originally isolated to dwarf2read.c. Or I just plain didn't think about (interfering with) other debuginfo readers.

Any symbol reader using add_symbol_to_list could create hashed dictionaries, but AFAICT there is no requirement that it does.

Out of curiosity, I moved (and adjusted) the assertion to add_symbol_to_list and ran some tests. As expected, DWARF tests showed no difference. STABS testing, though, triggers the assertion. add_symbol_to_list is often called before any dictionary is created.

Thank you for taking a look,
Keith
  

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> For a more thorough explanation, see the following mailing list
Keith> post:
Keith>   https://sourceware.org/ml/gdb-patches/2018-05/msg00703.html

I think it would be good for this analysis to end up in the commit
message.  I found that analysis compelling.

Otherwise, FWIW, this patch seems correct to me.

Tom
  

On 2018-07-12 11:47, Keith Seitz wrote:
>> I have a question about the assertion vs complaint.  Is it logically
>> impossible (putting aside other GDB bugs) for this assertion to fail?
>> Or is it possible now to feed bad debug info to GDB that will trigger
>> this new assert?  If it's the former, then no problem, if it's the 
>> later
>> then an assertion is not appropriate.
> 
> IMO the former. The code is currently structured so that a dictionary
> can *only* have symbols of one language inside it. While it may be
> possible to trigger this with malicious DWARF hacking, I cannot really
> say for certain that it isn't possible in valid DWARF. [Sorry, that's
> more of a politician's answer to your question.]

Ok.  I'm asking because ideally, we should react to bad input (including 
DWARF) by providing a useful error message rather than asserting.  I'm 
fine with leaving it like this.  That's not in the scope of your patch, 
and your patch does not make the situation worse (it only points out the 
problem earlier).

>>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>>> index 4ad0527406..ba70315957 100644
>>> --- a/gdb/dwarf2read.c
>>> +++ b/gdb/dwarf2read.c
>>> @@ -9701,6 +9701,24 @@ compute_delayed_physnames (struct dwarf2_cu 
>>> *cu)
>>>    cu->method_list.clear ();
>>>  }
>>> 
>>> +/* A wrapper for add_symbol_to_list to ensure that SYMBOL's language 
>>> is
>>> +   the same as all other symbols in LISTHEAD.  If a new symbol is 
>>> added
>>> +   with a different language, this function asserts.  */
>>> +
>>> +static inline void
>>> +dw2_add_symbol_to_list (struct symbol *symbol, struct pending 
>>> **listhead)
>>> +{
>>> +  /* Only assert if LISTHEAD already contains symbols of a different
>>> +     language (dict_create_hashed/insert_symbol_hashed requires that 
>>> all
>>> +     symbols in this list are of the same language).  */
>>> +  gdb_assert ((*listhead) == NULL
>>> +	      || (*listhead)->nsyms == 0
>>> +	      || (SYMBOL_LANGUAGE ((*listhead)->symbol[0])
>>> +		  == SYMBOL_LANGUAGE (symbol)));
>> 
>> I wonder if it's actually possible to have (*listhead)->nsyms == 0, 
>> since as
>> soon as a "struct pending" is allocated, there is at least one sym put 
>> into
>> it (in add_symbol_to_list).
> 
> Ha! Yes, that's true. I didn't even look. I just erred on the
> defensive side. I'll remove that.
> 
>> It's probably a bit hard to tell for older debug formats, but is it 
>> only for
>> DWARF that this condition must hold?  Did you consider putting this 
>> assertion
>> directly in add_symbol_to_list?  I'm not saying it's the right thing 
>> to do, I'm
>> just asking.
> 
> The assertion holds for any debug reader that uses hashed
> dictionaries, and that list isn't a straightforward list to compose.
> That's probably why I originally isolated to dwarf2read.c. Or I just
> plain didn't think about (interfering with) other debuginfo readers.
> 
> Any symbol reader using add_symbol_to_list could create hashed
> dictionaries, but AFAICT there is no requirement that it does.
> 
> Out of curiosity, I moved (and adjusted) the assertion to
> add_symbol_to_list and ran some tests. As expected, DWARF tests showed
> no difference. STABS testing, though, triggers the assertion.
> add_symbol_to_list is often called before any dictionary is created.

Ok, thanks.  The patch looks good to me, but I'd like if somebody with 
more experience in this area gave a second opinion.

Simon
  

On 2018-07-12 01:12 PM, Simon Marchi wrote:
> Ok, thanks.  The patch looks good to me, but I'd like if somebody with more experience in this area gave a second opinion.
> 
> Simon

Since we didn't hear from anybody else and don't want to keep you waiting
for ever, please go ahead and push the patch.

Thanks,

Simon
  

On 07/24/2018 01:13 PM, Simon Marchi wrote:
> On 2018-07-12 01:12 PM, Simon Marchi wrote:
>> Ok, thanks.  The patch looks good to me, but I'd like if somebody with more experience in this area gave a second opinion.
>>
>> Simon
> 
> Since we didn't hear from anybody else and don't want to keep you waiting
> for ever, please go ahead and push the patch.

Done [for both master and gdb-8.2-branch].

Thank you!
Keith