[RFA] New option "trust-readonly-sections"

[RFA] New option "trust-readonly-sections"

[Michael Snyder]

This is an optimization mainly for remote debugging, or any
context where reading from the child's memory is expensive.

In a nutshell, if you trust that read-only sections will really
not be modified, you can advise GDB of this fact, and GDB will
then satisfy all memory reads from read-only sections by reading
from the object file, instead of from the child/target.

Naturally it defaults to 'off'.

On targets that do a lot of prologue analysis (which
involves lots of reads from the text section), this can
be a huge speed win.

2002-01-15  Michael Snyder  <msnyder@redhat.com>

	* target.c: New command, "set trust-readonly on".
	(do_xfer_memory): Honor the suggestion to trust readonly sections
	by reading them from the object file instead of from the target.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.30
diff -c -3 -p -r1.30 target.c
*** target.c	2002/01/09 00:36:58	1.30
--- target.c	2002/01/24 03:23:44
*************** target_write_memory (CORE_ADDR memaddr, 
*** 835,840 ****
--- 835,842 ----
    return target_xfer_memory (memaddr, myaddr, len, 1);
  }
  
+ static int trust_readonly = 0;
+ 
  /* 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.
  
*************** do_xfer_memory (CORE_ADDR memaddr, char 
*** 857,862 ****
--- 859,883 ----
       0.  */
    errno = 0;
  
+   if (!write && trust_readonly)
+     {
+       /* User-settable option, "trust-readonly".  If true, then
+ 	 memory from any SEC_READONLY bfd section may be read
+ 	 directly from the bfd file. */
+ 
+       struct section_table *secp;
+ 
+       for (secp = current_target.to_sections;
+ 	   secp < current_target.to_sections_end;
+ 	   secp++)
+ 	{
+ 	  /* FIXME: take it only if it's entirely within the section. */
+ 	  if (memaddr >= secp->addr && memaddr + len <= secp->endaddr)
+ 	    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.to_xfer_memory
      (memaddr, myaddr, len, write, attrib, &current_target);
*************** initialize_targets (void)
*** 2254,2266 ****
    add_info ("target", target_info, targ_desc);
    add_info ("files", target_info, targ_desc);
  
!   add_show_from_set (
! 		add_set_cmd ("target", class_maintenance, var_zinteger,
! 			     (char *) &targetdebug,
! 			     "Set target debugging.\n\
  When non-zero, target debugging is enabled.", &setdebuglist),
! 		      &showdebuglist);
  
  
    add_com ("monitor", class_obscure, do_monitor_command,
  	   "Send a command to the remote monitor (remote targets only).");
--- 2275,2295 ----
    add_info ("target", target_info, targ_desc);
    add_info ("files", target_info, targ_desc);
  
!   add_show_from_set 
!     (add_set_cmd ("target", class_maintenance, var_zinteger,
! 		  (char *) &targetdebug,
! 		  "Set target debugging.\n\
  When non-zero, target debugging is enabled.", &setdebuglist),
!      &showdebuglist);
  
+   add_show_from_set 
+     (add_set_cmd ("trust-readonly", class_support, 
+ 		  var_boolean, (char *) &trust_readonly, 
+ 		  "Set memory reads to trust SEC_READONLY section attribute.\n\
+ When active, memory from readonly sections (such as .text)\n\
+ will be read from the executable file instead of from target memory.",
+ 		  &setlist),
+      &showlist);
  
    add_com ("monitor", class_obscure, do_monitor_command,
  	   "Send a command to the remote monitor (remote targets only).");

Re: [RFA] New option "trust-readonly-sections"

[Andrew Cagney]

> !   add_show_from_set (
> ! 		add_set_cmd ("target", class_maintenance, var_zinteger,
> ! 			     (char *) &targetdebug,
> ! 			     "Set target debugging.\n\
>   When non-zero, target debugging is enabled.", &setdebuglist),
> ! 		      &showdebuglist);



Michael, suggest add_set_boolean_cmd().  Andrew

Re: [RFA] New option "trust-readonly-sections"

[Daniel Jacobowitz]

On Wed, Jan 23, 2002 at 07:23:18PM -0800, Michael Snyder wrote:
> 
> This is an optimization mainly for remote debugging, or any
> context where reading from the child's memory is expensive.
> 
> In a nutshell, if you trust that read-only sections will really
> not be modified, you can advise GDB of this fact, and GDB will
> then satisfy all memory reads from read-only sections by reading
> from the object file, instead of from the child/target.
> 
> Naturally it defaults to 'off'.
> 
> On targets that do a lot of prologue analysis (which
> involves lots of reads from the text section), this can
> be a huge speed win.
> 
> 2002-01-15  Michael Snyder  <msnyder@redhat.com>
> 
> 	* target.c: New command, "set trust-readonly on".
> 	(do_xfer_memory): Honor the suggestion to trust readonly sections
> 	by reading them from the object file instead of from the target.

<asbestos suit on>

I'd rather see this default to on.  If you give GDB a binary, it's
reasonable that GDB read from it - I though it did in a lot of cases,
but maybe I was mistaken.

If you know it will be modified, then documenting that you must tell
GDB that is reasonable.  IMHO.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] New option "trust-readonly-sections"

[Eli Zaretskii]

On Thu, 24 Jan 2002, Daniel Jacobowitz wrote:

> I'd rather see this default to on.

That would be an incompatible change.  I think we should avoid such 
changes, unless we have a very good reason.

Re: [RFA] New option "trust-readonly-sections"

[Daniel Jacobowitz]

On Thu, Jan 24, 2002 at 09:22:09AM +0200, Eli Zaretskii wrote:
> 
> On Thu, 24 Jan 2002, Daniel Jacobowitz wrote:
> 
> > I'd rather see this default to on.
> 
> That would be an incompatible change.  I think we should avoid such 
> changes, unless we have a very good reason.

Stan's reply was convincing.  i guess I've been spoiled by
protected-memory situations.

I'd personally like to object to your objection though, Eli. 
Performance can be a very good reason.  If it wasn't for the other
drawbacks, I'd consider the argument.

Perhaps I'm in the minority there, though.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] New option "trust-readonly-sections"

[Andrew Cagney]

> On Thu, Jan 24, 2002 at 09:22:09AM +0200, Eli Zaretskii wrote:
> 
>> 
>> On Thu, 24 Jan 2002, Daniel Jacobowitz wrote:
>> 
> 
>> > I'd rather see this default to on.
> 
>> 
>> That would be an incompatible change.  I think we should avoid such 
>> changes, unless we have a very good reason.
> 
> 
> Stan's reply was convincing.  i guess I've been spoiled by
> protected-memory situations.
> 
> I'd personally like to object to your objection though, Eli. 
> Performance can be a very good reason.  If it wasn't for the other
> drawbacks, I'd consider the argument.
> 
> Perhaps I'm in the minority there, though.


(Would you go near someone wearing an asbestos suit? :-)

It is really important that GDB doesn't lie.  If the tweek is safe then 
certainly enable it.  This tweek _isn't_ safe in embedded targets.

The same goes for things like breakpoints.  GDB pulls them so that the 
target is always left in a clean state.  Not pulling them would be a 
performance bost (knowing the numbers not as much as this one!).

BTW, there are other things that can also be done - for instance 
checking that the target text area hasn't changed.  There is a qCRC 
packet (but from memory it was argued that wasn't strong enough).

Andrew

Re: [RFA] New option "trust-readonly-sections"

[Andrew Cagney]

> The same goes for things like breakpoints.  GDB pulls them so that the target is always left in a clean state.  Not pulling them would be a performance bost (knowing the numbers not as much as this one!).


Er, that doesn't parse.

Michael's trust read only sections patch has a dramatic impact on 
performance.  It eliminates the need to read target memory before 
writing a breakpoint.

Andrew

Re: [RFA] New option "trust-readonly-sections"

[Daniel Jacobowitz]

On Thu, Jan 24, 2002 at 11:51:30AM -0500, Andrew Cagney wrote:
> >On Thu, Jan 24, 2002 at 09:22:09AM +0200, Eli Zaretskii wrote:
> >
> >>
> >>On Thu, 24 Jan 2002, Daniel Jacobowitz wrote:
> >>
> >
> >>> I'd rather see this default to on.
> >
> >>
> >>That would be an incompatible change.  I think we should avoid such 
> >>changes, unless we have a very good reason.
> >
> >
> >Stan's reply was convincing.  i guess I've been spoiled by
> >protected-memory situations.
> >
> >I'd personally like to object to your objection though, Eli. 
> >Performance can be a very good reason.  If it wasn't for the other
> >drawbacks, I'd consider the argument.

er... "drawbacks (that Stan pointed out to me), I'd argue with you
(Eli)".

> >
> >Perhaps I'm in the minority there, though.
> 
> 
> (Would you go near someone wearing an asbestos suit? :-)
> 
> It is really important that GDB doesn't lie.  If the tweek is safe then 
> certainly enable it.  This tweek _isn't_ safe in embedded targets.

Agreed.

> BTW, there are other things that can also be done - for instance 
> checking that the target text area hasn't changed.  There is a qCRC 
> packet (but from memory it was argued that wasn't strong enough).

Perhaps a qMD5 packet? :)

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] New option "trust-readonly-sections"

[Stan Shebs]

Daniel Jacobowitz wrote:
> 
> On Wed, Jan 23, 2002 at 07:23:18PM -0800, Michael Snyder wrote:
> >
> > [...]
> 
> <asbestos suit on>
> 
> I'd rather see this default to on.  If you give GDB a binary, it's
> reasonable that GDB read from it - I though it did in a lot of cases,
> but maybe I was mistaken.

This is not the first time that someone has tried their hand at
pruning target reads - Steve Chamberlain introduced a data
cache for instance.

Hard experience tells us that this is not something you want to
default to be on.  The problem is that most cross-debugging is
to non-memory-protected systems, which means that the supposedly
inviolate text section may very well be scribbled on by an
errant program.  In fact, since the program is buggy (that's
why you're using the debugger, right? :-) ), there is a very
good chance that the program is going to be modified without
you realizing it.  And that is *really* confusing - I experienced
this myself, and it's most peculiar to have a display/i $pc on,
be si'ing along, and have the effect of each step be quite
different from what the displayed instructions are telling
you should be happening.

For a flag like this, by defaulting to off, we lessen the
chance of unpleasant surprises for newer users, while the
more experienced risk-takers can turn it on in .gdbinit
and not think about it again (or at least until they get hosed
by the optimization :-) ).

Stan

Re: [RFA] New option "trust-readonly-sections"

[Michael Snyder]

Stan Shebs wrote:
> 
> Daniel Jacobowitz wrote:
> >
> > On Wed, Jan 23, 2002 at 07:23:18PM -0800, Michael Snyder wrote:
> > >
> > > [...]
> >
> > <asbestos suit on>
> >
> > I'd rather see this default to on.  If you give GDB a binary, it's
> > reasonable that GDB read from it - I though it did in a lot of cases,
> > but maybe I was mistaken.
> 
> This is not the first time that someone has tried their hand at
> pruning target reads - Steve Chamberlain introduced a data
> cache for instance.
> 
> Hard experience tells us that this is not something you want to
> default to be on.  The problem is that most cross-debugging is
> to non-memory-protected systems, which means that the supposedly
> inviolate text section may very well be scribbled on by an
> errant program.  In fact, since the program is buggy (that's
> why you're using the debugger, right? :-) ), there is a very
> good chance that the program is going to be modified without
> you realizing it.  And that is *really* confusing - I experienced
> this myself, and it's most peculiar to have a display/i $pc on,
> be si'ing along, and have the effect of each step be quite
> different from what the displayed instructions are telling
> you should be happening.
> 
> For a flag like this, by defaulting to off, we lessen the
> chance of unpleasant surprises for newer users, while the
> more experienced risk-takers can turn it on in .gdbinit
> and not think about it again (or at least until they get hosed
> by the optimization :-) ).

I agree with Stan -- I'd rather start out by being conservative.

What if we accept the conservative change for now, and 
then if somebody wants to they can look into adding a
further enhanced interface allowing specific back-ends
to change the default to "on" if they wish?

Re: [RFA] New option "trust-readonly-sections"

[Andrew Cagney]

> *************** do_xfer_memory (CORE_ADDR memaddr, char 
> *** 857,862 ****
> --- 859,883 ----
>        0.  */
>     errno = 0;
>   
> +   if (!write && trust_readonly)
> +     {
> +       /* User-settable option, "trust-readonly".  If true, then
> + 	 memory from any SEC_READONLY bfd section may be read
> + 	 directly from the bfd file. */
> + 
> +       struct section_table *secp;
> + 
> +       for (secp = current_target.to_sections;
> + 	   secp < current_target.to_sections_end;
> + 	   secp++)
> + 	{
> + 	  /* FIXME: take it only if it's entirely within the section. */
> + 	  if (memaddr >= secp->addr && memaddr + len <= secp->endaddr)
> + 	    return xfer_memory (memaddr, myaddr, len, 0, 
> + 				attrib, &current_target);


My understanding of do_xfer_memory() is that it is allowed to do partial 
transfers.  Hence, here, if the data doesn't all lie in the section, it 
is safe to truncate the transfer and return the number of bytes transfered.

Andrew

Re: [RFA] New option "trust-readonly-sections"

[Michael Snyder]

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

Andrew Cagney wrote:
> 
> > *************** do_xfer_memory (CORE_ADDR memaddr, char
> > *** 857,862 ****
> > --- 859,883 ----
> >        0.  */
> >     errno = 0;
> >
> > +   if (!write && trust_readonly)
> > +     {
> > +       /* User-settable option, "trust-readonly".  If true, then
> > +      memory from any SEC_READONLY bfd section may be read
> > +      directly from the bfd file. */
> > +
> > +       struct section_table *secp;
> > +
> > +       for (secp = current_target.to_sections;
> > +        secp < current_target.to_sections_end;
> > +        secp++)
> > +     {
> > +       /* FIXME: take it only if it's entirely within the section. */
> > +       if (memaddr >= secp->addr && memaddr + len <= secp->endaddr)
> > +         return xfer_memory (memaddr, myaddr, len, 0,
> > +                             attrib, &current_target);
> 
> My understanding of do_xfer_memory() is that it is allowed to do partial
> transfers.  Hence, here, if the data doesn't all lie in the section, it
> is safe to truncate the transfer and return the number of bytes transfered.

Thanks.  Implementing both this suggestion and your other one about 
using "add_set_boolean_cmd", I have committed the patch in the new
form attached below:

Eli may now nag me for doco, and you may nag me for a NEWS entry.  ;-)

[-- Attachment #2: trust.patch --]
[-- Type: text/plain, Size: 3185 bytes --]

2002-01-15  Michael Snyder  <msnyder@redhat.com>

	* target.c: New command, "set trust-readonly-sections on".
	(do_xfer_memory): Honor the suggestion to trust readonly sections
	by reading them from the object file instead of from the target.
	(initialize_targets): Register command "set trust-readonly-sections".

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.30
diff -p -r1.30 target.c
*** target.c	2002/01/09 00:36:58	1.30
--- target.c	2002/01/31 02:20:53
*************** target_write_memory (CORE_ADDR memaddr, 
*** 835,840 ****
--- 835,842 ----
    return target_xfer_memory (memaddr, myaddr, len, 1);
  }
  
+ static int trust_readonly = 0;
+ 
  /* 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.
  
*************** do_xfer_memory (CORE_ADDR memaddr, char 
*** 857,862 ****
--- 859,882 ----
       0.  */
    errno = 0;
  
+   if (!write && trust_readonly)
+     {
+       /* User-settable option, "trust-readonly".  If true, then
+ 	 memory from any SEC_READONLY bfd section may be read
+ 	 directly from the bfd file. */
+ 
+       struct section_table *secp;
+ 
+       for (secp = current_target.to_sections;
+ 	   secp < current_target.to_sections_end;
+ 	   secp++)
+ 	{
+ 	  if (memaddr >= secp->addr && memaddr < secp->endaddr)
+ 	    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.to_xfer_memory
      (memaddr, myaddr, len, write, attrib, &current_target);
*************** initialize_targets (void)
*** 2254,2266 ****
    add_info ("target", target_info, targ_desc);
    add_info ("files", target_info, targ_desc);
  
!   add_show_from_set (
! 		add_set_cmd ("target", class_maintenance, var_zinteger,
! 			     (char *) &targetdebug,
! 			     "Set target debugging.\n\
  When non-zero, target debugging is enabled.", &setdebuglist),
! 		      &showdebuglist);
  
  
    add_com ("monitor", class_obscure, do_monitor_command,
  	   "Send a command to the remote monitor (remote targets only).");
--- 2274,2296 ----
    add_info ("target", target_info, targ_desc);
    add_info ("files", target_info, targ_desc);
  
!   add_show_from_set 
!     (add_set_cmd ("target", class_maintenance, var_zinteger,
! 		  (char *) &targetdebug,
! 		  "Set target debugging.\n\
  When non-zero, target debugging is enabled.", &setdebuglist),
!      &showdebuglist);
  
+   add_show_from_set 
+     (add_set_boolean_cmd 
+      ("trust-readonly-sections", class_support, 
+       &trust_readonly, 
+       "Set mode for reading from readonly sections.\n\
+ When this mode is on, memory reads from readonly sections (such as .text)\n\
+ will be read from the object file instead of from the target.  This will\n\
+ result in significant performance improvement for remote targets.",
+       &setlist),
+      &showlist);
  
    add_com ("monitor", class_obscure, do_monitor_command,
  	   "Send a command to the remote monitor (remote targets only).");

Re: [RFA] New option "trust-readonly-sections"

[Eli Zaretskii]

> Date: Wed, 30 Jan 2002 18:18:47 -0800
> From: Michael Snyder <msnyder@redhat.com>
> 
> Eli may now nag me for doco, and you may nag me for a NEWS entry.  ;-)

Nag, nag.

(Since you've already committed the NEWS entry, it seems like I'm the
only nagger ;-)