public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [rfa] frame address size incorrect if address size != ptr size
@ 2010-07-26 14:53 Corinna Vinschen
  2010-08-04 11:35 ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-07-26 14:53 UTC (permalink / raw)
  To: gdb-patches

Hi,

while analyzing some problem in XStormy16, I came across a weird effect.
Even though the dwarf2 frame unwinder is suppose to be preferred over
the xstormy16 unwinder, the code almost always called the
xstormy16_frame_prev_register function, rather than the
dwarf2_frame_prev_register function.

Debugging turned up that the "address_range" entry in almost all fde's
was incorrect, so the dwarf2_frame_find_fde function almost never found
a matching fde.

More debugging then pointed to the decode_frame_entry_1 function.  The
storage size of address_range is supposed to be cie->addr_size.

cie->addr_size is computed like this:

  /* The target address size.  For .eh_frame FDEs this is considered
     equal to the size of a target pointer.  For .dwarf_frame FDEs, 
     this is supposed to be the target address size from the associated
     CU header.  FIXME: We do not have a good way to determine the 
     latter.  Always use the target pointer size for now.  */
  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;

  [...]

  if (cie->version >= 4)
    {
      /* FIXME: check that this is the same as from the CU header.  */
      cie->addr_size = read_1_byte (unit->abfd, buf);
      ++buf;
      cie->segment_size = read_1_byte (unit->abfd, buf);
      ++buf;
    }
  else
    {
      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
      cie->segment_size = 0;
    }
		  
This is not helpful for .dwarf_frame sections in case cie->version < 4.

The address size of a target is not necessarily the size of a pointer.
And that's exactly the problem in the XStormy16 case.  The target is
very size sensitive.  The address range of the CPU is potentially 32
bit, so the address size for the target is 32 bit.  However, the
pointer size for this target is deliberately set to 16 bit, which
allows for smaller code, and also pointers fit into a single 16 bit
register.

The effect is that the above statements set cie->addr_size to 2, because
that's the size of a pointer.  But the address size is 4 byte and the
debug info has to use the address size to be able to cover the entire
address space of the target.  Consequentially GCC emits debug info
using the address size of the XStormy16 target, 4 bytes.

Therefore I propose the below patch.  It continues to compute addr_size
from gdbarch_ptr_bit for .eh_frame sections, but uses gdbarch_addr_bit
in case of .dwarf_frame sections.

Tested with XStormy16.  Now the dwarf2 unwinder is actually preferred over
the "manual" xstormy16 unwinder, because the dwarf2_frame_find_fde
function actually finds matching FDEs now.

Ok to apply?

Thanks,
Corinna


	* dwarf2-frame.c (decode_frame_entry_1): If addr_size isn't available
	in CIE, use address size rather than pointer size when decoding
	.dwarf_frame info.


Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.114
diff -u -p -r1.114 dwarf2-frame.c
--- dwarf2-frame.c	7 Jul 2010 17:26:38 -0000	1.114
+++ dwarf2-frame.c	26 Jul 2010 14:49:34 -0000
@@ -1736,8 +1736,12 @@ decode_frame_entry_1 (struct comp_unit *
 	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
 	 this is supposed to be the target address size from the associated
 	 CU header.  FIXME: We do not have a good way to determine the 
-	 latter.  Always use the target pointer size for now.  */
-      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	 latter.  Always use the target address size for .dwarf_frame
+	 sections for now.  */
+      if (eh_frame_p)
+	cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+      else
+	cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
 
       /* We'll determine the final value later, but we need to
 	 initialize it conservatively.  */
@@ -1779,7 +1783,10 @@ decode_frame_entry_1 (struct comp_unit *
 	}
       else
 	{
-	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	  if (eh_frame_p)
+	    cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	  else
+	    cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
 	  cie->segment_size = 0;
 	}
 


-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-07-26 14:53 [rfa] frame address size incorrect if address size != ptr size Corinna Vinschen
@ 2010-08-04 11:35 ` Corinna Vinschen
  2010-08-04 22:40   ` Jan Kratochvil
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-04 11:35 UTC (permalink / raw)
  To: gdb-patches

Ping?  This affects generic code in dwarf2-frame.c...

On Jul 26 16:52, Corinna Vinschen wrote:
> Hi,
> 
> while analyzing some problem in XStormy16, I came across a weird effect.
> Even though the dwarf2 frame unwinder is suppose to be preferred over
> the xstormy16 unwinder, the code almost always called the
> xstormy16_frame_prev_register function, rather than the
> dwarf2_frame_prev_register function.
> 
> Debugging turned up that the "address_range" entry in almost all fde's
> was incorrect, so the dwarf2_frame_find_fde function almost never found
> a matching fde.
> 
> More debugging then pointed to the decode_frame_entry_1 function.  The
> storage size of address_range is supposed to be cie->addr_size.
> 
> cie->addr_size is computed like this:
> 
>   /* The target address size.  For .eh_frame FDEs this is considered
>      equal to the size of a target pointer.  For .dwarf_frame FDEs, 
>      this is supposed to be the target address size from the associated
>      CU header.  FIXME: We do not have a good way to determine the 
>      latter.  Always use the target pointer size for now.  */
>   cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> 
>   [...]
> 
>   if (cie->version >= 4)
>     {
>       /* FIXME: check that this is the same as from the CU header.  */
>       cie->addr_size = read_1_byte (unit->abfd, buf);
>       ++buf;
>       cie->segment_size = read_1_byte (unit->abfd, buf);
>       ++buf;
>     }
>   else
>     {
>       cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
>       cie->segment_size = 0;
>     }
> 		  
> This is not helpful for .dwarf_frame sections in case cie->version < 4.
> 
> The address size of a target is not necessarily the size of a pointer.
> And that's exactly the problem in the XStormy16 case.  The target is
> very size sensitive.  The address range of the CPU is potentially 32
> bit, so the address size for the target is 32 bit.  However, the
> pointer size for this target is deliberately set to 16 bit, which
> allows for smaller code, and also pointers fit into a single 16 bit
> register.
> 
> The effect is that the above statements set cie->addr_size to 2, because
> that's the size of a pointer.  But the address size is 4 byte and the
> debug info has to use the address size to be able to cover the entire
> address space of the target.  Consequentially GCC emits debug info
> using the address size of the XStormy16 target, 4 bytes.
> 
> Therefore I propose the below patch.  It continues to compute addr_size
> from gdbarch_ptr_bit for .eh_frame sections, but uses gdbarch_addr_bit
> in case of .dwarf_frame sections.
> 
> Tested with XStormy16.  Now the dwarf2 unwinder is actually preferred over
> the "manual" xstormy16 unwinder, because the dwarf2_frame_find_fde
> function actually finds matching FDEs now.
> 
> Ok to apply?
> 
> Thanks,
> Corinna
> 
> 
> 	* dwarf2-frame.c (decode_frame_entry_1): If addr_size isn't available
> 	in CIE, use address size rather than pointer size when decoding
> 	.dwarf_frame info.
> 
> 
> Index: dwarf2-frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 dwarf2-frame.c
> --- dwarf2-frame.c	7 Jul 2010 17:26:38 -0000	1.114
> +++ dwarf2-frame.c	26 Jul 2010 14:49:34 -0000
> @@ -1736,8 +1736,12 @@ decode_frame_entry_1 (struct comp_unit *
>  	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
>  	 this is supposed to be the target address size from the associated
>  	 CU header.  FIXME: We do not have a good way to determine the 
> -	 latter.  Always use the target pointer size for now.  */
> -      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +	 latter.  Always use the target address size for .dwarf_frame
> +	 sections for now.  */
> +      if (eh_frame_p)
> +	cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +      else
> +	cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
>  
>        /* We'll determine the final value later, but we need to
>  	 initialize it conservatively.  */
> @@ -1779,7 +1783,10 @@ decode_frame_entry_1 (struct comp_unit *
>  	}
>        else
>  	{
> -	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +	  if (eh_frame_p)
> +	    cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +	  else
> +	    cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
>  	  cie->segment_size = 0;
>  	}

Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-04 11:35 ` Corinna Vinschen
@ 2010-08-04 22:40   ` Jan Kratochvil
  2010-08-05  8:06     ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Jan Kratochvil @ 2010-08-04 22:40 UTC (permalink / raw)
  To: gdb-patches

On Wed, 04 Aug 2010 13:35:01 +0200, Corinna Vinschen wrote:
> Ping?  This affects generic code in dwarf2-frame.c...
> 
> On Jul 26 16:52, Corinna Vinschen wrote:
[...]
>   /* The target address size.  For .eh_frame FDEs this is considered
>      equal to the size of a target pointer.  For .dwarf_frame FDEs,
                                                   ^^^^^^^^^^^^ = .debug_frame
>      this is supposed to be the target address size from the associated
>      CU header.  FIXME: We do not have a good way to determine the
>      latter.  Always use the target pointer size for now.  */
>   cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
[...]
> Therefore I propose the below patch.  It continues to compute addr_size
> from gdbarch_ptr_bit for .eh_frame sections, but uses gdbarch_addr_bit
> in case of .dwarf_frame sections.
             ^^^^^^^^^^^^ = .debug_frame
             
I find the GDB comment above to be obsolete now.  FSF GCC (at least 4.4+ at
least x86_64-linux) no longer generates .debug_frame sections (unless you
explicitly use -fno-asynchronous-unwind-tables) as generated .eh_frame
sections already contains all the needed info.

I have not built gcc for the xstrormy16 target to check it more.  Still
I believe .eh_frame and .dwarf_frame address size should be treated the same.


Regards,
Jan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-04 22:40   ` Jan Kratochvil
@ 2010-08-05  8:06     ` Corinna Vinschen
  2010-08-05 10:04       ` Ulrich Weigand
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-05  8:06 UTC (permalink / raw)
  To: gdb-patches

On Aug  5 00:39, Jan Kratochvil wrote:
> On Wed, 04 Aug 2010 13:35:01 +0200, Corinna Vinschen wrote:
> > Ping?  This affects generic code in dwarf2-frame.c...
> > 
> > On Jul 26 16:52, Corinna Vinschen wrote:
> [...]
> >   /* The target address size.  For .eh_frame FDEs this is considered
> >      equal to the size of a target pointer.  For .dwarf_frame FDEs,
>                                                    ^^^^^^^^^^^^ = .debug_frame
> >      this is supposed to be the target address size from the associated
> >      CU header.  FIXME: We do not have a good way to determine the
> >      latter.  Always use the target pointer size for now.  */
> >   cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> [...]
> > Therefore I propose the below patch.  It continues to compute addr_size
> > from gdbarch_ptr_bit for .eh_frame sections, but uses gdbarch_addr_bit
> > in case of .dwarf_frame sections.
>              ^^^^^^^^^^^^ = .debug_frame
>              
> I find the GDB comment above to be obsolete now.  FSF GCC (at least 4.4+ at
> least x86_64-linux) no longer generates .debug_frame sections (unless you
> explicitly use -fno-asynchronous-unwind-tables) as generated .eh_frame
> sections already contains all the needed info.

I tested this with a linux->XStormy16 cross gcc 4.4.3, and it does not
generate .eh_frame sections, but .debug_frame sections.  The cie version
is 1, so the address size can't be fetched from the debug info.  That's
the case I'm trying to fix.

Besides, there are still a lot of installations using older compiler
versions so the comment is still useful, isn't it?

> I have not built gcc for the xstrormy16 target to check it more.  Still
> I believe .eh_frame and .dwarf_frame address size should be treated the same.

That would be fine with me.

I made the difference because the comment explicitely states that
.eh_frame sections are defined to use the target *pointer* size and
.dwarf_frame (sic) sections are using the *address* size as given in
the CU header.  This address size is exactly what is defined by
gdbarch_addr_bit().

So, the alternative would be to assume that the address size is always
correct and so is the one generated by the compiler, rather than the
pointer size.  This does not affect any target with pointer size ==
address size anyway, nor does it affect targets using a compiler
generating CIEs with a version >= 4.

The alternative patch would be:

	* dwarf2-frame.c (decode_frame_entry_1): Fix typo in comment.
	Assume target address size rather than target pointer size
	to cover small targets.

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.114
diff -u -p -r1.114 dwarf2-frame.c
--- dwarf2-frame.c	7 Jul 2010 17:26:38 -0000	1.114
+++ dwarf2-frame.c	5 Aug 2010 07:48:07 -0000
@@ -1733,11 +1733,11 @@ decode_frame_entry_1 (struct comp_unit *
       cie->encoding = DW_EH_PE_absptr;
 
       /* The target address size.  For .eh_frame FDEs this is considered
-	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
+	 equal to the size of a target pointer.  For .debug_frame FDEs, 
 	 this is supposed to be the target address size from the associated
 	 CU header.  FIXME: We do not have a good way to determine the 
-	 latter.  Always use the target pointer size for now.  */
-      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	 latter.  Always use the target address size for now.  */
+      cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
 
       /* We'll determine the final value later, but we need to
 	 initialize it conservatively.  */
@@ -1779,7 +1779,7 @@ decode_frame_entry_1 (struct comp_unit *
 	}
       else
 	{
-	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	  cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
 	  cie->segment_size = 0;
 	}
 

Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05  8:06     ` Corinna Vinschen
@ 2010-08-05 10:04       ` Ulrich Weigand
  2010-08-05 12:30         ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-05 10:04 UTC (permalink / raw)
  To: gdb-patches

Corinna Vinschen wrote:
> On Aug  5 00:39, Jan Kratochvil wrote:
> > I have not built gcc for the xstrormy16 target to check it more.  Still
> > I believe .eh_frame and .dwarf_frame address size should be treated the same.

That is actually not the case; the comment in GDB is correct.

For .debug_frame, the standard refers to "the size of an address on the
target architecture", and uses this value to define sizes of several
FDE/CFI fields holding address values, without clearly specifying what
that value means.  This has now been clarified in version 4, but for
older versions you simply have to know what the appropriate value for a
given architecture is (i.e. what value the compiler has chosen to use).

GCC defines a target macro DWARF2_ADDR_SIZE to provide this value;
it defaults to the size of a pointer (POINTER_SIZE), but can (and is)
overridden by a target back-end to some other explicit value.

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

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

> I made the difference because the comment explicitely states that
> .eh_frame sections are defined to use the target *pointer* size and
> .dwarf_frame (sic) sections are using the *address* size as given in
> the CU header.

Indeed, see above.

> This address size is exactly what is defined by gdbarch_addr_bit().

This, however, is not so clear to me.  gdbarch_addr_bit defines the size
of an address *in GDB internal representation*, i.e. what the contents
of a CORE_ADDR value mean.  On some platforms, target addresses may be
encoded into GDB internal addresses in complex ways, e.g. for platforms
that use segments or different code/data address spaces.  It is not
obvious that the size of GDB internal addresses always agrees with what
GCC chooses as Dwarf address size.


Looking at the (few) platforms where GCC defines a DWARF2_ADDR_SIZE
that differs from the default, we have:

 avr:       POINTER_SIZE 16         /   DWARF2_ADDR_SIZE 4
 m32c:      POINTER_SIZE 16 or 32   /   DWARF2_ADDR_SIZE 4
 m68hc11:   POINTER_SIZE 16         /   DWARF2_ADDR_SIZE 4
 mips:      POINTER_SIZE 32 or 64   /   DWARF2_ADDR_SIZE 4 or 8  (**)
 xstormy16: POINTER_SIZE 16         /   DWARF2_ADDR_SIZE 4

(**) but those may not always match:
   "Note that the default POINTER_SIZE test is not appropriate for MIPS.
    EABI64 has 64-bit pointers but uses 32-bit ELF."

On the other hand, in GDB we have these platforms that set addr_bit
to a different value than ptr_bit:

 avr:       ptr_bit 16         /   addr_bit 32
 m32c:      ptr_bit 16 or 32   /   addr_bit 32   (**)
 m68hc11:   ptr_bit 16         /   addr_bit 16 or 32
 xstormy16: ptr_bit 16         /   addr_bit 32

(**) but the back-end would have prefered to use 24:
   "GCC uses 32 bits for addrs in the dwarf info, even though
    only 16/24 bits are used.  Setting addr_bit to 24 causes
    errors in reading the dwarf addresses."

So it seems of the five platforms, on three of them GDB's addr_bit
always agrees with GCC's DWARF2_ADDR_SIZE (even though on one of
them the match is forced), and on two of them they do not always
agree ...


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


>        /* The target address size.  For .eh_frame FDEs this is considered
> -	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
> +	 equal to the size of a target pointer.  For .debug_frame FDEs, 
>  	 this is supposed to be the target address size from the associated
>  	 CU header.  FIXME: We do not have a good way to determine the 
> -	 latter.  Always use the target pointer size for now.  */
> -      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +	 latter.  Always use the target address size for now.  */
> +      cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
>  
>        /* We'll determine the final value later, but we need to
>  	 initialize it conservatively.  */
> @@ -1779,7 +1779,7 @@ decode_frame_entry_1 (struct comp_unit *
>  	}
>        else
>  	{
> -	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +	  cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
>  	  cie->segment_size = 0;
>  	}

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?

Bye,
Ulrich

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 10:04       ` Ulrich Weigand
@ 2010-08-05 12:30         ` Corinna Vinschen
  2010-08-05 14:08           ` Ulrich Weigand
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-05 12:30 UTC (permalink / raw)
  To: gdb-patches

On Aug  5 12:04, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> > On Aug  5 00:39, Jan Kratochvil wrote:
> > > I have not built gcc for the xstrormy16 target to check it more.  Still
> > > I believe .eh_frame and .dwarf_frame address size should be treated the same.
> 
> That is actually not the case; the comment in GDB is correct.
> 
> For .debug_frame, the standard refers to "the size of an address on the
> target architecture", and uses this value to define sizes of several
> FDE/CFI fields holding address values, without clearly specifying what
> that value means.  This has now been clarified in version 4, but for
> older versions you simply have to know what the appropriate value for a
> given architecture is (i.e. what value the compiler has chosen to use).

I'm wondering if that shouldn't always be the address size, though.  The
pointer size could be any arbitrary size which fits a given scenario,
but the address size is what can safely be used to address the entire
address range of the target.  As far as I understand the address size.

> GCC defines a target macro DWARF2_ADDR_SIZE to provide this value;
> it defaults to the size of a pointer (POINTER_SIZE), but can (and is)
> overridden by a target back-end to some other explicit value.
> 
> 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.

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

> > I made the difference because the comment explicitely states that
> > .eh_frame sections are defined to use the target *pointer* size and
> > .dwarf_frame (sic) sections are using the *address* size as given in
> > the CU header.
> 
> Indeed, see above.
> 
> > This address size is exactly what is defined by gdbarch_addr_bit().
> 
> This, however, is not so clear to me.  gdbarch_addr_bit defines the size
> of an address *in GDB internal representation*, i.e. what the contents
> of a CORE_ADDR value mean.  On some platforms, target addresses may be
> encoded into GDB internal addresses in complex ways, e.g. for platforms
> that use segments or different code/data address spaces.  It is not
> obvious that the size of GDB internal addresses always agrees with what
> GCC chooses as Dwarf address size.
> 
> 
> Looking at the (few) platforms where GCC defines a DWARF2_ADDR_SIZE
> that differs from the default, we have:
> 
>  avr:       POINTER_SIZE 16         /   DWARF2_ADDR_SIZE 4
>  m32c:      POINTER_SIZE 16 or 32   /   DWARF2_ADDR_SIZE 4
>  m68hc11:   POINTER_SIZE 16         /   DWARF2_ADDR_SIZE 4
>  mips:      POINTER_SIZE 32 or 64   /   DWARF2_ADDR_SIZE 4 or 8  (**)
>  xstormy16: POINTER_SIZE 16         /   DWARF2_ADDR_SIZE 4
> 
> (**) but those may not always match:
>    "Note that the default POINTER_SIZE test is not appropriate for MIPS.
>     EABI64 has 64-bit pointers but uses 32-bit ELF."
> 
> On the other hand, in GDB we have these platforms that set addr_bit
> to a different value than ptr_bit:
> 
>  avr:       ptr_bit 16         /   addr_bit 32
>  m32c:      ptr_bit 16 or 32   /   addr_bit 32   (**)
>  m68hc11:   ptr_bit 16         /   addr_bit 16 or 32
>  xstormy16: ptr_bit 16         /   addr_bit 32
> 
> (**) but the back-end would have prefered to use 24:
>    "GCC uses 32 bits for addrs in the dwarf info, even though
>     only 16/24 bits are used.  Setting addr_bit to 24 causes
>     errors in reading the dwarf addresses."
> 
> So it seems of the five platforms, on three of them GDB's addr_bit
> always agrees with GCC's DWARF2_ADDR_SIZE (even though on one of
> them the match is forced), and on two of them they do not always
> agree ...

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

> 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?

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.

> >        /* The target address size.  For .eh_frame FDEs this is considered
> > -	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
> > +	 equal to the size of a target pointer.  For .debug_frame FDEs, 
> >  	 this is supposed to be the target address size from the associated
> >  	 CU header.  FIXME: We do not have a good way to determine the 
> > -	 latter.  Always use the target pointer size for now.  */
> > -      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> > +	 latter.  Always use the target address size for now.  */
> > +      cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
> >  
> >        /* We'll determine the final value later, but we need to
> >  	 initialize it conservatively.  */
> > @@ -1779,7 +1779,7 @@ decode_frame_entry_1 (struct comp_unit *
> >  	}
> >        else
> >  	{
> > -	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> > +	  cie->addr_size = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
> >  	  cie->segment_size = 0;
> >  	}
> 
> 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).


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 12:30         ` Corinna Vinschen
@ 2010-08-05 14:08           ` Ulrich Weigand
  2010-08-05 14:30             ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-05 14:08 UTC (permalink / raw)
  To: gdb-patches

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 14:08           ` Ulrich Weigand
@ 2010-08-05 14:30             ` Corinna Vinschen
  2010-08-05 14:59               ` Ulrich Weigand
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-05 14:30 UTC (permalink / raw)
  To: gdb-patches

On Aug  5 16:07, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> > [...]
> > 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.

Ok, I agree with all you say above.

I'm going to create a patch which defines and uses a new
gdbarch_dwarf2_addr_size function.  It will be defined as a variable
like this in gdbarch.sh:

  v:int:dwarf2_addr_size:::sizeof (void*):0:gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT:

Given that, and also given that I will remove the redundant setting of
cie->addr_size in decode_frame_entry_1, I have one question left.

What about that comment in decode_frame_entry_1?

If we only use either the V4 addr_size, or the gdbarch_dwarf2_addr_size
function, then the comment really doesn't make much sense anymore in that
spot.  I'm wondering if it should be moved to the gdbarch.sh file.  What
do you think?


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 14:30             ` Corinna Vinschen
@ 2010-08-05 14:59               ` Ulrich Weigand
  2010-08-05 15:30                 ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-05 14:59 UTC (permalink / raw)
  To: gdb-patches

Corinna Vinschen wrote:

> I'm going to create a patch which defines and uses a new
> gdbarch_dwarf2_addr_size function.  It will be defined as a variable
> like this in gdbarch.sh:
> 
>   v:int:dwarf2_addr_size:::sizeof (void*):0:gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT:

Looks good, thanks.

> Given that, and also given that I will remove the redundant setting of
> cie->addr_size in decode_frame_entry_1, I have one question left.
> 
> What about that comment in decode_frame_entry_1?
> 
> If we only use either the V4 addr_size, or the gdbarch_dwarf2_addr_size
> function, then the comment really doesn't make much sense anymore in that
> spot.  I'm wondering if it should be moved to the gdbarch.sh file.  What
> do you think?

I agree.  In fact, the comment in gdbarch.sh could even be expanded to
say that gdbarch_dwarf2_addr_size needs to be defined if and only if the
platform GCC back-end defines a non-default DWARF2_ADDR_SIZE (and is only
necessary if Dwarf versions < 4 need to be supported).

Bye,
Ulrich

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 14:59               ` Ulrich Weigand
@ 2010-08-05 15:30                 ` Corinna Vinschen
  2010-08-05 16:52                   ` Ulrich Weigand
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-05 15:30 UTC (permalink / raw)
  To: gdb-patches

On Aug  5 16:58, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> 
> > I'm going to create a patch which defines and uses a new
> > gdbarch_dwarf2_addr_size function.  It will be defined as a variable
> > like this in gdbarch.sh:
> > 
> >   v:int:dwarf2_addr_size:::sizeof (void*):0:gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT:
> 
> Looks good, thanks.
> 
> > Given that, and also given that I will remove the redundant setting of
> > cie->addr_size in decode_frame_entry_1, I have one question left.
> > 
> > What about that comment in decode_frame_entry_1?
> > 
> > If we only use either the V4 addr_size, or the gdbarch_dwarf2_addr_size
> > function, then the comment really doesn't make much sense anymore in that
> > spot.  I'm wondering if it should be moved to the gdbarch.sh file.  What
> > do you think?
> 
> I agree.  In fact, the comment in gdbarch.sh could even be expanded to
> say that gdbarch_dwarf2_addr_size needs to be defined if and only if the
> platform GCC back-end defines a non-default DWARF2_ADDR_SIZE (and is only
> necessary if Dwarf versions < 4 need to be supported).

Yes, that makes sense.  See the patch below.  It contains all the generic
parts as well as the patch to xstormy16-tdep.c which I used to verify that
the new function works fine, and that the dwarf2_frame_find_fde function
works as advertised now on XStormy16.

Is the generic part ok to apply?


Thanks,
Corinna


	* dwarf2-frame.c (decode_frame_entry_1): Remove redundant setting of
	addr_size.  Call gdbarch_dwarf2_addr_size rather than gdbarch_ptr_bit
	to determine addr_size in Dwarf versions < 4.
	* gdbarch.sh (dwarf2_addr_size): Define as variable.  Add lengthy
	comment to explain usage.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* xstormy16-tdep.c (xstormy16_gdbarch_init): Set dwarf2_addr_size to 4.


Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.114
diff -u -p -r1.114 dwarf2-frame.c
--- dwarf2-frame.c	7 Jul 2010 17:26:38 -0000	1.114
+++ dwarf2-frame.c	5 Aug 2010 15:25:27 -0000
@@ -1732,13 +1732,6 @@ decode_frame_entry_1 (struct comp_unit *
          depends on the target address size.  */
       cie->encoding = DW_EH_PE_absptr;
 
-      /* The target address size.  For .eh_frame FDEs this is considered
-	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
-	 this is supposed to be the target address size from the associated
-	 CU header.  FIXME: We do not have a good way to determine the 
-	 latter.  Always use the target pointer size for now.  */
-      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
-
       /* We'll determine the final value later, but we need to
 	 initialize it conservatively.  */
       cie->signal_frame = 0;
@@ -1779,7 +1772,7 @@ decode_frame_entry_1 (struct comp_unit *
 	}
       else
 	{
-	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	  cie->addr_size = gdbarch_dwarf2_addr_size (gdbarch);
 	  cie->segment_size = 0;
 	}
 
Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.514
diff -u -p -r1.514 gdbarch.sh
--- gdbarch.sh	19 Jul 2010 17:51:23 -0000	1.514
+++ gdbarch.sh	5 Aug 2010 15:25:28 -0000
@@ -384,14 +384,27 @@ v:const struct floatformat **:long_doubl
 # / addr_bit will be set from it.
 #
 # If gdbarch_ptr_bit and gdbarch_addr_bit are different, you'll probably
-# also need to set gdbarch_pointer_to_address and gdbarch_address_to_pointer
-# as well.
+# also need to set gdbarch_dwarf2_addr_size, gdbarch_pointer_to_address and
+# gdbarch_address_to_pointer as well.
 #
 # ptr_bit is the size of a pointer on the target
 v:int:ptr_bit:::8 * sizeof (void*):gdbarch->int_bit::0
 # addr_bit is the size of a target address as represented in gdb
 v:int:addr_bit:::8 * sizeof (void*):0:gdbarch_ptr_bit (gdbarch):
 #
+# dwarf2_addr_size is the target address size as used int the Dwarf debug
+# info.  For .eh_frame FDEs this is considered equal to the size of a target
+# pointer.  For .debug_frame FDEs, this is supposed to be the target address
+# size from the associated CU header, and which is equivalent to the
+# DWARF2_ADDR_SIZE as defined by the target specific GCC back-end.
+# Unfortunately there is no good way to determine this value.  Therefore
+# dwarf2_addr_size simply defaults to the target pointer size.
+#
+# Note that dwarf2_addr_size only needs to be redefined by a target if the
+# GCC back-end defines a DWARF2_ADDR_SIZE other than the target pointer size,
+# and if Dwarf versions < 4 need to be supported.
+v:int:dwarf2_addr_size:::sizeof (void*):0:gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT:
+#
 # One if \`char' acts like \`signed char', zero if \`unsigned char'.
 v:int:char_signed:::1:-1:1
 #
Index: xstormy16-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/xstormy16-tdep.c,v
retrieving revision 1.110
diff -u -p -r1.110 xstormy16-tdep.c
--- xstormy16-tdep.c	1 Jan 2010 07:31:46 -0000	1.110
+++ xstormy16-tdep.c	5 Aug 2010 15:25:28 -0000
@@ -809,6 +809,7 @@ xstormy16_gdbarch_init (struct gdbarch_i
 
   set_gdbarch_ptr_bit (gdbarch, 2 * TARGET_CHAR_BIT);
   set_gdbarch_addr_bit (gdbarch, 4 * TARGET_CHAR_BIT);
+  set_gdbarch_dwarf2_addr_size (gdbarch, 4);
 
   set_gdbarch_address_to_pointer (gdbarch, xstormy16_address_to_pointer);
   set_gdbarch_pointer_to_address (gdbarch, xstormy16_pointer_to_address);


-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 15:30                 ` Corinna Vinschen
@ 2010-08-05 16:52                   ` Ulrich Weigand
  2010-08-06 10:48                     ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-05 16:52 UTC (permalink / raw)
  To: gdb-patches

Corinna Vinschen wrote:

> Index: dwarf2-frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 dwarf2-frame.c
> --- dwarf2-frame.c	7 Jul 2010 17:26:38 -0000	1.114
> +++ dwarf2-frame.c	5 Aug 2010 15:25:27 -0000
> @@ -1732,13 +1732,6 @@ decode_frame_entry_1 (struct comp_unit *
>           depends on the target address size.  */
>        cie->encoding = DW_EH_PE_absptr;
>  
> -      /* The target address size.  For .eh_frame FDEs this is considered
> -	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
> -	 this is supposed to be the target address size from the associated
> -	 CU header.  FIXME: We do not have a good way to determine the 
> -	 latter.  Always use the target pointer size for now.  */
> -      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> -
>        /* We'll determine the final value later, but we need to
>  	 initialize it conservatively.  */
>        cie->signal_frame = 0;
> @@ -1779,7 +1772,7 @@ decode_frame_entry_1 (struct comp_unit *
>  	}
>        else
>  	{
> -	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
> +	  cie->addr_size = gdbarch_dwarf2_addr_size (gdbarch);
>  	  cie->segment_size = 0;
>  	}

Ah, wait a minute -- now we're missing the distinction between .eh_frame
and .debug_frame again.  For .eh_frame, FDE encodings needs to keep using
ptr_bit as discussed earlier, even on targets that do define a non-default
gdbarch_dwarf2_addr_size.  Sorry for not noticing earlier.

In fact, this means we might as well fix the inconsistent use between
addr_size as input to read_encoded_value (which needs to be ptr_size
on .eh_frame and addr_size on .debug_frame), and addr_size as input
to execute_stack_op (which needs to be addr_size always).

Could you add a second variable ptr_size to struct dwarf2_cie, and
initialize it (after the addr_size initialization) along the lines of:

  if (eh_frame_p)
    cie->ptr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
  else
    cie->ptr_size = cie->addr_size;

and then change all calls to read_encoded_value throughout dwarf2-frame.c
to pass cie->ptr_size instead of cie->addr_size.

It would then be interesting to see whether *both* .debug_frame and
.eh_frame work on xstormy16 ... you might be able to try the latter
by building with -fasynchronous-unwind-tables.

Bye,
Ulrich

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-05 16:52                   ` Ulrich Weigand
@ 2010-08-06 10:48                     ` Corinna Vinschen
  2010-08-06 11:17                       ` Mark Kettenis
  2010-08-06 14:51                       ` Ulrich Weigand
  0 siblings, 2 replies; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-06 10:48 UTC (permalink / raw)
  To: gdb-patches

On Aug  5 18:51, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> 
> > Index: dwarf2-frame.c
> > [...]
> 
> Ah, wait a minute -- now we're missing the distinction between .eh_frame
> and .debug_frame again.  For .eh_frame, FDE encodings needs to keep using
> ptr_bit as discussed earlier, even on targets that do define a non-default
> gdbarch_dwarf2_addr_size.  Sorry for not noticing earlier.
> 
> In fact, this means we might as well fix the inconsistent use between
> addr_size as input to read_encoded_value (which needs to be ptr_size
> on .eh_frame and addr_size on .debug_frame), and addr_size as input
> to execute_stack_op (which needs to be addr_size always).
> 
> Could you add a second variable ptr_size to struct dwarf2_cie, and
> initialize it (after the addr_size initialization) along the lines of:
> 
>   if (eh_frame_p)
>     cie->ptr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
>   else
>     cie->ptr_size = cie->addr_size;
> 
> and then change all calls to read_encoded_value throughout dwarf2-frame.c
> to pass cie->ptr_size instead of cie->addr_size.

No big deal, patch attached.

However, here's a question:

At one point in this thread you mentioned that .eh_frame isn't really
standarized.

Additionally, using the pointer size in .eh_frame sections would
invariably break unwinding on targets like avr and XStormy16.

And...

> It would then be interesting to see whether *both* .debug_frame and
> .eh_frame work on xstormy16 ... you might be able to try the latter
> by building with -fasynchronous-unwind-tables.

... .eh_frame sections are never generated for XStormy16.  The
-fasynchronous-unwind-tables option is a no-op.

Given all that, doesn't that mean that the .eh_frame sections for those
targets would *have* to use the address size rather than the pointer
size, as soon as .eh_frame sections are to be defined for those targets?
Their unwinders will never have a chance to do their job right otherwise.

Consequentially, is it *really* correct to define the ptr_size as you
requested now, or isn't it *more* correct to use the target dependent
approach as my previous patch implemented?  Since it's using the pointer
size as fallback, it will be correct for all existing .eh_frame sections
anyway.

It's your choice, but I'm quite certain that the previous approach/patch
will result in less headache in the long run.


Corinna


	* dwarf2-frame.c (struct dwarf2_cie): Add ptr_size member.
	Throughout, call read_encoded_value with ptr_size rather than addr_size.
	(decode_frame_entry_1): Remove redundant setting of
	addr_size.  Call gdbarch_dwarf2_addr_size rather than gdbarch_ptr_bit
	to determine addr_size in Dwarf versions < 4.  Set ptr_size dependent
	on examined frame section.  Add comment to explain why.
	* gdbarch.sh (dwarf2_addr_size): Define as variable.  Add lengthy
	comment to explain usage.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* xstormy16-tdep.c (xstormy16_gdbarch_init): Set dwarf2_addr_size to 4.


Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.114
diff -u -p -r1.114 dwarf2-frame.c
--- dwarf2-frame.c	7 Jul 2010 17:26:38 -0000	1.114
+++ dwarf2-frame.c	6 Aug 2010 10:41:26 -0000
@@ -77,6 +77,9 @@ struct dwarf2_cie
   /* Target address size in bytes.  */
   int addr_size;
 
+  /* Target pointer size in bytes. */
+  int ptr_size;
+
   /* True if a 'z' augmentation existed.  */
   unsigned char saw_z_augmentation;
 
@@ -452,7 +455,7 @@ execute_cfa_program (struct dwarf2_fde *
 	    {
 	    case DW_CFA_set_loc:
 	      fs->pc = read_encoded_value (fde->cie->unit, fde->cie->encoding,
-					   fde->cie->addr_size, insn_ptr,
+					   fde->cie->ptr_size, insn_ptr,
 					   &bytes_read, fde->initial_location);
 	      /* Apply the objfile offset for relocatable objects.  */
 	      fs->pc += ANOFFSET (fde->cie->unit->objfile->section_offsets,
@@ -1732,13 +1735,6 @@ decode_frame_entry_1 (struct comp_unit *
          depends on the target address size.  */
       cie->encoding = DW_EH_PE_absptr;
 
-      /* The target address size.  For .eh_frame FDEs this is considered
-	 equal to the size of a target pointer.  For .dwarf_frame FDEs, 
-	 this is supposed to be the target address size from the associated
-	 CU header.  FIXME: We do not have a good way to determine the 
-	 latter.  Always use the target pointer size for now.  */
-      cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
-
       /* We'll determine the final value later, but we need to
 	 initialize it conservatively.  */
       cie->signal_frame = 0;
@@ -1779,9 +1775,17 @@ decode_frame_entry_1 (struct comp_unit *
 	}
       else
 	{
-	  cie->addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+	  cie->addr_size = gdbarch_dwarf2_addr_size (gdbarch);
 	  cie->segment_size = 0;
 	}
+      /* Address values in .eh_frame sections are defined to have the
+	 target's pointer size.  Watchout: This breaks frame info for
+	 targets with pointer size < address size, unless a .debug_frame
+	 section exists as well. */
+      if (eh_frame_p)
+	cie->ptr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT;
+      else
+	cie->ptr_size = cie->addr_size;
 
       cie->code_alignment_factor =
 	read_unsigned_leb128 (unit->abfd, buf, &bytes_read);
@@ -1841,7 +1845,7 @@ decode_frame_entry_1 (struct comp_unit *
 	    {
 	      /* Skip.  Avoid indirection since we throw away the result.  */
 	      gdb_byte encoding = (*buf++) & ~DW_EH_PE_indirect;
-	      read_encoded_value (unit, encoding, cie->addr_size,
+	      read_encoded_value (unit, encoding, cie->ptr_size,
 				  buf, &bytes_read, 0);
 	      buf += bytes_read;
 	      augmentation++;
@@ -1907,13 +1911,13 @@ decode_frame_entry_1 (struct comp_unit *
       gdb_assert (fde->cie != NULL);
 
       fde->initial_location =
-	read_encoded_value (unit, fde->cie->encoding, fde->cie->addr_size,
+	read_encoded_value (unit, fde->cie->encoding, fde->cie->ptr_size,
 			    buf, &bytes_read, 0);
       buf += bytes_read;
 
       fde->address_range =
 	read_encoded_value (unit, fde->cie->encoding & 0x0f,
-			    fde->cie->addr_size, buf, &bytes_read, 0);
+			    fde->cie->ptr_size, buf, &bytes_read, 0);
       buf += bytes_read;
 
       /* A 'z' augmentation in the CIE implies the presence of an
Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.514
diff -u -p -r1.514 gdbarch.sh
--- gdbarch.sh	19 Jul 2010 17:51:23 -0000	1.514
+++ gdbarch.sh	6 Aug 2010 10:41:27 -0000
@@ -384,14 +384,27 @@ v:const struct floatformat **:long_doubl
 # / addr_bit will be set from it.
 #
 # If gdbarch_ptr_bit and gdbarch_addr_bit are different, you'll probably
-# also need to set gdbarch_pointer_to_address and gdbarch_address_to_pointer
-# as well.
+# also need to set gdbarch_dwarf2_addr_size, gdbarch_pointer_to_address and
+# gdbarch_address_to_pointer as well.
 #
 # ptr_bit is the size of a pointer on the target
 v:int:ptr_bit:::8 * sizeof (void*):gdbarch->int_bit::0
 # addr_bit is the size of a target address as represented in gdb
 v:int:addr_bit:::8 * sizeof (void*):0:gdbarch_ptr_bit (gdbarch):
 #
+# dwarf2_addr_size is the target address size as used int the Dwarf debug
+# info.  For .eh_frame FDEs this is considered equal to the size of a target
+# pointer.  For .debug_frame FDEs, this is supposed to be the target address
+# size from the associated CU header, and which is equivalent to the
+# DWARF2_ADDR_SIZE as defined by the target specific GCC back-end.
+# Unfortunately there is no good way to determine this value.  Therefore
+# dwarf2_addr_size simply defaults to the target pointer size.
+#
+# Note that dwarf2_addr_size only needs to be redefined by a target if the
+# GCC back-end defines a DWARF2_ADDR_SIZE other than the target pointer size,
+# and if Dwarf versions < 4 need to be supported.
+v:int:dwarf2_addr_size:::sizeof (void*):0:gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT:
+#
 # One if \`char' acts like \`signed char', zero if \`unsigned char'.
 v:int:char_signed:::1:-1:1
 #
Index: xstormy16-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/xstormy16-tdep.c,v
retrieving revision 1.110
diff -u -p -r1.110 xstormy16-tdep.c
--- xstormy16-tdep.c	1 Jan 2010 07:31:46 -0000	1.110
+++ xstormy16-tdep.c	6 Aug 2010 10:41:27 -0000
@@ -809,6 +809,7 @@ xstormy16_gdbarch_init (struct gdbarch_i
 
   set_gdbarch_ptr_bit (gdbarch, 2 * TARGET_CHAR_BIT);
   set_gdbarch_addr_bit (gdbarch, 4 * TARGET_CHAR_BIT);
+  set_gdbarch_dwarf2_addr_size (gdbarch, 4);
 
   set_gdbarch_address_to_pointer (gdbarch, xstormy16_address_to_pointer);
   set_gdbarch_pointer_to_address (gdbarch, xstormy16_pointer_to_address);


-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 10:48                     ` Corinna Vinschen
@ 2010-08-06 11:17                       ` Mark Kettenis
  2010-08-06 12:01                         ` Corinna Vinschen
  2010-08-06 14:51                       ` Ulrich Weigand
  1 sibling, 1 reply; 20+ messages in thread
From: Mark Kettenis @ 2010-08-06 11:17 UTC (permalink / raw)
  To: gdb-patches; +Cc: gdb-patches

> Date: Fri, 6 Aug 2010 12:48:10 +0200
> From: Corinna Vinschen <vinschen@redhat.com>
> 
> And...
> 
> > It would then be interesting to see whether *both* .debug_frame and
> > .eh_frame work on xstormy16 ... you might be able to try the latter
> > by building with -fasynchronous-unwind-tables.
> 
> ... .eh_frame sections are never generated for XStormy16.  The
> -fasynchronous-unwind-tables option is a no-op.

Is there no C++ exception exception handling on XStormy16?  Or did
nobody bother to do the work of switching it away from
setjump/longjump-style exception handling?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 11:17                       ` Mark Kettenis
@ 2010-08-06 12:01                         ` Corinna Vinschen
  0 siblings, 0 replies; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-06 12:01 UTC (permalink / raw)
  To: gdb-patches

On Aug  6 13:17, Mark Kettenis wrote:
> > Date: Fri, 6 Aug 2010 12:48:10 +0200
> > From: Corinna Vinschen <vinschen@redhat.com>
> > 
> > And...
> > 
> > > It would then be interesting to see whether *both* .debug_frame and
> > > .eh_frame work on xstormy16 ... you might be able to try the latter
> > > by building with -fasynchronous-unwind-tables.
> > 
> > ... .eh_frame sections are never generated for XStormy16.  The
> > -fasynchronous-unwind-tables option is a no-op.
> 
> Is there no C++ exception exception handling on XStormy16?  Or did
> nobody bother to do the work of switching it away from
> setjump/longjump-style exception handling?

XStormy16 is using SJLJ.  And from the today's perspective I'm glad that
it hasn't been converted to DW2 EH, given the obvious problem with the
definition of the .eh_frame section.

There's also the problem that the .eh_frame section is rather big, and
that the section has to be loaded for EH to work.  This is not a
terribly good idea for a very space constricted target like XStormy16.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 10:48                     ` Corinna Vinschen
  2010-08-06 11:17                       ` Mark Kettenis
@ 2010-08-06 14:51                       ` Ulrich Weigand
  2010-08-06 15:57                         ` Corinna Vinschen
  1 sibling, 1 reply; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-06 14:51 UTC (permalink / raw)
  To: gdb-patches

Corinna Vinschen wrote:

> At one point in this thread you mentioned that .eh_frame isn't really
> standarized.
> 
> Additionally, using the pointer size in .eh_frame sections would
> invariably break unwinding on targets like avr and XStormy16.

No, it would make it work :-)  In .eh_frame sections, the contents
of DW_EH_PE_absptr encoded fields are *used as pointer values* at
run time.  See e.g. unwind-pe.h:read_encoded_value_with_base:

static const unsigned char *
read_encoded_value_with_base (unsigned char encoding, _Unwind_Ptr base,
                              const unsigned char *p, _Unwind_Ptr *val)
{
  union unaligned
    {
      void *ptr;
      unsigned u2 __attribute__ ((mode (HI)));
      unsigned u4 __attribute__ ((mode (SI)));
      unsigned u8 __attribute__ ((mode (DI)));
      signed s2 __attribute__ ((mode (HI)));
      signed s4 __attribute__ ((mode (SI)));
      signed s8 __attribute__ ((mode (DI)));
    } __attribute__((__packed__));

  const union unaligned *u = (const union unaligned *) p;
  _Unwind_Internal_Ptr result;
[...]
      switch (encoding & 0x0f)
        {
        case DW_EH_PE_absptr:
          result = (_Unwind_Internal_Ptr) u->ptr;
          p += sizeof (void *);
          break;

"p" points directly into the mapped .eh_frame image.  The contents
are simply interpreted as a native "void *" at run-time.  If GDB
wants to use this section as well, it has to treat it like it would
read a "void *" global variable from the target ...

> > It would then be interesting to see whether *both* .debug_frame and
> > .eh_frame work on xstormy16 ... you might be able to try the latter
> > by building with -fasynchronous-unwind-tables.
>
> ... .eh_frame sections are never generated for XStormy16.  The
> -fasynchronous-unwind-tables option is a no-op.

OK, that's unfortunate.

> Consequentially, is it *really* correct to define the ptr_size as you
> requested now, or isn't it *more* correct to use the target dependent
> approach as my previous patch implemented?  Since it's using the pointer
> size as fallback, it will be correct for all existing .eh_frame sections
> anyway.

The difference between .eh_frame and .debug_frame is really fundamental;
.eh_frame holds *pointer* values in target "void *" format; while
.debug_frame holds *address* values using the same format as the rest
of the DWARF debug sections.

Therefore we will definitely have to make a distinction between the two.
If you have another suggestion how to achieve that, I'd certainly be
interested ...


> 	* dwarf2-frame.c (struct dwarf2_cie): Add ptr_size member.
> 	Throughout, call read_encoded_value with ptr_size rather than addr_size.
> 	(decode_frame_entry_1): Remove redundant setting of
> 	addr_size.  Call gdbarch_dwarf2_addr_size rather than gdbarch_ptr_bit
> 	to determine addr_size in Dwarf versions < 4.  Set ptr_size dependent
> 	on examined frame section.  Add comment to explain why.
> 	* gdbarch.sh (dwarf2_addr_size): Define as variable.  Add lengthy
> 	comment to explain usage.
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* xstormy16-tdep.c (xstormy16_gdbarch_init): Set dwarf2_addr_size to 4.

This looks good to me, except a couple of the comments:

> +      /* Address values in .eh_frame sections are defined to have the
> +	 target's pointer size.  Watchout: This breaks frame info for
> +	 targets with pointer size < address size, unless a .debug_frame
> +	 section exists as well. */

As discussed above, I don't agree that this breaks anything ...  Do you
still feel it does?

> +# dwarf2_addr_size is the target address size as used int the Dwarf debug
> +# info.  For .eh_frame FDEs this is considered equal to the size of a target
> +# pointer.  For .debug_frame FDEs, this is supposed to be the target address
> +# size from the associated CU header, and which is equivalent to the
> +# DWARF2_ADDR_SIZE as defined by the target specific GCC back-end.
> +# Unfortunately there is no good way to determine this value.  Therefore
> +# dwarf2_addr_size simply defaults to the target pointer size.

I'd reword this to make clear that this value is *not* used for .eh_frame,
but solely for .debug_frame.

Thanks,
Ulrich

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 14:51                       ` Ulrich Weigand
@ 2010-08-06 15:57                         ` Corinna Vinschen
  2010-08-06 16:27                           ` Ulrich Weigand
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-06 15:57 UTC (permalink / raw)
  To: gdb-patches

On Aug  6 16:50, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> 
> > At one point in this thread you mentioned that .eh_frame isn't really
> > standarized.
> > 
> > Additionally, using the pointer size in .eh_frame sections would
> > invariably break unwinding on targets like avr and XStormy16.
> 
> No, it would make it work :-)  In .eh_frame sections, the contents
> of DW_EH_PE_absptr encoded fields are *used as pointer values* at
> run time.  See e.g. unwind-pe.h:read_encoded_value_with_base:
> [...]
>         case DW_EH_PE_absptr:
>           result = (_Unwind_Internal_Ptr) u->ptr;
>           p += sizeof (void *);
>           break;
> 
> "p" points directly into the mapped .eh_frame image.  The contents
> are simply interpreted as a native "void *" at run-time.  If GDB
> wants to use this section as well, it has to treat it like it would
> read a "void *" global variable from the target ...
> [...]
> > Consequentially, is it *really* correct to define the ptr_size as you
> > requested now, or isn't it *more* correct to use the target dependent
> > approach as my previous patch implemented?  Since it's using the pointer
> > size as fallback, it will be correct for all existing .eh_frame sections
> > anyway.
> 
> The difference between .eh_frame and .debug_frame is really fundamental;
> .eh_frame holds *pointer* values in target "void *" format; while
> .debug_frame holds *address* values using the same format as the rest
> of the DWARF debug sections.
> 
> Therefore we will definitely have to make a distinction between the two.
> If you have another suggestion how to achieve that, I'd certainly be
> interested ...

I understand what you're up to, but to me this means that the above code
from unwind-pe.h is potentially not usable for certain small targets,
unless some conditions are met.

As it is the one example I really know well, let's stick to XStormy16
for now.

The problem for XStormy16 in 16 bit pointer mode is that a pointer is
not able to point to every place in the 24 bit address space the CPU can
address.  For function pointers that means that the target potentially
has to use a jump table.  For the stack that means it is restricted to
the first 64K RAM.

So, afaics, the unwind-pe.h code only works correct for XStormy16, if
either the application fits into the first 64K of memory, or if
DW_EH_PE_absptr is not used, rather DW_EH_PE_pcrel, DW_EH_PE_textrel,
DW_EH_PE_datarel, or DW_EH_PE_funcrel.  Oh, and then there's the
type of _Unwind_Ptr, which would have to be big enough, 32 bit.

Alternatively, never use DW EH on such targets since it has very
obviously never been designed with small targets in mind.

> > 	* dwarf2-frame.c (struct dwarf2_cie): Add ptr_size member.
> > 	Throughout, call read_encoded_value with ptr_size rather than addr_size.
> > 	(decode_frame_entry_1): Remove redundant setting of
> > 	addr_size.  Call gdbarch_dwarf2_addr_size rather than gdbarch_ptr_bit
> > 	to determine addr_size in Dwarf versions < 4.  Set ptr_size dependent
> > 	on examined frame section.  Add comment to explain why.
> > 	* gdbarch.sh (dwarf2_addr_size): Define as variable.  Add lengthy
> > 	comment to explain usage.
> > 	* gdbarch.c: Regenerate.
> > 	* gdbarch.h: Regenerate.
> > 	* xstormy16-tdep.c (xstormy16_gdbarch_init): Set dwarf2_addr_size to 4.
> 
> This looks good to me, except a couple of the comments:
> 
> > +      /* Address values in .eh_frame sections are defined to have the
> > +	 target's pointer size.  Watchout: This breaks frame info for
> > +	 targets with pointer size < address size, unless a .debug_frame
> > +	 section exists as well. */
> 
> As discussed above, I don't agree that this breaks anything ...  Do you
> still feel it does?

Right now?  Yes.  It's not GDBs fault, though...

> > +# dwarf2_addr_size is the target address size as used int the Dwarf debug
> > +# info.  For .eh_frame FDEs this is considered equal to the size of a target
> > +# pointer.  For .debug_frame FDEs, this is supposed to be the target address
> > +# size from the associated CU header, and which is equivalent to the
> > +# DWARF2_ADDR_SIZE as defined by the target specific GCC back-end.
> > +# Unfortunately there is no good way to determine this value.  Therefore
> > +# dwarf2_addr_size simply defaults to the target pointer size.
> 
> I'd reword this to make clear that this value is *not* used for .eh_frame,
> but solely for .debug_frame.

Ok, will do.  I'd just like to put the discussion to an end, first.
Just tell me what you think of what I wrote above.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 15:57                         ` Corinna Vinschen
@ 2010-08-06 16:27                           ` Ulrich Weigand
  2010-08-06 16:59                             ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-06 16:27 UTC (permalink / raw)
  To: gdb-patches

Corinna Vinschen wrote:
> On Aug  6 16:50, Ulrich Weigand wrote:
> > The difference between .eh_frame and .debug_frame is really fundamental;
> > .eh_frame holds *pointer* values in target "void *" format; while
> > .debug_frame holds *address* values using the same format as the rest
> > of the DWARF debug sections.
> > 
> > Therefore we will definitely have to make a distinction between the two.
> > If you have another suggestion how to achieve that, I'd certainly be
> > interested ...
> 
> I understand what you're up to, but to me this means that the above code
> from unwind-pe.h is potentially not usable for certain small targets,
> unless some conditions are met.
> 
> As it is the one example I really know well, let's stick to XStormy16
> for now.
> 
> The problem for XStormy16 in 16 bit pointer mode is that a pointer is
> not able to point to every place in the 24 bit address space the CPU can
> address.  For function pointers that means that the target potentially
> has to use a jump table.  For the stack that means it is restricted to
> the first 64K RAM.
> 
> So, afaics, the unwind-pe.h code only works correct for XStormy16, if
> either the application fits into the first 64K of memory, or if
> DW_EH_PE_absptr is not used, rather DW_EH_PE_pcrel, DW_EH_PE_textrel,
> DW_EH_PE_datarel, or DW_EH_PE_funcrel.  Oh, and then there's the
> type of _Unwind_Ptr, which would have to be big enough, 32 bit.

OK, I see what you mean.  So if we were to enable DWARF EH for XStormy16,
we'd either have to do what you just described (all of which should in
principle be doable), or else add something new to support larger "pointer"
or address types.  I'd assume this might then be a new encoding type ...

In any case, I'd still say that GDB today ought to match what GCC today
does, which is that DW_EH_PE_absptr encoding uses target-format pointers.
If and when GCC is extended, say to support another encoding type, we'd
then likewise extend GDB to support that new feature.


> > I'd reword this to make clear that this value is *not* used for .eh_frame,
> > but solely for .debug_frame.
> 
> Ok, will do.  I'd just like to put the discussion to an end, first.
> Just tell me what you think of what I wrote above.

Does the above make sense to you?

Bye,
Ulrich

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 16:27                           ` Ulrich Weigand
@ 2010-08-06 16:59                             ` Corinna Vinschen
  2010-08-06 19:03                               ` Corinna Vinschen
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-06 16:59 UTC (permalink / raw)
  To: gdb-patches

On Aug  6 18:27, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> > The problem for XStormy16 in 16 bit pointer mode is that a pointer is
> > not able to point to every place in the 24 bit address space the CPU can
> > address.  For function pointers that means that the target potentially
> > has to use a jump table.  For the stack that means it is restricted to
> > the first 64K RAM.
> > 
> > So, afaics, the unwind-pe.h code only works correct for XStormy16, if
> > either the application fits into the first 64K of memory, or if
> > DW_EH_PE_absptr is not used, rather DW_EH_PE_pcrel, DW_EH_PE_textrel,
> > DW_EH_PE_datarel, or DW_EH_PE_funcrel.  Oh, and then there's the
> > type of _Unwind_Ptr, which would have to be big enough, 32 bit.
> 
> OK, I see what you mean.  So if we were to enable DWARF EH for XStormy16,
> we'd either have to do what you just described (all of which should in
> principle be doable), or else add something new to support larger "pointer"
> or address types.  I'd assume this might then be a new encoding type ...
> 
> In any case, I'd still say that GDB today ought to match what GCC today
> does, which is that DW_EH_PE_absptr encoding uses target-format pointers.
> If and when GCC is extended, say to support another encoding type, we'd
> then likewise extend GDB to support that new feature.
> 
> 
> > > I'd reword this to make clear that this value is *not* used for .eh_frame,
> > > but solely for .debug_frame.
> > 
> > Ok, will do.  I'd just like to put the discussion to an end, first.
> > Just tell me what you think of what I wrote above.
> 
> Does the above make sense to you?

Yes, fine with me, but that also means the comment I added makes still
sense...


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 16:59                             ` Corinna Vinschen
@ 2010-08-06 19:03                               ` Corinna Vinschen
  2010-08-08 14:55                                 ` Ulrich Weigand
  0 siblings, 1 reply; 20+ messages in thread
From: Corinna Vinschen @ 2010-08-06 19:03 UTC (permalink / raw)
  To: gdb-patches

On Aug  6 18:59, Corinna Vinschen wrote:
> On Aug  6 18:27, Ulrich Weigand wrote:
> > Corinna Vinschen wrote:
> > > The problem for XStormy16 in 16 bit pointer mode is that a pointer is
> > > not able to point to every place in the 24 bit address space the CPU can
> > > address.  For function pointers that means that the target potentially
> > > has to use a jump table.  For the stack that means it is restricted to
> > > the first 64K RAM.
> > > 
> > > So, afaics, the unwind-pe.h code only works correct for XStormy16, if
> > > either the application fits into the first 64K of memory, or if
> > > DW_EH_PE_absptr is not used, rather DW_EH_PE_pcrel, DW_EH_PE_textrel,
> > > DW_EH_PE_datarel, or DW_EH_PE_funcrel.  Oh, and then there's the
> > > type of _Unwind_Ptr, which would have to be big enough, 32 bit.
> > 
> > OK, I see what you mean.  So if we were to enable DWARF EH for XStormy16,
> > we'd either have to do what you just described (all of which should in
> > principle be doable), or else add something new to support larger "pointer"
> > or address types.  I'd assume this might then be a new encoding type ...
> > 
> > In any case, I'd still say that GDB today ought to match what GCC today
> > does, which is that DW_EH_PE_absptr encoding uses target-format pointers.
> > If and when GCC is extended, say to support another encoding type, we'd
> > then likewise extend GDB to support that new feature.
> > 
> > 
> > > > I'd reword this to make clear that this value is *not* used for .eh_frame,
> > > > but solely for .debug_frame.
> > > 
> > > Ok, will do.  I'd just like to put the discussion to an end, first.
> > > Just tell me what you think of what I wrote above.
> > 
> > Does the above make sense to you?
> 
> Yes, fine with me, but that also means the comment I added makes still
> sense...

I applied the patch with the suggested change to the comment in gdbarch.sh.


Thanks for your review,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [rfa] frame address size incorrect if address size != ptr size
  2010-08-06 19:03                               ` Corinna Vinschen
@ 2010-08-08 14:55                                 ` Ulrich Weigand
  0 siblings, 0 replies; 20+ messages in thread
From: Ulrich Weigand @ 2010-08-08 14:55 UTC (permalink / raw)
  To: gdb-patches

Corinna Vinschen wrote:

> I applied the patch with the suggested change to the comment in gdbarch.sh.

Great; thanks for working on this!

Bye,
Ulrich

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2010-08-08 14:55 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-26 14:53 [rfa] frame address size incorrect if address size != ptr size Corinna Vinschen
2010-08-04 11:35 ` Corinna Vinschen
2010-08-04 22:40   ` Jan Kratochvil
2010-08-05  8:06     ` Corinna Vinschen
2010-08-05 10:04       ` Ulrich Weigand
2010-08-05 12:30         ` Corinna Vinschen
2010-08-05 14:08           ` Ulrich Weigand
2010-08-05 14:30             ` Corinna Vinschen
2010-08-05 14:59               ` Ulrich Weigand
2010-08-05 15:30                 ` Corinna Vinschen
2010-08-05 16:52                   ` Ulrich Weigand
2010-08-06 10:48                     ` Corinna Vinschen
2010-08-06 11:17                       ` Mark Kettenis
2010-08-06 12:01                         ` Corinna Vinschen
2010-08-06 14:51                       ` Ulrich Weigand
2010-08-06 15:57                         ` Corinna Vinschen
2010-08-06 16:27                           ` Ulrich Weigand
2010-08-06 16:59                             ` Corinna Vinschen
2010-08-06 19:03                               ` Corinna Vinschen
2010-08-08 14:55                                 ` Ulrich Weigand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).