Use the address mask with addresses for SREC, etc.

Use the address mask with addresses for SREC, etc.

[Maciej W. Rozycki]

Hello,

 Some binary formats, such as SREC, only support 32-bit addresses and do 
not sign-extend them properly for targets that require it.  This causes a 
problem with the MIPS target when KSEG addresses are used as they have bit 
31 set which should be propagated through the high 32 bits to suit 64-bit 
BFD.  It is seen with the gdb.base/dump.exp set of tests.

 Here is a change that fixes the problem.  It has been successfully tested 
for mipsisa32-sde-elf, with the mips-sim-sde32/-EB, 
mips-sim-sde32/-mips16/-EB, mips-sim-sde32/-EL and 
mips-sim-sde32/-mips16/-EL target boards, removing all the 15 failures 
like below seen there:

(gdb) file intarr1.srec
Load new symbol table from ".../gdb/testsuite.mips-sim-sde32.-EL/intarr1.srec"? (y or n) y
Reading symbols from .../gdb/testsuite.mips-sim-sde32.-EL/intarr1.srec...done.
(gdb) print intarray
Cannot access memory at address 0x800220a8
(gdb) PASS: gdb.base/dump.exp: reload array as value, srec; capture intarray
FAIL: gdb.base/dump.exp: reload array as value, srec; value restored ok

2007-07-24  Nigel Stephens  <nigel@mips.com>
            Chris Dearman  <chris@mips.com>
            Maciej W. Rozycki  <macro@mips.com>

	* cli/cli-dump.c (restore_section_callback): Use the address mask
	when comparing addresses.
	* exec.c (xfer_memory): Likewise.

 OK to apply?

  Maciej

12100-3.diff
Index: binutils-quilt/src/gdb/exec.c
===================================================================
--- binutils-quilt.orig/src/gdb/exec.c	2007-07-23 18:59:51.000000000 +0100
+++ binutils-quilt/src/gdb/exec.c	2007-07-23 19:00:23.000000000 +0100
@@ -463,6 +463,8 @@
   struct section_table *p;
   CORE_ADDR nextsectaddr, memend;
   asection *section = NULL;
+  int addr_bit = gdbarch_addr_bit (current_gdbarch);
+  CORE_ADDR addr_mask;
 
   if (len <= 0)
     internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
@@ -474,30 +476,46 @@
 	memaddr = overlay_mapped_address (memaddr, section);
     }
 
+  /* Only match address bits which are significant
+     for the current architecture.  */
+  if (addr_bit < sizeof (CORE_ADDR) * HOST_CHAR_BIT)
+    {
+      addr_mask = ((CORE_ADDR)1 << addr_bit) - 1;
+      memaddr &= addr_mask;
+    }
+  else
+    addr_mask = ~(CORE_ADDR)0;
+
   memend = memaddr + len;
   nextsectaddr = memend;
 
   for (p = target->to_sections; p < target->to_sections_end; p++)
     {
+      CORE_ADDR p_addr;
+
       if (overlay_debugging && section && p->the_bfd_section &&
 	  strcmp (section->name, p->the_bfd_section->name) != 0)
 	continue;		/* not the section we need */
-      if (memaddr >= p->addr)
+
+      p_addr = p->addr & addr_mask;
+      if (memaddr >= p_addr)
         {
-	  if (memend <= p->endaddr)
+	  CORE_ADDR p_endaddr = p->endaddr & addr_mask;
+
+	  if (memend <= p_endaddr)
 	    {
 	      /* Entire transfer is within this section.  */
 	      if (write)
 		res = bfd_set_section_contents (p->bfd, p->the_bfd_section,
-						myaddr, memaddr - p->addr,
+						myaddr, memaddr - p_addr,
 						len);
 	      else
 		res = bfd_get_section_contents (p->bfd, p->the_bfd_section,
-						myaddr, memaddr - p->addr,
+						myaddr, memaddr - p_addr,
 						len);
 	      return (res != 0) ? len : 0;
 	    }
-	  else if (memaddr >= p->endaddr)
+	  else if (memaddr >= p_endaddr)
 	    {
 	      /* This section ends before the transfer starts.  */
 	      continue;
@@ -505,20 +523,20 @@
 	  else
 	    {
 	      /* This section overlaps the transfer.  Just do half.  */
-	      len = p->endaddr - memaddr;
+	      len = p_endaddr - memaddr;
 	      if (write)
 		res = bfd_set_section_contents (p->bfd, p->the_bfd_section,
-						myaddr, memaddr - p->addr,
+						myaddr, memaddr - p_addr,
 						len);
 	      else
 		res = bfd_get_section_contents (p->bfd, p->the_bfd_section,
-						myaddr, memaddr - p->addr,
+						myaddr, memaddr - p_addr,
 						len);
 	      return (res != 0) ? len : 0;
 	    }
         }
       else
-	nextsectaddr = min (nextsectaddr, p->addr);
+	nextsectaddr = min (nextsectaddr, p_addr);
     }
 
   if (nextsectaddr >= memend)
Index: binutils-quilt/src/gdb/cli/cli-dump.c
===================================================================
--- binutils-quilt.orig/src/gdb/cli/cli-dump.c	2007-07-23 18:59:51.000000000 +0100
+++ binutils-quilt/src/gdb/cli/cli-dump.c	2007-07-23 19:00:34.000000000 +0100
@@ -462,6 +462,8 @@
   bfd_vma sec_end    = sec_start + size;
   bfd_size_type sec_offset = 0;
   bfd_size_type sec_load_count = size;
+  int addr_bit = gdbarch_addr_bit (current_gdbarch);
+  CORE_ADDR addr_mask;
   struct cleanup *old_chain;
   gdb_byte *buf;
   int ret;
@@ -470,9 +472,17 @@
   if (!(bfd_get_section_flags (ibfd, isec) & SEC_LOAD))
     return;
 
-  /* Does the section overlap with the desired restore range? */
-  if (sec_end <= data->load_start 
-      || (data->load_end > 0 && sec_start >= data->load_end))
+  /* Only match address bits which are significant
+     for the current architecture.  */
+  if (addr_bit < sizeof (CORE_ADDR) * HOST_CHAR_BIT)
+    addr_mask = ((CORE_ADDR) 1 << addr_bit) - 1;
+  else
+    addr_mask = ~(CORE_ADDR) 0;
+
+  /* Does the section overlap with the desired restore range?  */
+  if (sec_end <= (data->load_start & addr_mask)
+      || ((data->load_end & addr_mask) > 0
+	  && sec_start >= (data->load_end & addr_mask)))
     {
       /* No, no useable data in this section. */
       printf_filtered (_("skipping section %s...\n"), 
@@ -483,11 +493,11 @@
   /* Compare section address range with user-requested
      address range (if any).  Compute where the actual
      transfer should start and end.  */
-  if (sec_start < data->load_start)
+  if (sec_start < (data->load_start & addr_mask))
     sec_offset = data->load_start - sec_start;
   /* Size of a partial transfer: */
   sec_load_count -= sec_offset;
-  if (data->load_end > 0 && sec_end > data->load_end)
+  if (data->load_end > 0 && sec_end > (data->load_end & addr_mask))
     sec_load_count -= sec_end - data->load_end;
 
   /* Get the data.  */

Re: Use the address mask with addresses for SREC, etc.

[Jim Blandy]

It's my understanding that addresses are signed on MIPS architectures,
and thus 32-bit SREC files are only capable of addressing locations
0xffffffff80000000 -- 0x000000007fffffff.  And that the issue you're
fixing here arises from BFD handing you addresses read from an SREC
file (which is using 32-bit addresses) in the 0x80000000 -- 0xffffffff
range.  Is that right?

I'm not too happy with complicating code in GDB because BFD is
providing it with the wrong addresses.  I have to imagine the same
thing would happen elsewhere.  When BFD reads the SREC file, does it
have any idea that it's a MIPS SREC file?  It looks like
bfd_get_sign_extend_vma doesn't know about MIPS SREC targets; would
fixing that, and then bfd/srec.c, help us get the right addresses into
the BFD?

Re: Use the address mask with addresses for SREC, etc.

[Maciej W. Rozycki]

On Wed, 25 Jul 2007, Jim Blandy wrote:

> It's my understanding that addresses are signed on MIPS architectures,
> and thus 32-bit SREC files are only capable of addressing locations
> 0xffffffff80000000 -- 0x000000007fffffff.  And that the issue you're
> fixing here arises from BFD handing you addresses read from an SREC
> file (which is using 32-bit addresses) in the 0x80000000 -- 0xffffffff
> range.  Is that right?

 That is correct.  The same applies to the HEX formats.  I gather this is 
more or less the reason dump.exp skips testing these formats for pure 
64-bit targets (but MIPS is "impure").  Note that the actual MIPS target 
may be 32-bit making the notion of the signedness of addresses irrelevant, 
but BFD is still 64-bit causing trouble inbetween.

> I'm not too happy with complicating code in GDB because BFD is
> providing it with the wrong addresses.  I have to imagine the same
> thing would happen elsewhere.  When BFD reads the SREC file, does it

 Well, the issue hits here and there across the whole src/ tree every once 
in a while.  It looks like MIPS is about the only oddball to have its 
addresses signed -- which is actually the result of how the (smart) 
extension from a 32-bit to a 64-bit CPU has been made (FYI, bits 31 and 63 
differentiate between kernel- and user-mode addresses for 32-bit and 
64-bit addressing respectively).

> have any idea that it's a MIPS SREC file?  It looks like
> bfd_get_sign_extend_vma doesn't know about MIPS SREC targets; would
> fixing that, and then bfd/srec.c, help us get the right addresses into
> the BFD?

 Well, AFAICS SREC and HEX files are target-agnostic, much like "binary" 
BFD.  The only possible way of handling it in BFD itself would be by 
sign-extending addresses at the "right point" if the destination BFD 
implies bfd_get_sign_extend_vma() true.  I somehow dislike hardcoding the 
"right point" in {ihex,srec,tekhex}.c, but given these formats appear to 
me as pure 32-bit, perhaps chosing bit 31 as the "right point" is OK.  
What do you think?

  Maciej

Re: Use the address mask with addresses for SREC, etc.

[Jim Blandy]

"Maciej W. Rozycki" <macro@mips.com> writes:
> On Wed, 25 Jul 2007, Jim Blandy wrote:
>> have any idea that it's a MIPS SREC file?  It looks like
>> bfd_get_sign_extend_vma doesn't know about MIPS SREC targets; would
>> fixing that, and then bfd/srec.c, help us get the right addresses into
>> the BFD?
>
>  Well, AFAICS SREC and HEX files are target-agnostic, much like "binary" 
> BFD.  The only possible way of handling it in BFD itself would be by 
> sign-extending addresses at the "right point" if the destination BFD 
> implies bfd_get_sign_extend_vma() true.  I somehow dislike hardcoding the 
> "right point" in {ihex,srec,tekhex}.c, but given these formats appear to 
> me as pure 32-bit, perhaps chosing bit 31 as the "right point" is OK.  
> What do you think?

For srec.c, that seems reasonable to me.  The logic there for writing
records will always output "negative" addresses as 32-bit values ---
it does not assume that s-records with two- or three-byte addresses
will be sign-extended from those lengths.  I haven't looked at ihex or
texhex.

The binutils folks will be the ones to approve the BFD changes, so
before you dive into this, you might want to explain your situation
there and get their general approval.  They might even have better
ideas.  :)