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

[Simon Marchi <simark@simark.ca>]

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