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

  Doug Evans <xdje42@gmail.com> writes:
> "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).

I went through and played with adding gdbarch to the
symbol lookup routines. I pushed into some places that I didn't
I need to, just to see the effect.
I'll clean this up and resubmit in a bit.

This patch also allows block == NULL and gdbarch == NULL
in the call to lookup_symbol. Sometimes the caller doesn't have either
one and doesn't need either one. I'd prefer something else though:
It'd be cleaner to remove the choice and require the caller to use
routines that are more explicit.
Otherwise I suspect a subtle bug or two will creep in.
[IOW, if the lookup may be for an arch-specific symbol (e.g., primitive type)
then require the caller to use a routine that requires a non-NULL gdbarch.
Internal to symtab.c we can do whatever we want though.]

This patch also adds some const-correctness to gdbarch.*.
[It'll get split out of course.]

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

Commit Message

Patch