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

[Keith Seitz <keiths@redhat.com>]

On 06/11/2018 03:26 PM, Joel Brobecker wrote:
>> +/* A wrapper for add_symbol_to_list to issue a complaint if a symbol
>> +   with a different language is added to LISTHEAD.  */
>> +
>> +static inline void
>> +dw2_add_symbol_to_list (struct symbol *symbol, struct pending **listhead)
>> +{
>> +  /* Only complain if LISTHEAD already contains symbols of a different
>> +     language.  */
> 
> I confess I got confused a bit by this comment about thinking this was
> a comment about the function's behavior, as in "only complain once",
> and therefore belonging in the function's introductory comment.  But
> thinking more clearly about this, you have to wait for at least
> one symbol to be in the list; otherwise, talking to myself what else
> would you compare it too, right?

Yes, that's just about right. As you note below, we could use the new symbol's CU.

> But I am wondering if we could do it a bit better. In particular,
> at the moment, it seems like all you have is the existing list of
> symbols; but more interesting would be the CU's language, right?
> What about passing the dwarf_cu? From what I can tell, we seem to
> have it each time we call this function. Or is that exactly the
> issue we're dealing with?

I'm not sure I completely follow, but let me see if I can reason this out a little.

When add_symbol_to_list is called, we have (among other things), the symbol, the CU from which the symbol came, and the symbol list we are going to add the symbol to.

From this data, we need to check that either the CU's language or the symbol's language is the same as the language of the symbol list. *ALL* those symbols in the list must have the same language (or we hit the now infamous assertion).

So that suggests two options for ensuring that all symbols added to any list are the same, provided that there are already symbols in the list (if there are no symbols, then this new symbol will define the list's language):

1) We check SYMBOL_LANGUAGE of the new symbol to the SYMBOL_LANGUAGE of first symbol in the list.
   This is the approach used in the current version.
2) We can check the CU's langauge against the SYMBOL_LANGUAGE of the first symbol in the list.

I don't see a clear winner here.

>> +#if USE_ASSERT
>> +  gdb_assert ((*listhead) == NULL
>> +	      || (*listhead)->nsyms == 0
>> +	      || (SYMBOL_LANGUAGE ((*listhead)->symbol[0])
>> +		  == SYMBOL_LANGUAGE (symbol)));
> 
> So, if I understand your preliminary explanation, this part will be
> removed, right?

Yes -- one of these branches would be removed. I prefer the assertion, but I could make a case for simply issuing the complaint. So I've left it up to maintainers. :-) [Biggest plus for assertion: issuing only the complaint here will cause the DICT_LANGUAGE != SYMBOL_LANGUAGE assertion we've been seeing. I'd rather see this happen sooner than later.]

>> +#else
>> +  if ((*listhead) != NULL && (*listhead)->nsyms > 0
>> +      && SYMBOL_LANGUAGE ((*listhead)->symbol[0]) != SYMBOL_LANGUAGE (symbol))
>> +    {
>> +      complaint (_("recording symbol \"%s\" with language %s "
>> +		   "into list of langauge %s"), SYMBOL_LINKAGE_NAME (symbol),
>                                  ^^^^^^^^
> Small typo in "langauge"

Fixed.

> Do we want something for the gdb-8.1.1 release? I would have thought
> so. But I might suggest instead the shorter version, without the
> complaint (just because it gives us a bit more time on master to
> double-check that the overhead is indeed minimal -- not as obvious
> as I might have thought when I first thought about it).

Yes, I can commit the simple one-line change for 8.1.1. I think it important that this fix get pushed everywhere.

Keith