Re: Cell multi-arch symbol lookup broken (Re: [PATCH] solib_global_lookup: Fetch arch from objfile.)

[Doug Evans <xdje42@gmail.com>]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> Doug Evans wrote:
>
>> So I get now that there is an inferior gdbarch that is potentially
>> distinct from whatever gdbarch one has at hand (e.g., an objfile's),
>> [that's the easy part]
>> and that certain API calls *have* to use the inferior gdbarch.
>> [that's the subtle part]
>> 
>> One question that comes to mind is why don't the other gdbarches
>> in the system, e.g., the spu's, delegate these calls to its "parent" gdbarch?
>> Any design that requires global state like this is suboptimal,
>> and I'd like to see gdb move away from it wherever possible.
>> [Here we have a gdbarch from the objfile, but we can't use it.]
>> 
>> Another thought that comes to mind is, if we don't want "child gdbarches"
>> to delegate to "parent gdbarches" (and I haven't decided myself), then
>> another way to go is to use different types.
>> E.g., inferior_gdbarch and gdbarch.
>> [I might name inferior_gdbarch differently, depending on what API calls
>> it contains, but this would be another way to avoid potential confusion
>> over what the correct gdbarch to use is in any particular situation.]
>
> Well, it's a bit more subtle, even.  First of all, there's two distinct
> "classes" of gdbarch structures, corresponding to the "symbol side" and
> the "target side" of GDB, respectively.  On the symbol side, we have all
> the information that is determined solely by looking at an objfile and
> well-known ABI conventions.  On the target side, we have in addition all
> the information that is determined by the running target (e.g. register
> names etc.).  For more details, see here:
> https://sourceware.org/ml/gdb-patches/2007-12/msg00142.html

Thanks for the reference.

> The "symbol side" gdbarch is used as the architecture associated with
> objfiles, types, and symbols.  The "target side" gdbarch is used as
> the architecture associated with the inferior as a whole, with a
> thread, or with a stack frame.
>
> Now, in both symbol side and target side, there may be different
> architecture instances active during a single debug session.  In
> particular, during Cell/B.E. debugging, there will be PowerPC
> objfiles and SPU objfiles.  In addition, some threads and/or stack
> frames will be of PowerPC architecture, and others of SPU architecture.
> The main inferior will be of PowerPC architecture (in mixed debugging)
> or of SPU architecture (when doing SPU stand-alone debugging).
> (With multi-inferior debugging, we may have different main inferior
> architectures at the same time as well.)

This much I get. :-)

> Now, I tend to agree that there should be some sort of child/parent
> relationship between a symbol-side gdbarch and a target-side gdbarch.
> Specifically, a target-side gdbarch should *contain* a symbol-side
> gdbarch *and additional information*.

... and we should make them separate types.

> However, this should still
> apply only to the same basic architecture, i.e. the PowerPC target
> gdbarch should include the PowerPC symbol gdbarch, and likewise
> for SPU.  [ I originally wanted to implement this as two distinct
> data types, but this will require a significant amount of refactoring
> work, and in the end I never got around to doing this. ]

Heh. Good to know.
[And I'm not complaining btw.]

> But I don't think there should be a direct relationship between
> the SPU arch and the PowerPC arch, as you describe above.
> Execution may be in a PowerPC frame on an SPU thread running
> in a PowerPC inferior ... but there's no relationship between
> the architectures as such.  When using any of these, you just
> have to know whether you're interested in a property of the
> current frame, current thread, or current inferior.

Well, I wouldn't read too much into my suggestion.
It was pragmatic and pragmatism has tradeoffs by definition.
[For reference sake, I was envisioning setting up the
hierarchy at runtime when we knew what kind of hierarchy
was actually present. As for *which* architecture to ultimately use
that problem is still up to the user/caller.]

> Now, the "target_gdbarch" is a bit of a special case.  It used
> to have a distinct semantics: the architecture used at the
> target interface level.  (Think of it as the architecture that
> defines the contents of the register packets of the remote
> gdbserver protocol.)  However, since the remote protocol was
> extended to actually support using *multiple* different
> architectures, there's no longer a need for target_gdbarch
> as a distinct concept.  And in fact, it's now simply defined
> as the architecture of the current inferior:
>
> struct gdbarch *
> target_gdbarch (void)
> {
>   return current_inferior ()->gdbarch;
> }
>
> I'd be in favor of inlining this function into every user,
> and starting to replace "current_inferior" with any given
> inferior that we may be actually operating on in these
> places.

I'm all for removal of references to global state (where appropriate)
and passing in the needed context.
IWBN to also clean up the types while we're at it.

>> I'm ok with reverting this particular part of the patch,
>> though it'd be helpful to add a comment explaining
>> why things are the way they are.
>
> There is a comment in gdbarch.h, but that may not be in
> a prominent enough place:
>
> /* The architecture associated with the inferior through the
>    connection to the target.
>
>    The architecture vector provides some information that is really a
>    property of the inferior, accessed through a particular target:
>    ptrace operations; the layout of certain RSP packets; the solib_ops
>    vector; etc.  To differentiate architecture accesses to
>    per-inferior/target properties from
>    per-thread/per-frame/per-objfile properties, accesses to
>    per-inferior/target properties should be made through this
>    gdbarch.  */

Yeah, I found that, but too late.
I was suggesting putting a comment at the call site.

[While putting such comments at all such call sites has the potential to
reduce readability, not improve it (if taken to an extreme :-)); until we
use types/APIs that make the code self-documenting, I like the comments.]

> Maybe one way to make this obvious would be to change solib_ops
> to take an inferior instead of a gdbarch as argument ...

In the particular case of solib_ops it does seem like
gdbarch is the wrong "this/self" parameter.

Thanks.