dwarf2-frame.c read_reg problems, again ...

dwarf2-frame.c read_reg problems, again ...

[Ulrich Weigand]

Dan,

the patch you committed to read_reg last year:
http://sourceware.org/ml/gdb-patches/2006-05/msg00089.html

in combination with the recent switch to XML register definitions,
appears to cause problems in ppc64 x ppc32 bi-arch debugging.

Specifically, when debugging a 32-bit inferior on a 64-bit
system, the DWARF-2 unwinder now sign-extends the CFA values
to 64-bit (as the stack addresses are typically in the 
0xf0000000 range, this is an actual problem).

As long as every CFA is treated the same, this is does not
really matter.  However, if there are "special" frames
(signal trampoline or dummy frames) in between, *their*
unwinders do *not* sign-extend the CFA.  This causes the
generic frame code to stop unwinding with "previous frame
inner to this frame" errors.

With the patch refered to above, read_reg will respect the
signedness of the register_type, if it is an integral type.
This is not a problem if the register type is a pointer type
(in which case pointer_to_address would be consulted), but
on ppc the CFA gets computed from regular general purpose
registers, with an integral register_type.

Those in turn used to be described as "builtin_type_uint32"
by the original rs6000_register_type.  The generic XML-based
machinery now apparently uses a signed integer type instead,
exposing the problem.


Now I'm wondering: what was the motivation behind using
unpack_long here?   The dwarf2loc.c:dwarf_expr_read_reg 
routine, which saves basically the same purpose, now uses
address_from_register -- i.e. specifically treats the
value as pointer, not integer ...

I guess we could change the XML definitions to force the
general purpose registers back to unsigned types, but that
doesn't seem a real solution to me.

Any suggestions how to fix this?


Thanks,
Ulrich


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

Re: dwarf2-frame.c read_reg problems, again ...

[Daniel Jacobowitz]

On Wed, Oct 31, 2007 at 02:51:29AM +0100, Ulrich Weigand wrote:
> With the patch refered to above, read_reg will respect the
> signedness of the register_type, if it is an integral type.
> This is not a problem if the register type is a pointer type
> (in which case pointer_to_address would be consulted), but
> on ppc the CFA gets computed from regular general purpose
> registers, with an integral register_type.
> 
> Those in turn used to be described as "builtin_type_uint32"
> by the original rs6000_register_type.  The generic XML-based
> machinery now apparently uses a signed integer type instead,
> exposing the problem.

This was plainly and simply a mistake.  While I agree that changing
them back is not a real solution to the problem you've found, I didn't
mean to flip the signedness of all those registers.  If uint32 is in
any sense more architecturally appropriate, or even for sheer
tradition, let's flip them back.

> Now I'm wondering: what was the motivation behind using
> unpack_long here?   The dwarf2loc.c:dwarf_expr_read_reg 
> routine, which saves basically the same purpose, now uses
> address_from_register -- i.e. specifically treats the
> value as pointer, not integer ...

I think we have about five too many ways to take a register and make
it into a number.  On the other hand, dwarf_expr_read_reg
uses builtin_type_void_data_ptr.  That is probably broken on
whatever target Michael Snyder was trying to fix in the patch
you referenced, where the sizes differ.

If we use address_from_register, we will end up in a call to
unpack_long using the provided type.  So I think that is exactly the
same as what we have now.

This is the trouble with using a host integer type to represent target
addresses.  If we did all our arithmetic on opaque CORE_ADDR's, this
wouldn't happen.  I wonder if there's no getting around the need to
define a sensible calculus for them...

-- 
Daniel Jacobowitz
CodeSourcery

Re: dwarf2-frame.c read_reg problems, again ...

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Wed, Oct 31, 2007 at 02:51:29AM +0100, Ulrich Weigand wrote:
> > Those in turn used to be described as "builtin_type_uint32"
> > by the original rs6000_register_type.  The generic XML-based
> > machinery now apparently uses a signed integer type instead,
> > exposing the problem.
> 
> This was plainly and simply a mistake.  While I agree that changing
> them back is not a real solution to the problem you've found, I didn't
> mean to flip the signedness of all those registers.  If uint32 is in
> any sense more architecturally appropriate, or even for sheer
> tradition, let's flip them back.

I don't see how either way would be architecturally more appropriate
than the other, but it look like Andrew specifically made them 
unsigned to fix what appears to be a similar problem:
http://sourceware.org/ml/gdb-patches/2004-03/msg00176.html

So I don't mind making them unsigned again.  How would that work
with the XML definitions?  Do you have to add an explicit type
attribute everywhere?

> > Now I'm wondering: what was the motivation behind using
> > unpack_long here?   The dwarf2loc.c:dwarf_expr_read_reg 
> > routine, which saves basically the same purpose, now uses
> > address_from_register -- i.e. specifically treats the
> > value as pointer, not integer ...
> 
> I think we have about five too many ways to take a register and make
> it into a number.  On the other hand, dwarf_expr_read_reg
> uses builtin_type_void_data_ptr.  That is probably broken on
> whatever target Michael Snyder was trying to fix in the patch
> you referenced, where the sizes differ.

I'd say targets like that should be able to define a proper conversion to
builtin_type_void_data_ptr via convert_register or value_from_register
for that specific register.

Alternatively, if builtin_type_void_data_ptr is in fact the wrong
type to describe a CFA on some weird platform, we could allow the
gdbarch to define the type to be used for this.
 
> If we use address_from_register, we will end up in a call to
> unpack_long using the provided type.  So I think that is exactly the
> same as what we have now.

Well, but the provided type is a pointer type, not an integer type,
so we'll do the proper conversion via pointer_to_address, and not
rely on the signednesss of the (integral) type.


I think address_from_register (builtin_type_void_data_ptr, ...)
*should* really do the right thing conceptually.  Of course, the
problem with that function is that is requires the ("this") frame
where the register resides, but in the dwarf2-frame.c routine we're
still unwinding, so we only have the "next" frame available.  Maybe
your "lazy" frame unwinding approach would solve this.


> This is the trouble with using a host integer type to represent target
> addresses.  If we did all our arithmetic on opaque CORE_ADDR's, this
> wouldn't happen.  I wonder if there's no getting around the need to
> define a sensible calculus for them...

Hmmm, an opaque CORE_ADDR might be nice, e.g. to at some point
implement multiple address spaces.  On the other hand, having
target-specific semantics of CORE_ADDR would also complicate
a multi-target build of GDB ...

Bye,
Ulrich

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

Re: dwarf2-frame.c read_reg problems, again ...

[Daniel Jacobowitz]

On Wed, Oct 31, 2007 at 07:37:03PM +0100, Ulrich Weigand wrote:
> So I don't mind making them unsigned again.  How would that work
> with the XML definitions?  Do you have to add an explicit type
> attribute everywhere?

That's right.  The default is "int", which is signed.

When I was designing the format I thought up a pile of clever ways to
make the format easier to write and more concise, like repeating
register sets.  Of course, they also make it harder to parse and
more complicated to document.  So I haven't implemented any of them
:-)

> I'd say targets like that should be able to define a proper conversion to
> builtin_type_void_data_ptr via convert_register or value_from_register
> for that specific register.
> 
> Alternatively, if builtin_type_void_data_ptr is in fact the wrong
> type to describe a CFA on some weird platform, we could allow the
> gdbarch to define the type to be used for this.

That's an interesting idea...

I don't have a helpful opinion about this particular case, I'm afraid.
I just don't want to break whatever Michael was working with, or
MIPS again (which requires sign extension here).

-- 
Daniel Jacobowitz
CodeSourcery