Re: Cell multi-arch type resolution broken (Re: [PATCH 5/6] [PR 17684] add support for primitive types as symbols)

[Doug Evans <xdje42 at gmail dot com>]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> Doug Evans wrote:
>
>> 2014-12-18  Doug Evans  <xdje42@gmail.com>
>> 
>>	* gdbtypes.c (lookup_typename): Call lookup_symbol_in_language.
>>	Remove call to language_lookup_primitive_type.  No longer necessary.
>
>> 	(basic_lookup_symbol_nonlocal): Try looking up the symbol as a
>> 	primitive type.
>
> This seems to have broken per-architecture primitive type resolution for
> Cell multi-arch debugging.  The problem here is that some primitive types
> have different properties on SPU than on PowerPC, and so you want e.g.
>   print sizeof (long double)
> to print 16 while in a PowerPC frame, but print 8 in a SPU frame.
>
> This used to be triggered by the explicit gdbarch argument that was passed
> to the language_typename routine (and related).  But after your patch, that
> routine is either no longer called at all, and even where it is, its
> gdbarch argument to language_typename is now simply ignored.
>
> Instead, we have this code in symtab.c:basic_lookup_symbol_nonlocal:
>
>> +  /* If we didn't find a definition for a builtin type in the static block,
>> +     search for it now.  This is actually the right thing to do and can be
>> +     a massive performance win.  E.g., when debugging a program with lots of
>> +     shared libraries we could search all of them only to find out the
>> +     builtin type isn't defined in any of them.  This is common for types
>> +     like "void".  */
>> +  if (domain == VAR_DOMAIN)
>> +    {
>> +      struct gdbarch *gdbarch;
>> +
>> +      if (block == NULL)
>> +	gdbarch = target_gdbarch ();
>> +      else
>> +	gdbarch = block_gdbarch (block);
>> +      sym = language_lookup_primitive_type_as_symbol (langdef, gdbarch, name);
>> +      if (sym != NULL)
>> +	return sym;
>> +    }
>
> which just uses target_gdbarch.  This is wrong just about always in symbol-
> related code, and on Cell multi-arch debugging it in effect always uses the
> PowerPC architecture even while in a SPU frame.
>
> Note that while sometime the block architecture is used, this doesn't really
> help here, since lookup_typename is nearly always called with a NULL block.
>
> As a quick fix to get Cell going again, the appended patch works for me
> (by using get_current_arch () instead of target_gdbarch ()).  But this
> isn't a real fix either.   I guess we should either:
>
> - Pass the intended target architecture alongside the intended language
>   throughout the symbol resolution stack, or ...
>
> - Make sure we always have a current block when calling lookup_typename
>
> (Note that latter still isn't quite the same: e.g. when debugging code
> without debug info, or code outside any objfile, we can never have a
> current block; but we can still have a proper architecture detected
> at runtime for the current frame.)
>
> Any thoughts on this?

I'd be ok with adding a gdbarch parameter to lookup_symbol,
and require at least one of block or gdbarch to be non-NULL.

The symbol lookup code is a lot simpler when block == NULL,
and handling all the different cases in one set of functions
makes things more complex than they could otherwise be.
One might then split things up into two paths underneath
(one for block, one for arch).

> Index: binutils-gdb/gdb/ada-lang.c
> ===================================================================
> --- binutils-gdb.orig/gdb/ada-lang.c
> +++ binutils-gdb/gdb/ada-lang.c
> @@ -5792,10 +5792,11 @@ ada_lookup_symbol_nonlocal (const struct
>  
>    if (domain == VAR_DOMAIN)
>      {
> +      /* FIXME: gdbarch should be passed by the caller.  */
>        struct gdbarch *gdbarch;
>  
>        if (block == NULL)
> -	gdbarch = target_gdbarch ();
> +	gdbarch = get_current_arch ();
>        else
>  	gdbarch = block_gdbarch (block);
>        sym.symbol = language_lookup_primitive_type_as_symbol (langdef, gdbarch, name);
> Index: binutils-gdb/gdb/symtab.c
> ===================================================================
> --- binutils-gdb.orig/gdb/symtab.c
> +++ binutils-gdb/gdb/symtab.c
> @@ -41,6 +41,7 @@
>  #include "p-lang.h"
>  #include "addrmap.h"
>  #include "cli/cli-utils.h"
> +#include "arch-utils.h"
>  
>  #include "hashtab.h"
>  
> @@ -2531,10 +2532,11 @@ basic_lookup_symbol_nonlocal (const stru
>       like "void".  */
>    if (domain == VAR_DOMAIN)
>      {
> +      /* FIXME: gdbarch should be passed by the caller.  */
>        struct gdbarch *gdbarch;
>  
>        if (block == NULL)
> -	gdbarch = target_gdbarch ();
> +	gdbarch = get_current_arch ();
>        else
>  	gdbarch = block_gdbarch (block);
>        result.symbol = language_lookup_primitive_type_as_symbol (langdef,

LGTM.
Thanks.

I ran the perf testsuite on this just as a sanity check,
and didn't find anything.