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

[Joel Brobecker <brobecker@adacore.com>]

Hey Keith,

First of all, thanks a lot again for all your work on this PR.
Anyone, please feel free to comment as well!

> 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 | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 1cabfbb0d4..04f22114f8 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -9701,6 +9701,33 @@ compute_delayed_physnames (struct dwarf2_cu *cu)
>    cu->method_list.clear ();
>  }
>  
> +/* 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?

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?

> +#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?

> +#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"

Other than that, no other comment for branch "master".

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

Thanks!
-- 
Joel