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

[Simon Marchi <simark@simark.ca>]

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