Re: [PATCH] gdb: Handle ICC's unexpected void return type

[Keith Seitz <keiths@redhat.com>]

On 10/23/18 2:28 PM, Andrew Burgess wrote:
> So function 'function_foo' has void return type, but still has a
> DW_AT_type attribute for a 0 sized, type called void.

Not only that, but the C/C++ "void" type is explicitly called out in
the spec (section 5.2)...

> gdb/ChangeLog:
> 
> 	* dwarf2read.c (struct dwarf2_cu): Add producer_is_icc field.
> 	(producer_is_icc): New function.
> 	(check_producer): Set producer_is_icc field on dwarf2_cu.
> 	(read_base_type): Spot ICC's unexpected void type, and convert to
> 	GDB's expected void type.
> 	(dwarf2_cu::dwarf2_cu): Initialise producer_is_icc field.
> 	* valprint.c (maybe_negate_by_bytes): Add an assertion that the
> 	LEN is greater than 0.

While I think this is reasonable (and non-invasive/safe), I have some questions.

> @@ -17548,7 +17565,11 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
>  	type = dwarf2_init_float_type (objfile, bits, name, name);
>  	break;
>        case DW_ATE_signed:
> -	type = init_integer_type (objfile, bits, 0, name);
> +	if (bits == 0 && producer_is_icc (cu)
> +	    && strcmp (name, "void") == 0)
> +	  type = objfile_type (objfile)->builtin_void;
> +	else
> +	  type = init_integer_type (objfile, bits, 0, name);
>  	break;
>        case DW_ATE_unsigned:
>  	if (cu->language == language_fortran

Is this the appropriate place for this? The patch is attempting to deal specifically
with the void return of a function. I would have thought to catch this anomaly in
read_func_scope/add_dwarf2_member_fn. These sorts of exceptions are handled
relatively commonly in dwarf2read.c.

While the DWARF specifications specifically call out using DW_TAG_unspecified_type
for the void type (in C/C++), it doesn't actually say that this particular usage
is invalid AFAICT. [Although I think it is.] Nonetheless, changing this here would
preclude some language from doing something like this (as silly as that sounds).

Also, if it this is the appropriate place (or even if it is decided to move this
check elsewhere), why limit this to ICC? Is it simply because ICC only handles
C/C++? Would it hurt/be worth it to safe guard that gcc or clang or rustc or
who-knows-what wouldn't cause us similar harm?

As for the actual code as it is, why only check DW_ATE_signed? I realize that this
is specifically to address the particular ICC instance you are trying to fix, but
something tells me that this could potentially change. Regardless of the encoding,
this would be invalid.

I'm not really asking for any changes. After looking at all kinds of DWARF output
that I never dreamed possible, my thoughts tend to defensive programming in the
DWARF reader.
 
Thanks,
Keith (IANAM*)

* I Am Not A Maintainer