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

["Ulrich Weigand" <uweigand@de.ibm.com>]

Doug Evans wrote:
> > 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.

Thanks!

> 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.]

Absolutely.  I agree that it would be preferable to define the
interface so that if you want to enable looking up primitive types,
you need to pass in an gdbarch explicitly.

Conversely, many places know for sure they do not need primitive types
(e.g. because they are looking up a variable or function name, or they
are looking up some non-primitive type like a struct type), and those
should preferably pass in a NULL gdbarch.  (Certainly preferable to
using some random arch from target_gdbarch or get_current_arch ...)

As an aside, I'm not really happy with the use of current_language
in lookup_symbol.  Ideally, this is one more of those globals we'd
really better get rid of.  (In particular, since many callers actually
do know which language they're operating on.)

I'd prefer an interface where lookup_symbol uses language_unknown,
i.e. does not perform any language-specific lookup (which should
actually be fine for many of the current users).  If you do want
do perform language-specific lookup, you should pass in the language
explicitly.

(Just mentioning this now since you're already going through all
users :-))

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

This is just for the variable accessors and predicates, right?
I guess that makes sense.   Not sure if it really makes much
of a difference since most callers will still need to call one
of the other routines ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com