Re: [RFA] fetch result of locdesc expressions as integer (not address)

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Joel Brobecker wrote:
> > Well, maybe they should, but right now they don't, and neither does
> > your patch add any such correction.  The point I was trying to make
> > is that therefore, your patch as it is, while fixing one class of
> > bugs on some targets, may simultaneously introduce a different class
> > of bugs on other targets.  I'm not sure this is a good idea ...
> 
> I understand, but I'm just restoring the previous behavior which,
> I believe was changed unintentionally.

Well, there *is* a change compared to the previous behavior (i.e. before
Jan's patch: http://sourceware.org/ml/gdb-patches/2011-07/msg00762.html).

Consider the typical case for DW_AT_location for a global variable:
just a single DW_OP_addr statement.  This used to be handled previously
via a call to read_address.  Note that read_address attempts to perform
target-specific conversions on the address, in particular it will be
sign-extended if required.

The dwarf_expr_eval machinery, on the other hand, does not use read_address
or otherwise attempt to sign-extend DW_OP_addr addresses.  The thought here
is that performing that step at this point is premature, since subsequent
DWARF operations (e.g. to enforce alignment) may destroy the effect of such
extensions.  Instead, the sign-extension is performed at the very end of
expression evaluation, via the architecture's integer_to_address routine,
if required.

So for example on mips, before Jan's patch, the address of a global variable
would have been sign-extended (in read_address); after Jan's patch, it would
*still* have been sign-extended (via integer_to_address); but after your
patch, it will no longer be.

> It's working fine for instance
> on AVR, where the distinction makes a difference, so we're not doing
> so bad.  Additionally, I tested this patch against the testsuite
> which does include a test for the problem that Jan wanted to fix.

Hmm, looking into this a bit more, I think I understand why this change
doesn't affect testing.  The only caller of decode_locdesc that actually
expects to handle full location descriptions returning an address is
add_partial_symbol when handling a DW_TAG_variable.  However, the address
of a *partial* symbol for a *variable* (as opposed to a function) doesn't
appear to be used for anything; and in fact there is even a comment to
that effect ...

Given that, I withdraw my objection to the patch.  It would still be
preferable IMO to no longer call decode_locdesc from add_partial_symbol
(but instead just handle those cases that this routine cares about,
along the lines of what var_decode_location does).  But that can then
be a follow-on patch.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com