[RFA] dwarf2-frame.c sign extension patch

[RFA] dwarf2-frame.c sign extension patch

[Martin M. Hunt]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

This patch fixes some dwarf2 problems with sign-extension. 

2004-07-20  Kevin Buettner and Martin Hunt  <hunt@redhat.com>
  
        * dwarf2-frame.c (execute_cfa_program): Fix typo in which the
        alignment was being added to the offset instead of multiplied.

        (struct comp_unit): Add new field ``signed_addr_p''.

        (encoding_for_size): Add new parameter ``signed_addr_p''. 
	Adjust all callers.  Add code for handling signed encodings.

        (dwarf2_build_frame_info): Initialize ``unit.signed_addr_p''.

        (dwarf2_build_frame_info): Set unit.addr_size.

-- 
Martin M. Hunt <hunt@redhat.com>
Red Hat Inc.

[-- Attachment #2: cfi_patch --]
[-- Type: text/x-patch, Size: 2606 bytes --]

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.36
diff -w -u -r1.36 dwarf2-frame.c
--- dwarf2-frame.c	15 Jun 2004 01:04:19 -0000	1.36
+++ dwarf2-frame.c	20 Jul 2004 18:54:54 -0000
@@ -426,7 +428,7 @@
 	    case DW_CFA_offset_extended_sf:
 	      insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
 	      insn_ptr = read_sleb128 (insn_ptr, insn_end, &offset);
-	      offset += fs->data_align;
+	      offset *= fs->data_align;
 	      dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
 	      fs->regs.reg[reg].how = DWARF2_FRAME_REG_SAVED_OFFSET;
 	      fs->regs.reg[reg].loc.offset = offset;
@@ -893,6 +895,9 @@
   /* Address size for this unit - from unit header.  */
   unsigned char addr_size;
 
+  /* Are addresses signed?  */
+  unsigned char signed_addr_p;
+
   /* Pointer to the .debug_frame section loaded into memory.  */
   char *dwarf_frame_buffer;
 
@@ -1021,15 +1026,24 @@
    should be dereferenced.  */
 
 static unsigned char
-encoding_for_size (unsigned int size)
+encoding_for_size (unsigned int size, int signed_addr_p)
 {
   switch (size)
     {
     case 2:
+      if (signed_addr_p)
+	return DW_EH_PE_sdata2;
+      else
       return DW_EH_PE_udata2;
     case 4:
+      if (signed_addr_p)
+	return DW_EH_PE_sdata4;
+      else
       return DW_EH_PE_udata4;
     case 8:
+      if (signed_addr_p)
+	return DW_EH_PE_sdata8;
+      else
       return DW_EH_PE_udata8;
     default:
       internal_error (__FILE__, __LINE__, "Unsupported address size");
@@ -1110,7 +1124,7 @@
     }
 
   if ((encoding & 0x0f) == 0x00)
-    encoding |= encoding_for_size (ptr_len);
+    encoding |= encoding_for_size (ptr_len, unit->signed_addr_p);
 
   switch (encoding & 0x0f)
     {
@@ -1286,7 +1300,7 @@
       /* The encoding for FDE's in a normal .debug_frame section
          depends on the target address size as specified in the
          Compilation Unit Header.  */
-      cie->encoding = encoding_for_size (unit->addr_size);
+      cie->encoding = encoding_for_size (unit->addr_size, unit->signed_addr_p);
 
       /* Check version number.  */
       cie_version = read_1_byte (unit->abfd, buf);
@@ -1557,7 +1571,8 @@
   /* Build a minimal decoding of the DWARF2 compilation unit.  */
   unit.abfd = objfile->obfd;
   unit.objfile = objfile;
-  unit.addr_size = objfile->obfd->arch_info->bits_per_address / 8;
+  unit.addr_size = TYPE_LENGTH (builtin_type_void_data_ptr);
+  unit.signed_addr_p = bfd_get_sign_extend_vma (unit.abfd);
   unit.dbase = 0;
   unit.tbase = 0;

Re: [RFA] dwarf2-frame.c sign extension patch

[Mark Kettenis]

   From: "Martin M. Hunt" <hunt@redhat.com>
   Date: Tue, 20 Jul 2004 12:11:25 -0700

   This patch fixes some dwarf2 problems with sign-extension.

   2004-07-20  Kevin Buettner and Martin Hunt  <hunt@redhat.com>

	   * dwarf2-frame.c (execute_cfa_program): Fix typo in which the
	   alignment was being added to the offset instead of multiplied.

This is unrelated to the rest of the changes.  Please make this a
seperate commit.  Consider that one pre-approved.

	   (struct comp_unit): Add new field ``signed_addr_p''.

	   (encoding_for_size): Add new parameter ``signed_addr_p''. 
	   Adjust all callers.  Add code for handling signed encodings.

	   (dwarf2_build_frame_info): Initialize ``unit.signed_addr_p''.

These are all related, so there shouldn't be any blank lines between
them.  The formatting of your changes to encoding_for_size() is wrong.
Please re-indent the exitsing return()-lines.  Or perhaps it's better
to use:

  return signed_addr_p ? DW_EH_PE_sdata4 : DW_EH_PE_udata4;

	   (dwarf2_build_frame_info): Set unit.addr_size.

Why is the old code wrong?  The comment clearly says "from unit
header" which is what the DWARF standard says (or at least implies).
Theoretically it could change from compilation unit to compilation
unit.  So I think your change to replace it by
TYPE_LENGTH(builtin_type_void_data_ptr) is wrong.

Mark

Re: [RFA] dwarf2-frame.c sign extension patch

[Daniel Jacobowitz]

The rest I have no comment on, but this issue I know...

On Wed, Jul 21, 2004 at 10:29:43PM +0200, Mark Kettenis wrote:
> 	   (dwarf2_build_frame_info): Set unit.addr_size.
> 
> Why is the old code wrong?  The comment clearly says "from unit
> header" which is what the DWARF standard says (or at least implies).
> Theoretically it could change from compilation unit to compilation
> unit.  So I think your change to replace it by
> TYPE_LENGTH(builtin_type_void_data_ptr) is wrong.

The old code doesn't do it from the compilation unit header either:
-  unit.addr_size = objfile->obfd->arch_info->bits_per_address / 8;
+  unit.addr_size = TYPE_LENGTH (builtin_type_void_data_ptr);

Any use of bits_per_address, IMO, is a bug.  Take a look at how this
field is set on MIPS; it's based on the architecture, not the ABI or
the pointer size or the dwarf address size or anything like that.

TYPE_LENGTH doesn't seem like an ideal replacement.  Can we use the
value in the compilation unit header here?

-- 
Daniel Jacobowitz

Re: [RFA] dwarf2-frame.c sign extension patch

[Kevin Buettner]

On Wed, 21 Jul 2004 17:17:23 -0400
Daniel Jacobowitz <drow@false.org> wrote:

> The rest I have no comment on, but this issue I know...
> 
> On Wed, Jul 21, 2004 at 10:29:43PM +0200, Mark Kettenis wrote:
> > 	   (dwarf2_build_frame_info): Set unit.addr_size.
> > 
> > Why is the old code wrong?  The comment clearly says "from unit
> > header" which is what the DWARF standard says (or at least implies).
> > Theoretically it could change from compilation unit to compilation
> > unit.  So I think your change to replace it by
> > TYPE_LENGTH(builtin_type_void_data_ptr) is wrong.
> 
> The old code doesn't do it from the compilation unit header either:
> -  unit.addr_size = objfile->obfd->arch_info->bits_per_address / 8;
> +  unit.addr_size = TYPE_LENGTH (builtin_type_void_data_ptr);
> 
> Any use of bits_per_address, IMO, is a bug.  Take a look at how this
> field is set on MIPS; it's based on the architecture, not the ABI or
> the pointer size or the dwarf address size or anything like that.

I agree.

> TYPE_LENGTH doesn't seem like an ideal replacement.  Can we use the
> value in the compilation unit header here?

I considered doing this, but was told that the information in
.debug_frame should be independent of the CU header.  I really
don't know what the "right" solution is.

Kevin

Re: [RFA] dwarf2-frame.c sign extension patch

[Daniel Jacobowitz]

On Wed, Jul 21, 2004 at 05:41:36PM -0700, Kevin Buettner wrote:
> On Wed, 21 Jul 2004 17:17:23 -0400
> Daniel Jacobowitz <drow@false.org> wrote:
> 
> > The rest I have no comment on, but this issue I know...
> > 
> > On Wed, Jul 21, 2004 at 10:29:43PM +0200, Mark Kettenis wrote:
> > > 	   (dwarf2_build_frame_info): Set unit.addr_size.
> > > 
> > > Why is the old code wrong?  The comment clearly says "from unit
> > > header" which is what the DWARF standard says (or at least implies).
> > > Theoretically it could change from compilation unit to compilation
> > > unit.  So I think your change to replace it by
> > > TYPE_LENGTH(builtin_type_void_data_ptr) is wrong.
> > 
> > The old code doesn't do it from the compilation unit header either:
> > -  unit.addr_size = objfile->obfd->arch_info->bits_per_address / 8;
> > +  unit.addr_size = TYPE_LENGTH (builtin_type_void_data_ptr);
> > 
> > Any use of bits_per_address, IMO, is a bug.  Take a look at how this
> > field is set on MIPS; it's based on the architecture, not the ABI or
> > the pointer size or the dwarf address size or anything like that.
> 
> I agree.
> 
> > TYPE_LENGTH doesn't seem like an ideal replacement.  Can we use the
> > value in the compilation unit header here?
> 
> I considered doing this, but was told that the information in
> .debug_frame should be independent of the CU header.  I really
> don't know what the "right" solution is.

Oh, right, we're in .debug_frame here.  I care very much about being
.able to use .debug_frame without .debug_info.  Debian distributes
library images with only .debug_frame, to provide backtraces.

.debug_frame doesn't provide this information.  That's a known defect;
I think that Jim proposed correcting it on the dwarf2 list.

Does anyone know if .eh_frame solves this problem somehow?

Otherwise, I think that using TYPE_LENGTH (with a big comment
explaining the issue) may be the best we can do.

-- 
Daniel Jacobowitz

Re: [RFA] dwarf2-frame.c sign extension patch

[Mark Kettenis]

   Date: Wed, 21 Jul 2004 20:46:21 -0400
   From: Daniel Jacobowitz <drow@false.org>

   On Wed, Jul 21, 2004 at 05:41:36PM -0700, Kevin Buettner wrote:
   > > Any use of bits_per_address, IMO, is a bug.  Take a look at how this
   > > field is set on MIPS; it's based on the architecture, not the ABI or
   > > the pointer size or the dwarf address size or anything like that.
   > 
   > I agree.
   > 
   > > TYPE_LENGTH doesn't seem like an ideal replacement.  Can we use the
   > > value in the compilation unit header here?
   > 
   > I considered doing this, but was told that the information in
   > .debug_frame should be independent of the CU header.  I really
   > don't know what the "right" solution is.

   Oh, right, we're in .debug_frame here.  I care very much about being
   .able to use .debug_frame without .debug_info.  Debian distributes
   library images with only .debug_frame, to provide backtraces.

Ah yes, the standard explicitly says that .debug_frame is
self-contained.

   .debug_frame doesn't provide this information.  That's a known defect;
   I think that Jim proposed correcting it on the dwarf2 list.

Hmm, I'm starting to wonder why we set unit->addr_size at all.  It's
only used in decode_frame_entry_1().  I suppose it makes sense to
cache this value, although I have no data to support that.

   Does anyone know if .eh_frame solves this problem somehow?

In many cases it directly encodes the size of pointers.

   Otherwise, I think that using TYPE_LENGTH (with a big comment
   explaining the issue) may be the best we can do.

Actually, it would make things more symmetric if we'd use TYPE_LENGTH.
Even better, avoid the issue completely by initializing the CIE
encoding to DW_EH_PE_absptr.  How about the attached patch?  Does it
solve your problems Martin?

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* dwarf2-frame.c (struct dwarf2_cie): Delete `addr_size' member.
	(decode_frame_entry_1): Use DW_EH_PE_absptr as default for CIE
	encoding.
	(dwarf2_build_frame_info): Adjust for removal of `addr_size'
	member of `struct comp_unit'.

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.36
diff -u -p -r1.36 dwarf2-frame.c
--- dwarf2-frame.c 15 Jun 2004 01:04:19 -0000 1.36
+++ dwarf2-frame.c 22 Jul 2004 20:45:28 -0000
@@ -890,9 +890,6 @@ struct comp_unit
   /* Linked list of CIEs for this object.  */
   struct dwarf2_cie *cie;
 
-  /* Address size for this unit - from unit header.  */
-  unsigned char addr_size;
-
   /* Pointer to the .debug_frame section loaded into memory.  */
   char *dwarf_frame_buffer;
 
@@ -1284,9 +1281,8 @@ decode_frame_entry_1 (struct comp_unit *
       cie->cie_pointer = cie_pointer;
 
       /* The encoding for FDE's in a normal .debug_frame section
-         depends on the target address size as specified in the
-         Compilation Unit Header.  */
-      cie->encoding = encoding_for_size (unit->addr_size);
+         depends on the target address size.  */
+      cie->encoding = DW_EH_PE_absptr;
 
       /* Check version number.  */
       cie_version = read_1_byte (unit->abfd, buf);
@@ -1557,7 +1553,6 @@ dwarf2_build_frame_info (struct objfile 
   /* Build a minimal decoding of the DWARF2 compilation unit.  */
   unit.abfd = objfile->obfd;
   unit.objfile = objfile;
-  unit.addr_size = objfile->obfd->arch_info->bits_per_address / 8;
   unit.dbase = 0;
   unit.tbase = 0;
 

Re: [RFA] dwarf2-frame.c sign extension patch

[Daniel Jacobowitz]

On Thu, Jul 22, 2004 at 10:45:46PM +0200, Mark Kettenis wrote:
> Actually, it would make things more symmetric if we'd use TYPE_LENGTH.
> Even better, avoid the issue completely by initializing the CIE
> encoding to DW_EH_PE_absptr.  How about the attached patch?  Does it
> solve your problems Martin?

A-ha!  I didn't realize that size_of_encoded_value already did this.
I like your solution much better :-)

-- 
Daniel Jacobowitz

Re: [RFA] dwarf2-frame.c sign extension patch

[Martin M. Hunt]

Mark,

Yes, I like your patch and it works fine.  Please check it in. 

Martin
  
On Thu, 2004-07-22 at 13:45, Mark Kettenis wrote:
>    Date: Wed, 21 Jul 2004 20:46:21 -0400
>    From: Daniel Jacobowitz <drow@false.org>
> 
>    On Wed, Jul 21, 2004 at 05:41:36PM -0700, Kevin Buettner wrote:
>    > > Any use of bits_per_address, IMO, is a bug.  Take a look at how this
>    > > field is set on MIPS; it's based on the architecture, not the ABI or
>    > > the pointer size or the dwarf address size or anything like that.
>    > 
>    > I agree.
>    > 
>    > > TYPE_LENGTH doesn't seem like an ideal replacement.  Can we use the
>    > > value in the compilation unit header here?
>    > 
>    > I considered doing this, but was told that the information in
>    > .debug_frame should be independent of the CU header.  I really
>    > don't know what the "right" solution is.
> 
>    Oh, right, we're in .debug_frame here.  I care very much about being
>    .able to use .debug_frame without .debug_info.  Debian distributes
>    library images with only .debug_frame, to provide backtraces.
> 
> Ah yes, the standard explicitly says that .debug_frame is
> self-contained.
> 
>    .debug_frame doesn't provide this information.  That's a known defect;
>    I think that Jim proposed correcting it on the dwarf2 list.
> 
> Hmm, I'm starting to wonder why we set unit->addr_size at all.  It's
> only used in decode_frame_entry_1().  I suppose it makes sense to
> cache this value, although I have no data to support that.
> 
>    Does anyone know if .eh_frame solves this problem somehow?
> 
> In many cases it directly encodes the size of pointers.
> 
>    Otherwise, I think that using TYPE_LENGTH (with a big comment
>    explaining the issue) may be the best we can do.
> 
> Actually, it would make things more symmetric if we'd use TYPE_LENGTH.
> Even better, avoid the issue completely by initializing the CIE
> encoding to DW_EH_PE_absptr.  How about the attached patch?  Does it
> solve your problems Martin?
> 
> Mark
> 
> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
> 	* dwarf2-frame.c (struct dwarf2_cie): Delete `addr_size' member.
> 	(decode_frame_entry_1): Use DW_EH_PE_absptr as default for CIE
> 	encoding.
> 	(dwarf2_build_frame_info): Adjust for removal of `addr_size'
> 	member of `struct comp_unit'.
> 
> Index: dwarf2-frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 dwarf2-frame.c
> --- dwarf2-frame.c 15 Jun 2004 01:04:19 -0000 1.36
> +++ dwarf2-frame.c 22 Jul 2004 20:45:28 -0000
> @@ -890,9 +890,6 @@ struct comp_unit
>    /* Linked list of CIEs for this object.  */
>    struct dwarf2_cie *cie;
>  
> -  /* Address size for this unit - from unit header.  */
> -  unsigned char addr_size;
> -
>    /* Pointer to the .debug_frame section loaded into memory.  */
>    char *dwarf_frame_buffer;
>  
> @@ -1284,9 +1281,8 @@ decode_frame_entry_1 (struct comp_unit *
>        cie->cie_pointer = cie_pointer;
>  
>        /* The encoding for FDE's in a normal .debug_frame section
> -         depends on the target address size as specified in the
> -         Compilation Unit Header.  */
> -      cie->encoding = encoding_for_size (unit->addr_size);
> +         depends on the target address size.  */
> +      cie->encoding = DW_EH_PE_absptr;
>  
>        /* Check version number.  */
>        cie_version = read_1_byte (unit->abfd, buf);
> @@ -1557,7 +1553,6 @@ dwarf2_build_frame_info (struct objfile 
>    /* Build a minimal decoding of the DWARF2 compilation unit.  */
>    unit.abfd = objfile->obfd;
>    unit.objfile = objfile;
> -  unit.addr_size = objfile->obfd->arch_info->bits_per_address / 8;
>    unit.dbase = 0;
>    unit.tbase = 0;
>  

Re: [RFA] dwarf2-frame.c sign extension patch

[Mark Kettenis]

   From: "Martin M. Hunt" <hunt@redhat.com>
   Date: Fri, 23 Jul 2004 10:54:08 -0700

   Mark,

   Yes, I like your patch and it works fine.  Please check it in. 

Thanks for testing,

Committed now.