[2/6] Move simple_search_memory to target/search.c

Message ID 20200723201216.3932150-3-tromey@adacore.com
State New
Headers show
Series
  • Share memory searching code between gdb and gdbserver
Related show

Commit Message

Tom Tromey July 23, 2020, 8:12 p.m.
This moves the simple_search_memory function to a new file,
target/search.c.  The API is slightly changed to make it more general.
This generality is useful for wiring it to gdbserver, and also for
unit testing.

2020-07-23  Tom Tromey  <tromey@adacore.com>

	* target/target.h (target_read_memory_ftype)
	(simple_search_memory): Declare.
	* target/search.c: New file.
	* target.h (simple_search_memory): Don't declare.
	* target.c (simple_search_memory): Move to target/search.c.
	(default_search_memory): Update.
	* remote.c (remote_target::search_memory): Update.
	* Makefile.in (SUBDIR_TARGET_SRCS): Add target/search.c.
---
 gdb/ChangeLog       |  11 ++++
 gdb/Makefile.in     |   2 +-
 gdb/remote.c        |  10 +++-
 gdb/target.c        | 109 +++------------------------------------
 gdb/target.h        |   8 ---
 gdb/target/search.c | 121 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/target/target.h |  18 +++++++
 7 files changed, 166 insertions(+), 113 deletions(-)
 create mode 100644 gdb/target/search.c

-- 
2.26.2

Comments

Simon Marchi July 30, 2020, 12:58 p.m. | #1
On 2020-07-23 4:12 p.m., Tom Tromey wrote:
> This moves the simple_search_memory function to a new file,

> target/search.c.  The API is slightly changed to make it more general.

> This generality is useful for wiring it to gdbserver, and also for

> unit testing.


What's the rationale for putting this under gdb/target/ instead of gdbsupport/?

I never really understood the point of target/.  It looks like the goal is to
share interfaces between gdb and gdbserver, having declarations in header files
in target/ and having different implementations in gdb and gdbserver.

Anyway, it's not a blocker in any case, just an honest question.

Simon
Tom Tromey Sept. 25, 2020, 7:02 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> On 2020-07-23 4:12 p.m., Tom Tromey wrote:
>> This moves the simple_search_memory function to a new file,

>> target/search.c.  The API is slightly changed to make it more general.

>> This generality is useful for wiring it to gdbserver, and also for

>> unit testing.


Simon> What's the rationale for putting this under gdb/target/ instead
Simon> of gdbsupport/?

I think originally I thought this code would rely on the target API, but
I see it does not.  I'll move it.

Simon> I never really understood the point of target/.  It looks like the goal is to
Simon> share interfaces between gdb and gdbserver, having declarations in header files
Simon> in target/ and having different implementations in gdb and gdbserver.

Yeah, at least sometimes I think.

Tom

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 9d484457397..8feb5e39ff5 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -461,7 +461,7 @@  SELFTESTS_SRCS = \
 
 SELFTESTS_OBS = $(patsubst %.c,%.o,$(SELFTESTS_SRCS))
 
-SUBDIR_TARGET_SRCS = target/waitstatus.c
+SUBDIR_TARGET_SRCS = target/search.c target/waitstatus.c
 SUBDIR_TARGET_OBS = $(patsubst %.c,%.o,$(SUBDIR_TARGET_SRCS))
 
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 59075cb09f2..2d728667da1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11181,6 +11181,12 @@  remote_target::search_memory (CORE_ADDR start_addr, ULONGEST search_space_len,
   int found;
   ULONGEST found_addr;
 
+  auto read_memory = [=] (CORE_ADDR addr, gdb_byte *result, size_t len)
+    {
+      return (target_read (this, TARGET_OBJECT_MEMORY, NULL, result, addr, len)
+	      == len);
+    };
+
   /* Don't go to the target if we don't have to.  This is done before
      checking packet_config_support to avoid the possibility that a
      success for this edge case means the facility works in
@@ -11200,7 +11206,7 @@  remote_target::search_memory (CORE_ADDR start_addr, ULONGEST search_space_len,
     {
       /* Target doesn't provided special support, fall back and use the
 	 standard support (copy memory and do the search here).  */
-      return simple_search_memory (this, start_addr, search_space_len,
+      return simple_search_memory (read_memory, start_addr, search_space_len,
 				   pattern, pattern_len, found_addrp);
     }
 
@@ -11232,7 +11238,7 @@  remote_target::search_memory (CORE_ADDR start_addr, ULONGEST search_space_len,
 	 supported.  If so, fall back to the simple way.  */
       if (packet_config_support (packet) == PACKET_DISABLE)
 	{
-	  return simple_search_memory (this, start_addr, search_space_len,
+	  return simple_search_memory (read_memory, start_addr, search_space_len,
 				       pattern, pattern_len, found_addrp);
 	}
       return -1;
diff --git a/gdb/target.c b/gdb/target.c
index 58189e62024..80ff4377df4 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2159,106 +2159,6 @@  target_read_description (struct target_ops *target)
   return target->read_description ();
 }
 
-/* This implements a basic search of memory, reading target memory and
-   performing the search here (as opposed to performing the search in on the
-   target side with, for example, gdbserver).  */
-
-int
-simple_search_memory (struct target_ops *ops,
-		      CORE_ADDR start_addr, ULONGEST search_space_len,
-		      const gdb_byte *pattern, ULONGEST pattern_len,
-		      CORE_ADDR *found_addrp)
-{
-  /* NOTE: also defined in find.c testcase.  */
-#define SEARCH_CHUNK_SIZE 16000
-  const unsigned chunk_size = SEARCH_CHUNK_SIZE;
-  /* Buffer to hold memory contents for searching.  */
-  unsigned search_buf_size;
-
-  search_buf_size = chunk_size + pattern_len - 1;
-
-  /* No point in trying to allocate a buffer larger than the search space.  */
-  if (search_space_len < search_buf_size)
-    search_buf_size = search_space_len;
-
-  gdb::byte_vector search_buf (search_buf_size);
-
-  /* Prime the search buffer.  */
-
-  if (target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-		   search_buf.data (), start_addr, search_buf_size)
-      != search_buf_size)
-    {
-      warning (_("Unable to access %s bytes of target "
-		 "memory at %s, halting search."),
-	       pulongest (search_buf_size), hex_string (start_addr));
-      return -1;
-    }
-
-  /* Perform the search.
-
-     The loop is kept simple by allocating [N + pattern-length - 1] bytes.
-     When we've scanned N bytes we copy the trailing bytes to the start and
-     read in another N bytes.  */
-
-  while (search_space_len >= pattern_len)
-    {
-      gdb_byte *found_ptr;
-      unsigned nr_search_bytes
-	= std::min (search_space_len, (ULONGEST) search_buf_size);
-
-      found_ptr = (gdb_byte *) memmem (search_buf.data (), nr_search_bytes,
-				       pattern, pattern_len);
-
-      if (found_ptr != NULL)
-	{
-	  CORE_ADDR found_addr = start_addr + (found_ptr - search_buf.data ());
-
-	  *found_addrp = found_addr;
-	  return 1;
-	}
-
-      /* Not found in this chunk, skip to next chunk.  */
-
-      /* Don't let search_space_len wrap here, it's unsigned.  */
-      if (search_space_len >= chunk_size)
-	search_space_len -= chunk_size;
-      else
-	search_space_len = 0;
-
-      if (search_space_len >= pattern_len)
-	{
-	  unsigned keep_len = search_buf_size - chunk_size;
-	  CORE_ADDR read_addr = start_addr + chunk_size + keep_len;
-	  int nr_to_read;
-
-	  /* Copy the trailing part of the previous iteration to the front
-	     of the buffer for the next iteration.  */
-	  gdb_assert (keep_len == pattern_len - 1);
-	  memcpy (&search_buf[0], &search_buf[chunk_size], keep_len);
-
-	  nr_to_read = std::min (search_space_len - keep_len,
-				 (ULONGEST) chunk_size);
-
-	  if (target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-			   &search_buf[keep_len], read_addr,
-			   nr_to_read) != nr_to_read)
-	    {
-	      warning (_("Unable to access %s bytes of target "
-			 "memory at %s, halting search."),
-		       plongest (nr_to_read),
-		       hex_string (read_addr));
-	      return -1;
-	    }
-
-	  start_addr += chunk_size;
-	}
-    }
-
-  /* Not found.  */
-
-  return 0;
-}
 
 /* Default implementation of memory-searching.  */
 
@@ -2268,9 +2168,14 @@  default_search_memory (struct target_ops *self,
 		       const gdb_byte *pattern, ULONGEST pattern_len,
 		       CORE_ADDR *found_addrp)
 {
+  auto read_memory = [=] (CORE_ADDR addr, gdb_byte *result, size_t len)
+    {
+      return target_read (current_top_target (), TARGET_OBJECT_MEMORY, NULL,
+			  result, addr, len) == len;
+    };
+
   /* Start over from the top of the target stack.  */
-  return simple_search_memory (current_top_target (),
-			       start_addr, search_space_len,
+  return simple_search_memory (read_memory, start_addr, search_space_len,
 			       pattern, pattern_len, found_addrp);
 }
 
diff --git a/gdb/target.h b/gdb/target.h
index 4e8d4cccd5c..563a1a50141 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2117,14 +2117,6 @@  extern const struct target_desc *target_read_description (struct target_ops *);
 #define target_get_ada_task_ptid(lwp, tid) \
      (current_top_target ()->get_ada_task_ptid) (lwp,tid)
 
-/* Utility implementation of searching memory.  */
-extern int simple_search_memory (struct target_ops* ops,
-                                 CORE_ADDR start_addr,
-                                 ULONGEST search_space_len,
-                                 const gdb_byte *pattern,
-                                 ULONGEST pattern_len,
-                                 CORE_ADDR *found_addrp);
-
 /* Main entry point for searching memory.  */
 extern int target_search_memory (CORE_ADDR start_addr,
                                  ULONGEST search_space_len,
diff --git a/gdb/target/search.c b/gdb/target/search.c
new file mode 100644
index 00000000000..9e8807e4c1f
--- /dev/null
+++ b/gdb/target/search.c
@@ -0,0 +1,121 @@ 
+/* Target memory searching
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "gdbsupport/common-defs.h"
+#include "target/target.h"
+
+#include "gdbsupport/byte-vector.h"
+
+/* This implements a basic search of memory, reading target memory and
+   performing the search here (as opposed to performing the search in on the
+   target side with, for example, gdbserver).  */
+
+int
+simple_search_memory
+  (gdb::function_view<target_read_memory_ftype> read_memory,
+   CORE_ADDR start_addr, ULONGEST search_space_len,
+   const gdb_byte *pattern, ULONGEST pattern_len,
+   CORE_ADDR *found_addrp)
+{
+  /* NOTE: also defined in find.c testcase.  */
+#define SEARCH_CHUNK_SIZE 16000
+  const unsigned chunk_size = SEARCH_CHUNK_SIZE;
+  /* Buffer to hold memory contents for searching.  */
+  unsigned search_buf_size;
+
+  search_buf_size = chunk_size + pattern_len - 1;
+
+  /* No point in trying to allocate a buffer larger than the search space.  */
+  if (search_space_len < search_buf_size)
+    search_buf_size = search_space_len;
+
+  gdb::byte_vector search_buf (search_buf_size);
+
+  /* Prime the search buffer.  */
+
+  if (!read_memory (start_addr, search_buf.data (), search_buf_size))
+    {
+      warning (_("Unable to access %s bytes of target "
+		 "memory at %s, halting search."),
+	       pulongest (search_buf_size), hex_string (start_addr));
+      return -1;
+    }
+
+  /* Perform the search.
+
+     The loop is kept simple by allocating [N + pattern-length - 1] bytes.
+     When we've scanned N bytes we copy the trailing bytes to the start and
+     read in another N bytes.  */
+
+  while (search_space_len >= pattern_len)
+    {
+      gdb_byte *found_ptr;
+      unsigned nr_search_bytes
+	= std::min (search_space_len, (ULONGEST) search_buf_size);
+
+      found_ptr = (gdb_byte *) memmem (search_buf.data (), nr_search_bytes,
+				       pattern, pattern_len);
+
+      if (found_ptr != NULL)
+	{
+	  CORE_ADDR found_addr = start_addr + (found_ptr - search_buf.data ());
+
+	  *found_addrp = found_addr;
+	  return 1;
+	}
+
+      /* Not found in this chunk, skip to next chunk.  */
+
+      /* Don't let search_space_len wrap here, it's unsigned.  */
+      if (search_space_len >= chunk_size)
+	search_space_len -= chunk_size;
+      else
+	search_space_len = 0;
+
+      if (search_space_len >= pattern_len)
+	{
+	  unsigned keep_len = search_buf_size - chunk_size;
+	  CORE_ADDR read_addr = start_addr + chunk_size + keep_len;
+	  int nr_to_read;
+
+	  /* Copy the trailing part of the previous iteration to the front
+	     of the buffer for the next iteration.  */
+	  gdb_assert (keep_len == pattern_len - 1);
+	  memcpy (&search_buf[0], &search_buf[chunk_size], keep_len);
+
+	  nr_to_read = std::min (search_space_len - keep_len,
+				 (ULONGEST) chunk_size);
+
+	  if (!read_memory (read_addr, &search_buf[keep_len], nr_to_read))
+	    {
+	      warning (_("Unable to access %s bytes of target "
+			 "memory at %s, halting search."),
+		       plongest (nr_to_read),
+		       hex_string (read_addr));
+	      return -1;
+	    }
+
+	  start_addr += chunk_size;
+	}
+    }
+
+  /* Not found.  */
+
+  return 0;
+}
diff --git a/gdb/target/target.h b/gdb/target/target.h
index a66459c2469..e0dd952e515 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -23,6 +23,8 @@ 
 #include "target/waitstatus.h"
 /* This header is a stopgap until more code is shared.  */
 
+#include "gdbsupport/function-view.h"
+
 /* Read LEN bytes of target memory at address MEMADDR, placing the
    results in GDB's memory at MYADDR.  Return zero for success,
    nonzero if any error occurs.  This function must be provided by
@@ -57,6 +59,22 @@  extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				ssize_t len);
 
+/* The type of a callback function that can be used to read memory.
+   Note that target_read_memory is not used here, because gdbserver
+   wants to be able to examine trace data when searching, and
+   target_read_memory does not do this.  */
+
+typedef bool target_read_memory_ftype (CORE_ADDR, gdb_byte *, size_t);
+
+/* Utility implementation of searching memory.  */
+extern int simple_search_memory
+  (gdb::function_view<target_read_memory_ftype> read_memory,
+   CORE_ADDR start_addr,
+   ULONGEST search_space_len,
+   const gdb_byte *pattern,
+   ULONGEST pattern_len,
+   CORE_ADDR *found_addrp);
+
 /* Cause the target to stop in a continuable fashion--for instance,
    under Unix, this should act like SIGSTOP--and wait for the target
    to be stopped before returning.  This function must be provided by