[rfa] Restore "trust-readonly-section"

[rfa] Restore "trust-readonly-section"

[Michael Snyder]

This seems to have succumbed to bit-rot -- there are new target-read 
functions
in target.c that don't pay any attention to this user-settable mode bit.

The purpose of "trust-readonly-sections" is to improve speed on
targets where reading memory is expensive (mostly remote).
It checks to see if a read is from a read-only section, and if so,
reads it from the exec file.  It defaults to "off" for safety, but if
users choose to use it, it really speeds up prologue analysis
(and therefore stepping).

This patch just makes it work again.

2005-05-12  Michael Snyder  <msnyder@redhat.com>

 * target.c (target_read_trusted): New function.
 Implements "trust-readonly-section".
 (target_xfer_partial): Honor trust-readonly-section.
 (do_xfer_memory): Ditto.
 (target_xfer_memory_partial): Ditto.
 (default_xfer_partial): Ditto.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.106
diff -p -r1.106 target.c
*** target.c 9 May 2005 21:20:35 -0000 1.106
--- target.c 12 May 2005 22:23:32 -0000
*************** target_section_by_addr (struct target_op
*** 849,854 ****
--- 849,880 ----
    return NULL;
  }

+ static int trust_readonly = 0;
+ static void
+ show_trust_readonly (struct ui_file *file, int from_tty,
+        struct cmd_list_element *c, const char *value)
+ {
+   fprintf_filtered (file, _("\
+ Mode for reading from readonly sections is %s.\n"),
+       value);
+ }
+
+ static int
+ target_read_trusted (CORE_ADDR memaddr, char *myaddr, int len)
+ {
+   struct section_table *secp;
+   /* User-settable option, "trust-readonly-sections".  If true,
+      then memory from any SEC_READONLY bfd section may be read
+      directly from the bfd file.  */
+   secp = target_section_by_addr (&current_target, memaddr);
+   if (secp != NULL
+       && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
+    & SEC_READONLY))
+     return xfer_memory (memaddr, myaddr, len, 0, NULL, &current_target);
+
+   return 0;
+ }
+
  /* Return non-zero when the target vector has supplied an xfer_partial
     method and it, rather than xfer_memory, should be used.  */
  static int
*************** target_xfer_partial (struct target_ops *
*** 864,870 ****
         void *readbuf, const void *writebuf,
         ULONGEST offset, LONGEST len)
  {
!   LONGEST retval;

    gdb_assert (ops->to_xfer_partial != NULL);
    retval = ops->to_xfer_partial (ops, object, annex, readbuf, writebuf,
--- 890,904 ----
         void *readbuf, const void *writebuf,
         ULONGEST offset, LONGEST len)
  {
!   LONGEST retval = -1;
!
!   /* No need to use target method if trusted readonly section can be
!      used (by user's wish).  */
!   if (trust_readonly
!       && object == TARGET_OBJECT_MEMORY
!       && readbuf != NULL)
!     if ((retval = target_read_trusted (offset, readbuf, len)) > 0)
!       return retval;

    gdb_assert (ops->to_xfer_partial != NULL);
    retval = ops->to_xfer_partial (ops, object, annex, readbuf, writebuf,
*************** target_stopped_data_address_p (struct ta
*** 1037,1052 ****
  }
  #endif

- static int trust_readonly = 0;
- static void
- show_trust_readonly (struct ui_file *file, int from_tty,
-        struct cmd_list_element *c, const char *value)
- {
-   fprintf_filtered (file, _("\
- Mode for reading from readonly sections is %s.\n"),
-       value);
- }
-
  /* Move memory to or from the targets.  The top target gets priority;
     if it cannot handle it, it is offered to the next one down, etc.

--- 1071,1076 ----
*************** do_xfer_memory (CORE_ADDR memaddr, char
*** 1069,1085 ****
    errno = 0;

    if (!write && trust_readonly)
!     {
!       struct section_table *secp;
!       /* User-settable option, "trust-readonly-sections".  If true,
!          then memory from any SEC_READONLY bfd section may be read
!          directly from the bfd file.  */
!       secp = target_section_by_addr (&current_target, memaddr);
!       if (secp != NULL
!    && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
!        & SEC_READONLY))
!  return xfer_memory (memaddr, myaddr, len, 0, attrib, &current_target);
!     }

    /* The quick case is that the top target can handle the transfer.  */
    res = current_target.deprecated_xfer_memory
--- 1093,1100 ----
    errno = 0;

    if (!write && trust_readonly)
!     if ((res = target_read_trusted (memaddr, myaddr, len)) > 0)
!       return res;

    /* The quick case is that the top target can handle the transfer.  */
    res = current_target.deprecated_xfer_memory
*************** target_xfer_memory_partial (CORE_ADDR me
*** 1199,1204 ****
--- 1214,1224 ----
        return 0;
      }

+   /* No need to use dcache if trusted readonly section can be used.  */
+   if (!write_p && trust_readonly)
+     if ((res = target_read_trusted (memaddr, myaddr, reg_len)) > 0)
+       return res;
+
    region = lookup_mem_region(memaddr);
    if (memaddr + len < region->hi)
      reg_len = len;
*************** default_xfer_partial (struct target_ops
*** 1308,1313 ****
--- 1328,1343 ----
          const char *annex, void *readbuf,
          const void *writebuf, ULONGEST offset, LONGEST len)
  {
+   LONGEST retval = -1;
+
+   /* No need to use target method if trusted readonly section can be
+      used (by user's wish).  */
+   if (trust_readonly
+       && object == TARGET_OBJECT_MEMORY
+       && readbuf != NULL)
+     if ((retval = target_read_trusted (offset, readbuf, len)) > 0)
+       return retval;
+
    if (object == TARGET_OBJECT_MEMORY
        && ops->deprecated_xfer_memory != NULL)
      /* If available, fall back to the target's

Re: [rfa] Restore "trust-readonly-section"

[Michael Snyder]

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

Hmm, tabs fubar -- I'll try again with the patch as an attachment.

----- Original Message ----- 
From: "Michael Snyder" <msnyder@redhat.com>
To: "GDB Patches" <gdb-patches@sources.redhat.com>
Sent: Thursday, May 12, 2005 3:55 PM
Subject: [rfa] Restore "trust-readonly-section"


> This seems to have succumbed to bit-rot -- there are new target-read 
> functions
> in target.c that don't pay any attention to this user-settable mode bit.
> 
> The purpose of "trust-readonly-sections" is to improve speed on
> targets where reading memory is expensive (mostly remote).
> It checks to see if a read is from a read-only section, and if so,
> reads it from the exec file.  It defaults to "off" for safety, but if
> users choose to use it, it really speeds up prologue analysis
> (and therefore stepping).
> 
> This patch just makes it work again.


[-- Attachment #2: trust_readonly --]
[-- Type: application/octet-stream, Size: 5302 bytes --]

2005-05-12  Michael Snyder  <msnyder@redhat.com>

	* target.c (target_read_trusted): New function.
	Implements "trust-readonly-section".
	(target_xfer_partial): Honor trust-readonly-section.
	(do_xfer_memory): Ditto.
	(target_xfer_memory_partial): Ditto.
	(default_xfer_partial): Ditto.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.106
diff -p -r1.106 target.c
*** target.c	9 May 2005 21:20:35 -0000	1.106
--- target.c	12 May 2005 22:23:32 -0000
*************** target_section_by_addr (struct target_op
*** 849,854 ****
--- 849,880 ----
    return NULL;
  }
  
+ static int trust_readonly = 0;
+ static void
+ show_trust_readonly (struct ui_file *file, int from_tty,
+ 		     struct cmd_list_element *c, const char *value)
+ {
+   fprintf_filtered (file, _("\
+ Mode for reading from readonly sections is %s.\n"),
+ 		    value);
+ }
+ 
+ static int
+ target_read_trusted (CORE_ADDR memaddr, char *myaddr, int len)
+ {
+   struct section_table *secp;
+   /* User-settable option, "trust-readonly-sections".  If true,
+      then memory from any SEC_READONLY bfd section may be read
+      directly from the bfd file.  */
+   secp = target_section_by_addr (&current_target, memaddr);
+   if (secp != NULL
+       && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
+ 	  & SEC_READONLY))
+     return xfer_memory (memaddr, myaddr, len, 0, NULL, &current_target);
+ 
+   return 0;
+ }
+ 
  /* Return non-zero when the target vector has supplied an xfer_partial
     method and it, rather than xfer_memory, should be used.  */
  static int
*************** target_xfer_partial (struct target_ops *
*** 864,870 ****
  		     void *readbuf, const void *writebuf,
  		     ULONGEST offset, LONGEST len)
  {
!   LONGEST retval;
  
    gdb_assert (ops->to_xfer_partial != NULL);
    retval = ops->to_xfer_partial (ops, object, annex, readbuf, writebuf,
--- 890,904 ----
  		     void *readbuf, const void *writebuf,
  		     ULONGEST offset, LONGEST len)
  {
!   LONGEST retval = -1;
! 
!   /* No need to use target method if trusted readonly section can be
!      used (by user's wish).  */
!   if (trust_readonly
!       && object == TARGET_OBJECT_MEMORY
!       && readbuf != NULL)
!     if ((retval = target_read_trusted (offset, readbuf, len)) > 0)
!       return retval;
  
    gdb_assert (ops->to_xfer_partial != NULL);
    retval = ops->to_xfer_partial (ops, object, annex, readbuf, writebuf,
*************** target_stopped_data_address_p (struct ta
*** 1037,1052 ****
  }
  #endif
  
- static int trust_readonly = 0;
- static void
- show_trust_readonly (struct ui_file *file, int from_tty,
- 		     struct cmd_list_element *c, const char *value)
- {
-   fprintf_filtered (file, _("\
- Mode for reading from readonly sections is %s.\n"),
- 		    value);
- }
- 
  /* Move memory to or from the targets.  The top target gets priority;
     if it cannot handle it, it is offered to the next one down, etc.
  
--- 1071,1076 ----
*************** do_xfer_memory (CORE_ADDR memaddr, char 
*** 1069,1085 ****
    errno = 0;
  
    if (!write && trust_readonly)
!     {
!       struct section_table *secp;
!       /* User-settable option, "trust-readonly-sections".  If true,
!          then memory from any SEC_READONLY bfd section may be read
!          directly from the bfd file.  */
!       secp = target_section_by_addr (&current_target, memaddr);
!       if (secp != NULL
! 	  && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
! 	      & SEC_READONLY))
! 	return xfer_memory (memaddr, myaddr, len, 0, attrib, &current_target);
!     }
  
    /* The quick case is that the top target can handle the transfer.  */
    res = current_target.deprecated_xfer_memory
--- 1093,1100 ----
    errno = 0;
  
    if (!write && trust_readonly)
!     if ((res = target_read_trusted (memaddr, myaddr, len)) > 0)
!       return res;
  
    /* The quick case is that the top target can handle the transfer.  */
    res = current_target.deprecated_xfer_memory
*************** target_xfer_memory_partial (CORE_ADDR me
*** 1199,1204 ****
--- 1214,1224 ----
        return 0;
      }
  
+   /* No need to use dcache if trusted readonly section can be used.  */
+   if (!write_p && trust_readonly)
+     if ((res = target_read_trusted (memaddr, myaddr, reg_len)) > 0)
+       return res;
+ 
    region = lookup_mem_region(memaddr);
    if (memaddr + len < region->hi)
      reg_len = len;
*************** default_xfer_partial (struct target_ops 
*** 1308,1313 ****
--- 1328,1343 ----
  		      const char *annex, void *readbuf, 
  		      const void *writebuf, ULONGEST offset, LONGEST len)
  {
+   LONGEST retval = -1;
+ 
+   /* No need to use target method if trusted readonly section can be
+      used (by user's wish).  */
+   if (trust_readonly
+       && object == TARGET_OBJECT_MEMORY
+       && readbuf != NULL)
+     if ((retval = target_read_trusted (offset, readbuf, len)) > 0)
+       return retval;
+ 
    if (object == TARGET_OBJECT_MEMORY
        && ops->deprecated_xfer_memory != NULL)
      /* If available, fall back to the target's

Re: [rfa] Restore "trust-readonly-section"

[Daniel Jacobowitz]

On Thu, May 12, 2005 at 04:14:19PM -0700, Michael Snyder wrote:
> Hmm, tabs fubar -- I'll try again with the patch as an attachment.
> 
> ----- Original Message ----- 
> From: "Michael Snyder" <msnyder@redhat.com>
> To: "GDB Patches" <gdb-patches@sources.redhat.com>
> Sent: Thursday, May 12, 2005 3:55 PM
> Subject: [rfa] Restore "trust-readonly-section"
> 
> 
> >This seems to have succumbed to bit-rot -- there are new target-read 
> >functions
> >in target.c that don't pay any attention to this user-settable mode bit.
> >
> >The purpose of "trust-readonly-sections" is to improve speed on
> >targets where reading memory is expensive (mostly remote).
> >It checks to see if a read is from a read-only section, and if so,
> >reads it from the exec file.  It defaults to "off" for safety, but if
> >users choose to use it, it really speeds up prologue analysis
> >(and therefore stepping).
> >
> >This patch just makes it work again.

It seems odd to add the test both in target_xfer_partial (a dispatcher)
and default_xfer_partial (an implementation).  Are they really both
necessary?

The code might be simpler if you push the trust_readonly check inside
target_read_trusted.  Also, could you name that something involving
memory?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [rfa] Restore "trust-readonly-section"

[Michael Snyder]

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

Daniel Jacobowitz wrote:
> On Thu, May 12, 2005 at 04:14:19PM -0700, Michael Snyder wrote:
> 
>>Hmm, tabs fubar -- I'll try again with the patch as an attachment.
>>
>>----- Original Message ----- 
>>From: "Michael Snyder" <msnyder@redhat.com>
>>To: "GDB Patches" <gdb-patches@sources.redhat.com>
>>Sent: Thursday, May 12, 2005 3:55 PM
>>Subject: [rfa] Restore "trust-readonly-section"
>>
>>
>>
>>>This seems to have succumbed to bit-rot -- there are new target-read 
>>>functions
>>>in target.c that don't pay any attention to this user-settable mode bit.
>>>
>>>The purpose of "trust-readonly-sections" is to improve speed on
>>>targets where reading memory is expensive (mostly remote).
>>>It checks to see if a read is from a read-only section, and if so,
>>>reads it from the exec file.  It defaults to "off" for safety, but if
>>>users choose to use it, it really speeds up prologue analysis
>>>(and therefore stepping).
>>>
>>>This patch just makes it work again.
> 
> 
> It seems odd to add the test both in target_xfer_partial (a dispatcher)
> and default_xfer_partial (an implementation).  Are they really both
> necessary?

I suppose you're right -- but I'm bewildered by the number of
possible entry points and paths thru this code.  In theory
I think I could get away with just covering three points:
target_read_memory, target_read_partial, and target_read_memory_partial,
were it not for the fact that do_xfer_memory is also an entry point.

do_xfer_memory seems to only be called from dcache.c --
but it's extern, so nothing prevents others from calling it.

And dcache_xfer_memory is extern too, so one can get in thru there
(although again, no one currently does AFAICT).

> The code might be simpler if you push the trust_readonly check inside
> target_read_trusted.  Also, could you name that something involving
> memory?

OK -- how about the attached, which includes four entry points?






[-- Attachment #2: trust-readonly --]
[-- Type: text/plain, Size: 5641 bytes --]

2005-05-12  Michael Snyder  <msnyder@redhat.com>

	* target.c (target_read_memory_trusted): New function.
	Implements "trust-readonly-section".
	(target_read_partial): Honor trust-readonly-section.
	(target_read_memory_partial): Ditto.
	(target_read_memory): Ditto.
	(do_xfer_memory): Ditto.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.108
diff -p -r1.108 target.c
*** target.c	16 May 2005 16:36:24 -0000	1.108
--- target.c	24 May 2005 01:00:20 -0000
*************** target_section_by_addr (struct target_op
*** 845,850 ****
--- 845,884 ----
    return NULL;
  }
  
+ static int trust_readonly = 0;
+ static void
+ show_trust_readonly (struct ui_file *file, int from_tty,
+ 		     struct cmd_list_element *c, const char *value)
+ {
+   fprintf_filtered (file, 
+ 		    _("Mode for reading from readonly sections is %s.\n"),
+ 		    value);
+ }
+ 
+ /* If trust-readonly-sections, and if MEMADDR is within a
+    read-only section, read LEN bytes from the bfd at MEMADDR,
+    placing the results in GDB's memory at MYADDR.  Returns
+    the number of bytes read.  */
+ 
+ static int
+ target_read_memory_trusted (CORE_ADDR memaddr, char *myaddr, int len)
+ {
+   struct section_table *secp;
+   /* User-settable option, "trust-readonly-sections".  If true,
+      then memory from any SEC_READONLY bfd section may be read
+      directly from the bfd file.  */
+   if (trust_readonly)
+     {
+       secp = target_section_by_addr (&current_target, memaddr);
+       if (secp != NULL
+ 	  && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
+ 	      & SEC_READONLY))
+ 	return xfer_memory (memaddr, myaddr, len, 0, NULL, &current_target);
+     }
+ 
+   return 0;
+ }
+ 
  /* Return non-zero when the target vector has supplied an xfer_partial
     method and it, rather than xfer_memory, should be used.  */
  static int
*************** xfer_using_stratum (enum target_object o
*** 999,1004 ****
--- 1033,1044 ----
  int
  target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
  {
+   int ret;
+ 
+   /* Honor "trust-readonly-sections" if set.  */
+   if ((ret = target_read_memory_trusted (memaddr, myaddr, len)) > 0)
+     return (ret != len);
+ 
    if (target_xfer_partial_p ())
      return xfer_using_stratum (TARGET_OBJECT_MEMORY, NULL,
  			       memaddr, len, myaddr, NULL);
*************** target_stopped_data_address_p (struct ta
*** 1033,1048 ****
  }
  #endif
  
- static int trust_readonly = 0;
- static void
- show_trust_readonly (struct ui_file *file, int from_tty,
- 		     struct cmd_list_element *c, const char *value)
- {
-   fprintf_filtered (file, _("\
- Mode for reading from readonly sections is %s.\n"),
- 		    value);
- }
- 
  /* Move memory to or from the targets.  The top target gets priority;
     if it cannot handle it, it is offered to the next one down, etc.
  
--- 1073,1078 ----
*************** do_xfer_memory (CORE_ADDR memaddr, gdb_b
*** 1060,1082 ****
    if (len == 0)
      return 0;
  
    /* deprecated_xfer_memory is not guaranteed to set errno, even when
       it returns 0.  */
    errno = 0;
  
-   if (!write && trust_readonly)
-     {
-       struct section_table *secp;
-       /* User-settable option, "trust-readonly-sections".  If true,
-          then memory from any SEC_READONLY bfd section may be read
-          directly from the bfd file.  */
-       secp = target_section_by_addr (&current_target, memaddr);
-       if (secp != NULL
- 	  && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
- 	      & SEC_READONLY))
- 	return xfer_memory (memaddr, myaddr, len, 0, attrib, &current_target);
-     }
- 
    /* The quick case is that the top target can handle the transfer.  */
    res = current_target.deprecated_xfer_memory
      (memaddr, myaddr, len, write, attrib, &current_target);
--- 1090,1104 ----
    if (len == 0)
      return 0;
  
+   /* Honor "trust-readonly-sections" if set.  */
+   if (!write && 
+       (res = target_read_memory_trusted (memaddr, myaddr, len)) > 0)
+ 	return res;
+ 
    /* deprecated_xfer_memory is not guaranteed to set errno, even when
       it returns 0.  */
    errno = 0;
  
    /* The quick case is that the top target can handle the transfer.  */
    res = current_target.deprecated_xfer_memory
      (memaddr, myaddr, len, write, attrib, &current_target);
*************** target_xfer_memory_partial (CORE_ADDR me
*** 1244,1253 ****
  int
  target_read_memory_partial (CORE_ADDR memaddr, char *buf, int len, int *err)
  {
    if (target_xfer_partial_p ())
      {
-       int retval;
- 
        retval = target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY,
  				    NULL, buf, NULL, memaddr, len);
  
--- 1266,1279 ----
  int
  target_read_memory_partial (CORE_ADDR memaddr, char *buf, int len, int *err)
  {
+   int retval;
+ 
+   /* Honor "trust-readonly-sections" if set.  */
+   if ((retval = target_read_memory_trusted (memaddr, buf, len)) > 0)
+     return retval;
+ 
    if (target_xfer_partial_p ())
      {
        retval = target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY,
  				    NULL, buf, NULL, memaddr, len);
  
*************** target_read_partial (struct target_ops *
*** 1351,1356 ****
--- 1377,1389 ----
  		     const char *annex, gdb_byte *buf,
  		     ULONGEST offset, LONGEST len)
  {
+   int ret;
+ 
+   /* Honor "trust-readonly-sections" if set.  */
+   if (object == TARGET_OBJECT_MEMORY)
+     if ((ret = target_read_memory_trusted (offset, buf, len)) > 0)
+       return ret;
+ 
    return target_xfer_partial (ops, object, annex, buf, NULL, offset, len);
  }
  

Re: [rfa] Restore "trust-readonly-section"

[Daniel Jacobowitz]

On Mon, May 23, 2005 at 06:03:49PM -0700, Michael Snyder wrote:
> I suppose you're right -- but I'm bewildered by the number of
> possible entry points and paths thru this code.  In theory
> I think I could get away with just covering three points:
> target_read_memory, target_read_partial, and target_read_memory_partial,
> were it not for the fact that do_xfer_memory is also an entry point.

Yes, there really are too many.  I hope that some day soon, we can
reduce the total number.

> do_xfer_memory seems to only be called from dcache.c --
> but it's extern, so nothing prevents others from calling it.
> 
> And dcache_xfer_memory is extern too, so one can get in thru there
> (although again, no one currently does AFAICT).
> 
> >The code might be simpler if you push the trust_readonly check inside
> >target_read_trusted.  Also, could you name that something involving
> >memory?
> 
> OK -- how about the attached, which includes four entry points?

This looks much nicer to me.  Thaks for revising!  One style concern;
I'd be happier if you avoided assignments in if statement.  But I'm not
much bothered about it either way.

> +   /* Honor "trust-readonly-sections" if set.  */
> +   if ((ret = target_read_memory_trusted (memaddr, myaddr, len)) > 0)
> +     return (ret != len);
> + 

This could be:
  if (target_read_memory_trusted (memaddr, myaddr, len) == len)
    return 0;

If it fails, might as well fall through.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [rfa] Restore "trust-readonly-section"

[Mark Kettenis]

   Date: Mon, 23 May 2005 18:03:49 -0700
   From: Michael Snyder <msnyder@redhat.com>

   I suppose you're right -- but I'm bewildered by the number of
   possible entry points and paths thru this code.  In theory
   I think I could get away with just covering three points:
   target_read_memory, target_read_partial, and target_read_memory_partial,
   were it not for the fact that do_xfer_memory is also an entry point.

I'd like to make the point here that the transition from
(deprecated_)xxx_xfer_memory to xxx_xfer_partial is taking us too long
and that we should actively work towards removing the deprecated stuff
instead of waiting for things to break.  The transition state we're in
now is much too confusing, at least that seems to be the case for me.

Mark