[RFA] Ignore breakpoints when reading memory.

[RFA] Ignore breakpoints when reading memory.

[Vladimir Prus]

This commit prepares us for always-inserted-breakpoints mode.
If breakpoints are always inserted, then reading the code memory
will read the breakpoint instructions, not the original content.
This patch makes us try to restore the original comments using
the breakpoints table. OK?

- Volodya

	* breakpoint.h (breakpoint_restore_shadows): New
	declaration.
	* breakpoint.c (breakpoint_restore_shadows): New.
	(read_memory_nobpt): Use breakpoint_restore_shadows.
	* target.c (memory_xfer_partial): Call
	breakpoint_restore_shadows.
---
 gdb/breakpoint.c |   80 ++++++++++++++++++++++-------------------------------
 gdb/breakpoint.h |    4 +++
 gdb/target.c     |   13 +++++++--
 3 files changed, 47 insertions(+), 50 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 59fec71..4365a3a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -713,14 +713,23 @@ commands_from_control_command (char *arg, struct command_line *cmd)
 int
 read_memory_nobpt (CORE_ADDR memaddr, gdb_byte *myaddr, unsigned len)
 {
-  int status;
-  const struct bp_location *b;
+  int status = target_read_memory (memaddr, myaddr, len);
+  if (status)
+    return status;
+
+  breakpoint_restore_shadows (myaddr, memaddr, len);
+  return 0;  
+}
+
+void
+breakpoint_restore_shadows (gdb_byte *buf,
+			    ULONGEST memaddr, 
+			    LONGEST len)
+{
+  struct bp_location *b;
   CORE_ADDR bp_addr = 0;
   int bp_size = 0;
-
-  if (gdbarch_breakpoint_from_pc (current_gdbarch, &bp_addr, &bp_size) == NULL)
-    /* No breakpoints on this machine. */
-    return target_read_memory (memaddr, myaddr, len);
+  int bptoffset = 0;
 
   ALL_BP_LOCATIONS (b)
   {
@@ -739,60 +748,37 @@ read_memory_nobpt (CORE_ADDR memaddr, gdb_byte *myaddr, unsigned len)
     if (bp_size == 0)
       /* bp isn't valid, or doesn't shadow memory.  */
       continue;
+
     if (bp_addr + bp_size <= memaddr)
       /* The breakpoint is entirely before the chunk of memory we
          are reading.  */
       continue;
+
     if (bp_addr >= memaddr + len)
       /* The breakpoint is entirely after the chunk of memory we are
          reading. */
       continue;
-    /* Copy the breakpoint from the shadow contents, and recurse for
-       the things before and after.  */
-    {
-      /* Offset within shadow_contents.  */
-      int bptoffset = 0;
 
-      if (bp_addr < memaddr)
-	{
-	  /* Only copy the second part of the breakpoint.  */
-	  bp_size -= memaddr - bp_addr;
-	  bptoffset = memaddr - bp_addr;
-	  bp_addr = memaddr;
-	}
-
-      if (bp_addr + bp_size > memaddr + len)
-	{
-	  /* Only copy the first part of the breakpoint.  */
-	  bp_size -= (bp_addr + bp_size) - (memaddr + len);
-	}
-
-      memcpy (myaddr + bp_addr - memaddr,
-	      b->target_info.shadow_contents + bptoffset, bp_size);
+    /* Offset within shadow_contents.  */
+    if (bp_addr < memaddr)
+      {
+	/* Only copy the second part of the breakpoint.  */
+	bp_size -= memaddr - bp_addr;
+	bptoffset = memaddr - bp_addr;
+	bp_addr = memaddr;
+      }
 
-      if (bp_addr > memaddr)
-	{
-	  /* Copy the section of memory before the breakpoint.  */
-	  status = read_memory_nobpt (memaddr, myaddr, bp_addr - memaddr);
-	  if (status != 0)
-	    return status;
-	}
+    if (bp_addr + bp_size > memaddr + len)
+      {
+	/* Only copy the first part of the breakpoint.  */
+	bp_size -= (bp_addr + bp_size) - (memaddr + len);
+      }
 
-      if (bp_addr + bp_size < memaddr + len)
-	{
-	  /* Copy the section of memory after the breakpoint.  */
-	  status = read_memory_nobpt (bp_addr + bp_size,
-				      myaddr + bp_addr + bp_size - memaddr,
-				      memaddr + len - (bp_addr + bp_size));
-	  if (status != 0)
-	    return status;
-	}
-      return 0;
-    }
+    memcpy (buf + bp_addr - memaddr,
+	    b->target_info.shadow_contents + bptoffset, bp_size);
   }
-  /* Nothing overlaps.  Just call read_memory_noerr.  */
-  return target_read_memory (memaddr, myaddr, len);
 }
+
 \f
 
 /* A wrapper function for inserting catchpoints.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 3855485..9f98718 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -859,4 +859,8 @@ extern int deprecated_remove_raw_breakpoint (void *);
    target.  */
 int watchpoints_triggered (struct target_waitstatus *);
 
+extern void breakpoint_restore_shadows (gdb_byte *buf,
+					ULONGEST memaddr, 
+					LONGEST len);
+
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/target.c b/gdb/target.c
index d89a7fb..f19c54d 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1071,7 +1071,11 @@ memory_xfer_partial (struct target_ops *ops, void *readbuf, const void *writebuf
       if (res <= 0)
 	return -1;
       else
-	return res;
+	{
+	  if (readbuf)
+	    breakpoint_restore_shadows (readbuf, memaddr, reg_len);
+	  return res;
+	}
     }
 
   /* If none of those methods found the memory we wanted, fall back
@@ -1089,17 +1093,20 @@ memory_xfer_partial (struct target_ops *ops, void *readbuf, const void *writebuf
       res = ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
 				  readbuf, writebuf, memaddr, reg_len);
       if (res > 0)
-	return res;
+	break;
 
       /* We want to continue past core files to executables, but not
 	 past a running target's memory.  */
       if (ops->to_has_all_memory)
-	return res;
+	break;
 
       ops = ops->beneath;
     }
   while (ops != NULL);
 
+  if (readbuf)
+    breakpoint_restore_shadows (readbuf, memaddr, reg_len);
+
   /* If we still haven't got anything, return the last error.  We
      give up.  */
   return res;
-- 
1.5.3.5

Re: [RFA] Ignore breakpoints when reading memory.

[Mark Kettenis]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 1 Dec 2007 14:19:45 +0300
> 
> This commit prepares us for always-inserted-breakpoints mode.
> If breakpoints are always inserted, then reading the code memory
> will read the breakpoint instructions, not the original content.
> This patch makes us try to restore the original comments using
> the breakpoints table. OK?

So now reading from target memory will need to traverse the complete
list of inserted breakpoints.  Did you do any benchmarking to see what
the impact of this change is, especially when running on a somewhat
slow machine?

> 	* breakpoint.h (breakpoint_restore_shadows): New
> 	declaration.
> 	* breakpoint.c (breakpoint_restore_shadows): New.
> 	(read_memory_nobpt): Use breakpoint_restore_shadows.
> 	* target.c (memory_xfer_partial): Call
> 	breakpoint_restore_shadows.

Re: [RFA] Ignore breakpoints when reading memory.

[Jim Blandy]

Mark Kettenis <mark.kettenis at xs4all.nl> writes:
>> From: Vladimir Prus <vladimir@codesourcery.com>
>> Date: Sat, 1 Dec 2007 14:19:45 +0300
>> 
>> This commit prepares us for always-inserted-breakpoints mode.
>> If breakpoints are always inserted, then reading the code memory
>> will read the breakpoint instructions, not the original content.
>> This patch makes us try to restore the original comments using
>> the breakpoints table. OK?
>
> So now reading from target memory will need to traverse the complete
> list of inserted breakpoints.  Did you do any benchmarking to see what
> the impact of this change is, especially when running on a somewhat
> slow machine?

Without taking a position on whether this is a significant problem:

If we used some ordered data structure (we've got splay trees handy)
to hold the breakpoint locations sorted by address, then that would
permit better algorithms in some of the always-inserted breakpoint
code as well.  The comparison between the old and new breakpoint
location lists could be linear instead of quadratic.

Re: [RFA] Ignore breakpoints when reading memory.

[Daniel Jacobowitz]

On Sat, Dec 01, 2007 at 02:19:45PM +0300, Vladimir Prus wrote:
> 
> This commit prepares us for always-inserted-breakpoints mode.
> If breakpoints are always inserted, then reading the code memory
> will read the breakpoint instructions, not the original content.
> This patch makes us try to restore the original comments using
> the breakpoints table. OK?

Doesn't this make the extra call in read_memory_nobpt redundant?

Are there any sites that need to see the breakpoint?  For instance,
look at ppc_linux_memory_remove_breakpoint.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Ignore breakpoints when reading memory.

[Jim Blandy]

Daniel Jacobowitz <drow at false.org> writes:
> On Sat, Dec 01, 2007 at 02:19:45PM +0300, Vladimir Prus wrote:
>> 
>> This commit prepares us for always-inserted-breakpoints mode.
>> If breakpoints are always inserted, then reading the code memory
>> will read the breakpoint instructions, not the original content.
>> This patch makes us try to restore the original comments using
>> the breakpoints table. OK?
>
> Doesn't this make the extra call in read_memory_nobpt redundant?
>
> Are there any sites that need to see the breakpoint?  For instance,
> look at ppc_linux_memory_remove_breakpoint.

The code I'm presently working on would like to see breakpoints, to my
surprise.

Re: [RFA] Ignore breakpoints when reading memory.

[Vladimir Prus]

Jim Blandy wrote:

> 
> Daniel Jacobowitz <drow at false.org> writes:
>> On Sat, Dec 01, 2007 at 02:19:45PM +0300, Vladimir Prus wrote:
>>> 
>>> This commit prepares us for always-inserted-breakpoints mode.
>>> If breakpoints are always inserted, then reading the code memory
>>> will read the breakpoint instructions, not the original content.
>>> This patch makes us try to restore the original comments using
>>> the breakpoints table. OK?
>>
>> Doesn't this make the extra call in read_memory_nobpt redundant?
>>
>> Are there any sites that need to see the breakpoint?  For instance,
>> look at ppc_linux_memory_remove_breakpoint.
> 
> The code I'm presently working on would like to see breakpoints, to my
> surprise.

Hmm, why?

I see two solutions:
1. Add new parameter to target_xfer_partial, telling whether breakpoint
content should be shown or restored. Pass that parameter whenever we
need to see breakpoints. That would need quite a bit of churn
everywhere.

2. Don't touch memory_xfer_partial at all. Audit all places where code
memory is accessed, and call restore_breakpoint_shadows explicitly.
Lots of work, likewise, but also eliminates the performance concerns.

Comments?

- Volodya

Re: [RFA] Ignore breakpoints when reading memory.

[Jim Blandy]

On Dec 4, 2007 10:11 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 1 Dec 2007 14:19:45 +0300
> >
> > This commit prepares us for always-inserted-breakpoints mode.
> > If breakpoints are always inserted, then reading the code memory
> > will read the breakpoint instructions, not the original content.
> > This patch makes us try to restore the original comments using
> > the breakpoints table. OK?
>
> So now reading from target memory will need to traverse the complete
> list of inserted breakpoints.  Did you do any benchmarking to see what
> the impact of this change is, especially when running on a somewhat
> slow machine?

I measured this on my laptop (not slow), by timing 'runtest break.exp
call-ar-st.exp' nine times.  I got:

with patch:
user    (+ 1.57 1.55 1.51 1.58 1.64 1.59 1.68 1.54 1.53) 14.19

without patch:
user    (+ 1.55 1.61 1.54 1.52 1.58 1.60 1.55 1.58 1.61) 14.14

These times include the compilations, symbol table reading, and so on.
 It would have been better to write a custom expect script, and
perhaps add a GDB maintenance command to print the current running
total for CPU time.  But for what it is, the effect of the patch is in
the noise.

Re: [RFA] Ignore breakpoints when reading memory.

[Jim Blandy]

On Dec 5, 2007 1:34 PM, Jim Blandy <jimb@codesourcery.com> wrote:
> > Are there any sites that need to see the breakpoint?  For instance,
> > look at ppc_linux_memory_remove_breakpoint.
>
> The code I'm presently working on would like to see breakpoints, to my
> surprise.

This code evolved, and it no longer needs to see breakpoints.  I have
no objections to this patch.

Re: [RFA] Ignore breakpoints when reading memory.

[Vladimir Prus]

Jim Blandy wrote:

> On Dec 5, 2007 1:34 PM, Jim Blandy <jimb@codesourcery.com> wrote:
>> > Are there any sites that need to see the breakpoint?  For instance,
>> > look at ppc_linux_memory_remove_breakpoint.
>>
>> The code I'm presently working on would like to see breakpoints, to my
>> surprise.
> 
> This code evolved, and it no longer needs to see breakpoints.  I have
> no objections to this patch.

So, is it OK to commit?

- Volodya

Re: [RFA] Ignore breakpoints when reading memory.

[Daniel Jacobowitz]

On Mon, Jan 21, 2008 at 09:24:12PM +0300, Vladimir Prus wrote:
> So, is it OK to commit?

No, please see the questions from my mail upthread - it's clear that
there is at least one user that wants to check if the breakpoint is
still inserted so we need a way to do that.  And read_memory_nobpt
shouldn't hang around duplicating this work.

-- 
Daniel Jacobowitz
CodeSourcery