Re: [rfa] frame address size incorrect if address size != ptr size

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

Corinna Vinschen wrote:
> On Aug  5 12:04, Ulrich Weigand wrote:
> > For .eh_frame, on the other hand, there is no standard.  On some platforms,
> > GCC now chooses to produce data that looks like a variant of DWARF CFI,
> > but there are certain differences.  In particular, it uses different
> > encodings to represent addresses in the those FDE/CFI fields mentioned
> > above; for example they may in fact be represented as offsets relative to
> > the text section or the eh_frame section itself.  Depending on the particular
> > encoding, the size of the data used to encode addresses is either hard-coded,
> > or is equal to the size of a *pointer* (i.e. GCC uses POINTER_SIZE to
> > represent this value, not DWARF2_ADDR_SIZE).
> 
> Which is a bug, IMHO, even if this only potentially affects targets
> where address and pointer size differ.
> 
> The description at
> http://refspecs.freestandards.org/LSB_3.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html
> just contains something like
> 
>   "PC Begin
> 
>      An encoded constant that indicates the address of the initial
>      location associated with this FDE."
> 
> To indicate the address, the compiler has to use the address size
> for these values.  It can't rely on the size of pointers.

Well, whatever it is, it is part of the ABI.  There is unwinder code that
reads the contents of .eh_frame at runtime during exception handling, so
the meaning of those encodings cannot be changed at this point.

And in fact, looking at that code, if the DW_EH_PE_absptr encoding is
used, the unwinder simply uses the contents of .eh_frame immediately
as *pointer* -- therefore its size *must* be the size of a pointer
(and its contents must be whatever in-memory contents of pointers are).

> > To capture this difference, the GDB dwarf2-frame.c code sets the
> > cie->addr_size field accordingly.  Looking at this code more carefully,
> > the usage of this field may actually not be fully correct, because it
> > is used both to pass to read_encoded_value (which needs the address size
> > in .debug_frame, but the pointer size in .eh_frame sections), *and* to
> > pass to dwarf_expr_eval when evaluating complex expressions (which *always*
> > needs the address size, even in .eh_frame sections).
> 
> Yeah, as I said, in theory the .eh_frame sections have to use the
> address size, otherwise .eh_frame sections will never be useful
> for small targets as the below mentioned ones.

As I said, that's not the case: they can use DW_EH_PE_absptr to create
any pointer contents that a pointer can hold.  If they need something
else, they can use different encodings (e.g. offset relative to the
text section), and those can be of an arbitrarily chosen size.  (For
example, many of the typical 64-bit platforms still use a 4-byte
relative encoding in .eh_frame in order to save space.)

> So, right now evaluating .debug_frame sections is broken for at least
> avr, m32c/16, and xstormy16.

... and possibly mips EABI64.

> > I'm not completely sure how to proceed here.  One way out could
> > certainly be to remove the overloaded semantics of addr_bit by
> > simply adding a *new* gdbarch callback gdbarch_dwarf2_addr_size
> > which encodes exactly what this target uses as address size
> > in .dwarf_frame sections (i.e. always equal to GCC's DWARF2_ADDR_SIZE
> > setting).
> > 
> > I'd appreciate further comments / suggestions on this issue ...
> 
> I think that's a good idea.  However, for targets which don't define
> gdbarch_dwarf2_addr_size, it's still necessary to assume a useful
> default.
> 
> And then there's the important hint from the comment in dwarf2-frame.c:
> 
>   "For .debug_frame FDEs, this is supposed to be the target address size
>    from the associated CU header."
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Doesn't that mean we should better try to check for this value in the
> first place?  I'm not fluent in GDBs dwarf2 code, but is that really
> *that* tricky?

The problem is that it *is* tricky; otherwise DWARF4 wouldn't have
added the new field.  We'd need to find the "associated" .debug_info
sections -- but there is no link between .debug_frame and any of the
other .debug sections; you'd basically have to fall back to the
address and search for a .debug_info matching that address.  But
then questions arise: what if there is no .debug_info covering
that same PC range?

> If not, I would prefer a solution like this:
> 
>   - If version > 4, use addr_size from .debug_frame section
>   - Otherwise, if we can fetch the target address size from the CU
>     header, use it.
>   - Otherwise, if the target defined gdbarch_dwarf2_addr_size, use it.
>   - Otherwise, default to gdbarch_addr_bit for .debug_frame sections
>     and to gdbarch_ptr_bit for .eh_frame sections.

As I said, finding the .debug_info may be difficult.  Also, I'd really
avoid getting another dependency on gdbarch_addr_bit in there; the point
of having a new callback is exactly to avoid overloading addr_bit with
more and more (possibly) different meanings.

I'd rather just have gdbarch_dwarf2_addr_size default unconditionally
to gdbarch_ptr_bit.  In dwarf2-frame we'd then simply use the embedded
addr_size if version >= 4, and gdbarch_dwarf2_addr_size otherwise.
Platforms where ptr_bit is not appropriate simply need to define
gdbarch_dwarf2_addr_size -- since this list is very short, and defining
gdbarch_dwarf2_addr_size correctly is very simple (you just need to look
at the definition of DWARF2_ADDR_SIZE in the corresponding GCC back-end),
that doesn't seem like an unreasonable restriction to me ...

> > As a side note, it seems odd that add_size is set in those two
> > different locations here.  The first one is always overwritten
> > by the second one anyway, isn't it?
> 
> There's an early return statement after checking the version number.
> That indicates a failure anyway, so it might be ok to set addr_size
> only once, at the second spot (lines 1779ff).

Yes, that sounds right to me.

Bye,
Ulrich

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