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

[Joel Brobecker <brobecker at adacore dot com>]

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

We are on the same page as to what I was suggesting. Option (2) felt
a bit more natural to me, but I am happy with (1) also. Since that's
what you've been testing so far, let's go with that.

> 
> >> +#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.]

I like assertions too, but have started to shy away from them for
situation that we can recover from. But indeed, your point about
the advantage of the assert is quite true, especially since the
alternative is actually not trying to handle the situation, so not
very useful in the grand scheme of things. To be more precise, choosing
to generate complaint means that the user would see the complaint
followed by the failed assertion, as opposed to what we have now,
which is the failed assertion. Might as well generate a failed assertion
here, which will be clearer for us to understand what is going on.

Perhaps we can slightly extend the comment you alread added to confirm
we expect all symbols of a given unit to be of the same language:

 /* Only complain if LISTHEAD already contains symbols of a different
    language (all symbols within this list should have the same language).  */

Something like that? (just a thought, feel free to ignore if you think
it is redundant.

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

Nice.

So I'll wait for the final version of the patch, just
Thanks Keith!
-- 
Joel