[PATCH 0/6] Share memory searching code between gdb and gdbserver

[PATCH 0/6] Share memory searching code between gdb and gdbserver

[Tom Tromey]

I happened to look at the memory searching code in gdb, and at the
bugs reported for the "find" command, and this series is the result.

The bulk of the change here is to share the underlying memory
searching implementation between gdb and gdbserver.  I did this in
anticipation of having to fix bugs in two nearly identical functions.

However, after doing so, I was unable to reproduce either of the
failures on file.  I did add a unit test to make this easier to test
later on.

Regression tested on x86-64 Fedora 32.  I also tested find.exp and
find-unmapped.exp using gdbserver.

Let me know what you think.

Tom

[PATCH 1/6] Rename some tests in find.exp

[Tom Tromey]

This renames some tests in find.exp, to avoid duplicate test names.

gdb/testsuite/ChangeLog
2020-07-23  Tom Tromey  <tromey@adacore.com>

	* gdb.base/find.exp: Rename some tests.
---
 gdb/testsuite/ChangeLog         |  4 ++++
 gdb/testsuite/gdb.base/find.exp | 12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/find.exp b/gdb/testsuite/gdb.base/find.exp
index ad512d51c78..23df0dab222 100644
--- a/gdb/testsuite/gdb.base/find.exp
+++ b/gdb/testsuite/gdb.base/find.exp
@@ -106,11 +106,11 @@ gdb_test_no_output "set int16_search_buf\[10\] = 0x1234" ""
 
 gdb_test "find /h &int16_search_buf\[0\], +sizeof(int16_search_buf), 0x1234" \
     "${hex_number}.*<int16_search_buf\\+20>${one_pattern_found}" \
-    "find 16-bit pattern"
+    "find 16-bit pattern /h"
 
 gdb_test "find &int16_search_buf\[0\], +sizeof(int16_search_buf), (int16_t) 0x1234" \
     "${hex_number}.*<int16_search_buf\\+20>${one_pattern_found}" \
-    "find 16-bit pattern"
+    "find 16-bit pattern cast"
 
 # Test 32-bit pattern.
 
@@ -118,11 +118,11 @@ gdb_test_no_output "set int32_search_buf\[10\] = 0x12345678" ""
 
 gdb_test "find &int32_search_buf\[0\], +sizeof(int32_search_buf), (int32_t) 0x12345678" \
     "${hex_number}.*<int32_search_buf\\+40>${one_pattern_found}" \
-    "find 32-bit pattern"
+    "find 32-bit pattern cast"
 
 gdb_test "find /w &int32_search_buf\[0\], +sizeof(int32_search_buf), 0x12345678" \
     "${hex_number}.*<int32_search_buf\\+40>${one_pattern_found}" \
-    "find 32-bit pattern"
+    "find 32-bit pattern /w"
 
 # Test 64-bit pattern.
 
@@ -130,11 +130,11 @@ gdb_test_no_output "set int64_search_buf\[10\] = 0xfedcba9876543210LL" ""
 
 gdb_test "find &int64_search_buf\[0\], +sizeof(int64_search_buf), (int64_t) 0xfedcba9876543210LL" \
     "${hex_number}.*<int64_search_buf\\+80>${one_pattern_found}" \
-    "find 64-bit pattern"
+    "find 64-bit pattern cast"
 
 gdb_test "find /g &int64_search_buf\[0\], +sizeof(int64_search_buf), 0xfedcba9876543210LL" \
     "${hex_number}.*<int64_search_buf\\+80>${one_pattern_found}" \
-    "find 64-bit pattern"
+    "find 64-bit pattern /g"
 
 # Test mixed-sized patterns.
 
-- 
2.26.2

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

[Tom Tromey]

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

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
-- 
2.26.2

[PATCH 3/6] Use simple_search_memory in gdbserver

[Tom Tromey]

This replaces gdbserver's memory-searching function with
simple_search_memory.

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

	* server.cc (handle_search_memory_1): Remove.
	(handle_search_memory): Use simple_search_memory.
	* Makefile.in (SFILES): Add target/search.c.
	(OBS): Add target/search.o.
---
 gdbserver/ChangeLog   |   7 +++
 gdbserver/Makefile.in |   2 +
 gdbserver/server.cc   | 112 ++----------------------------------------
 3 files changed, 14 insertions(+), 107 deletions(-)

diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index 9d7687be534..6e5449e3931 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -220,6 +220,7 @@ SFILES = \
 	$(srcdir)/../gdb/nat/ppc-linux.c \
 	$(srcdir)/../gdb/nat/riscv-linux-tdesc.c \
 	$(srcdir)/../gdb/nat/fork-inferior.c \
+	$(srcdir)/../gdb/target/search.c \
 	$(srcdir)/../gdb/target/waitstatus.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
@@ -247,6 +248,7 @@ OBS = \
 	tracepoint.o \
 	utils.o \
 	version.o \
+	target/search.o \
 	target/waitstatus.o \
 	$(DEPFILES) \
 	$(LIBOBJS) \
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index aadcb9b5d30..b050a1d8653 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1038,89 +1038,6 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
     }
 }
 
-/* Subroutine of handle_search_memory to simplify it.  */
-
-static int
-handle_search_memory_1 (CORE_ADDR start_addr, CORE_ADDR search_space_len,
-			gdb_byte *pattern, unsigned pattern_len,
-			gdb_byte *search_buf,
-			unsigned chunk_size, unsigned search_buf_size,
-			CORE_ADDR *found_addrp)
-{
-  /* Prime the search buffer.  */
-
-  if (gdb_read_memory (start_addr, search_buf, search_buf_size)
-      != search_buf_size)
-    {
-      warning ("Unable to access %ld bytes of target "
-	       "memory at 0x%lx, halting search.",
-	       (long) search_buf_size, (long) 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 = (search_space_len < search_buf_size
-				  ? search_space_len
-				  : search_buf_size);
-
-      found_ptr = (gdb_byte *) memmem (search_buf, nr_search_bytes, pattern,
-				       pattern_len);
-
-      if (found_ptr != NULL)
-	{
-	  CORE_ADDR found_addr = start_addr + (found_ptr - search_buf);
-	  *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.  */
-	  memcpy (search_buf, search_buf + chunk_size, keep_len);
-
-	  nr_to_read = (search_space_len - keep_len < chunk_size
-			? search_space_len - keep_len
-			: chunk_size);
-
-	  if (gdb_read_memory (read_addr, search_buf + keep_len,
-			       nr_to_read) != nr_to_read)
-	    {
-	      warning ("Unable to access %ld bytes of target memory "
-		       "at 0x%lx, halting search.",
-		       (long) nr_to_read, (long) read_addr);
-	      return -1;
-	    }
-
-	  start_addr += chunk_size;
-	}
-    }
-
-  /* Not found.  */
-
-  return 0;
-}
-
 /* Handle qSearch:memory packets.  */
 
 static void
@@ -1130,12 +1047,6 @@ handle_search_memory (char *own_buf, int packet_len)
   CORE_ADDR search_space_len;
   gdb_byte *pattern;
   unsigned int pattern_len;
-  /* 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.  */
-  gdb_byte *search_buf;
-  unsigned search_buf_size;
   int found;
   CORE_ADDR found_addr;
   int cmd_name_len = sizeof ("qSearch:memory:") - 1;
@@ -1158,25 +1069,13 @@ handle_search_memory (char *own_buf, int packet_len)
       return;
     }
 
-  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;
-
-  search_buf = (gdb_byte *) malloc (search_buf_size);
-  if (search_buf == NULL)
+  auto read_memory = [=] (CORE_ADDR addr, gdb_byte *result, size_t len)
     {
-      free (pattern);
-      error ("Unable to allocate memory to perform the search");
-      strcpy (own_buf, "E00");
-      return;
-    }
+      return gdb_read_memory (addr, result, len) == len;
+    };
 
-  found = handle_search_memory_1 (start_addr, search_space_len,
-				  pattern, pattern_len,
-				  search_buf, chunk_size, search_buf_size,
-				  &found_addr);
+  found = simple_search_memory (read_memory, start_addr, search_space_len,
+				pattern, pattern_len, &found_addr);
 
   if (found > 0)
     sprintf (own_buf, "1,%lx", (long) found_addr);
@@ -1185,7 +1084,6 @@ handle_search_memory (char *own_buf, int packet_len)
   else
     strcpy (own_buf, "E00");
 
-  free (search_buf);
   free (pattern);
 }
 
-- 
2.26.2

[PATCH 4/6] Remove some dead code from handle_search_memory

[Tom Tromey]

handle_search_memory had some code after a call to error.  This code
is dead, and this patch removes it.

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

	* server.cc (handle_search_memory): Remove dead code.
---
 gdbserver/ChangeLog | 4 ++++
 gdbserver/server.cc | 8 +-------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b050a1d8653..9af747cc0b7 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1053,11 +1053,7 @@ handle_search_memory (char *own_buf, int packet_len)
 
   pattern = (gdb_byte *) malloc (packet_len);
   if (pattern == NULL)
-    {
-      error ("Unable to allocate memory to perform the search");
-      strcpy (own_buf, "E00");
-      return;
-    }
+    error ("Unable to allocate memory to perform the search");
   if (decode_search_memory_packet (own_buf + cmd_name_len,
 				   packet_len - cmd_name_len,
 				   &start_addr, &search_space_len,
@@ -1065,8 +1061,6 @@ handle_search_memory (char *own_buf, int packet_len)
     {
       free (pattern);
       error ("Error in parsing qSearch:memory packet");
-      strcpy (own_buf, "E00");
-      return;
     }
 
   auto read_memory = [=] (CORE_ADDR addr, gdb_byte *result, size_t len)
-- 
2.26.2

[PATCH 5/6] Document inclusive range in help for "find"

[Tom Tromey]

PR gdb/16930 points out that the "find" command uses an inclusive
range; but this is not documented in the "help".  This patch updates
the help text.

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

	PR gdb/16930:
	* findcmd.c (_initialize_mem_search): Mention that the range is
	inclusive.
---
 gdb/ChangeLog | 6 ++++++
 gdb/findcmd.c | 1 +
 2 files changed, 7 insertions(+)

diff --git a/gdb/findcmd.c b/gdb/findcmd.c
index c8631571f20..3e4a790b589 100644
--- a/gdb/findcmd.c
+++ b/gdb/findcmd.c
@@ -292,6 +292,7 @@ find [/SIZE-CHAR] [/MAX-COUNT] START-ADDRESS, +LENGTH, EXPR1 [, EXPR2 ...]\n\
 SIZE-CHAR is one of b,h,w,g for 8,16,32,64 bit values respectively,\n\
 and if not specified the size is taken from the type of the expression\n\
 in the current language.\n\
+The two-address form specifies an inclusive range.\n\
 Note that this means for example that in the case of C-like languages\n\
 a search for an untyped 0x42 will search for \"(int) 0x42\"\n\
 which is typically four bytes, and a search for a string \"hello\" will\n\
-- 
2.26.2

[PATCH 6/6] Add simple_search_memory unit tests

[Tom Tromey]

This adds some unit tests for simple_search_memory.  I tried here to
reproduce some bugs (PR gdb/11158 and PR gdb/17756), but was unable
to.

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

	* unittests/search-memory-selftests.c: New file.
	* Makefile.in (SELFTESTS_SRCS): Add
	unittests/search-memory-selftests.c.
---
 gdb/ChangeLog                           |   6 ++
 gdb/Makefile.in                         |   1 +
 gdb/unittests/search-memory-selftests.c | 102 ++++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 gdb/unittests/search-memory-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 8feb5e39ff5..e279b8d944a 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -450,6 +450,7 @@ SELFTESTS_SRCS = \
 	unittests/scoped_fd-selftests.c \
 	unittests/scoped_mmap-selftests.c \
 	unittests/scoped_restore-selftests.c \
+	unittests/search-memory-selftests.c \
 	unittests/string_view-selftests.c \
 	unittests/style-selftests.c \
 	unittests/tracepoint-selftests.c \
diff --git a/gdb/unittests/search-memory-selftests.c b/gdb/unittests/search-memory-selftests.c
new file mode 100644
index 00000000000..4bf93756ed6
--- /dev/null
+++ b/gdb/unittests/search-memory-selftests.c
@@ -0,0 +1,102 @@
+/* Self tests for simple_search_memory for GDB, the GNU debugger.
+
+   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 "gdbsupport/selftest.h"
+#include "target/target.h"
+
+namespace selftests {
+namespace search_memory_tests {
+
+/* Note: Also defined in target/search.c.  */
+#define SEARCH_CHUNK_SIZE 16000
+
+static void
+run_tests ()
+{
+  size_t size = 2 * SEARCH_CHUNK_SIZE + 1;
+
+  std::vector<gdb_byte> data (size);
+  data[size - 1] = 'x';
+
+  bool read_fully = false;
+  bool read_off_end = false;
+  auto read_memory = [&] (CORE_ADDR from, gdb_byte *out, size_t len)
+    {
+      if (from + len > data.size ())
+	read_off_end = true;
+      else
+	memcpy (out, &data[from], len);
+      if (from + len == data.size ())
+	read_fully = true;
+      return true;
+    };
+
+  gdb_byte pattern = 'x';
+
+  CORE_ADDR addr = 0;
+  int result = simple_search_memory (read_memory, 0, data.size (),
+				     &pattern, 1, &addr);
+  /* In this case we don't care if read_fully was set or not.  */
+  SELF_CHECK (result == 1);
+  SELF_CHECK (!read_off_end);
+  SELF_CHECK (addr == size - 1);
+
+  addr = 0;
+  read_fully = false;
+  read_off_end = false;
+  pattern = 'q';
+  result = simple_search_memory (read_memory, 0, data.size (),
+				 &pattern, 1, &addr);
+  SELF_CHECK (result == 0);
+  SELF_CHECK (!read_off_end);
+  SELF_CHECK (read_fully);
+  SELF_CHECK (addr == 0);
+
+  /* Setup from PR gdb/17756.  */
+  size = 0x7bb00;
+  data = std::vector<gdb_byte> (size);
+  const CORE_ADDR base_addr = 0x08370000;
+  const gdb_byte wpattern[] = { 0x90, 0x8b, 0x98, 0x8 };
+  const CORE_ADDR found_addr = 0x837bac8;
+  memcpy (&data[found_addr - base_addr], wpattern, sizeof (wpattern));
+
+  auto read_memory_2 = [&] (CORE_ADDR from, gdb_byte *out, size_t len)
+    {
+      memcpy (out, &data[from - base_addr], len);
+      return true;
+    };
+
+  result = simple_search_memory (read_memory_2, base_addr, data.size (),
+				 wpattern, sizeof (wpattern), &addr);
+  SELF_CHECK (result == 1);
+  SELF_CHECK (addr == found_addr);
+}
+
+} /* namespace search_memory_tests */
+} /* namespace selftests */
+
+
+void _initialize_search_memory_selftests ();
+void
+_initialize_search_memory_selftests ()
+{
+  selftests::register_test ("search_memory",
+			    selftests::search_memory_tests::run_tests);
+}
-- 
2.26.2

Re: [PATCH 0/6] Share memory searching code between gdb and gdbserver

[Gary Benson]

Tom Tromey wrote:
> I happened to look at the memory searching code in gdb, and at the
> bugs reported for the "find" command, and this series is the result.
> 
> The bulk of the change here is to share the underlying memory
> searching implementation between gdb and gdbserver.  I did this in
> anticipation of having to fix bugs in two nearly identical functions.
> 
> However, after doing so, I was unable to reproduce either of the
> failures on file.  I did add a unit test to make this easier to test
> later on.
> 
> Regression tested on x86-64 Fedora 32.  I also tested find.exp and
> find-unmapped.exp using gdbserver.
> 
> Let me know what you think.

LGTM.  And gold stars for deduplicating stuff :D

Cheers,
Gary

-- 
Gary Benson - he / him / his
Principal Software Engineer, Red Hat

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

[Simon Marchi]

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

Re: [PATCH 3/6] Use simple_search_memory in gdbserver

[Simon Marchi]

On 2020-07-23 4:12 p.m., Tom Tromey wrote:
> @@ -1158,25 +1069,13 @@ handle_search_memory (char *own_buf, int packet_len)
>        return;
>      }
>  
> -  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;
> -
> -  search_buf = (gdb_byte *) malloc (search_buf_size);
> -  if (search_buf == NULL)
> +  auto read_memory = [=] (CORE_ADDR addr, gdb_byte *result, size_t len)

The capture is not needed here.

Simon

Re: [PATCH 4/6] Remove some dead code from handle_search_memory

[Simon Marchi]

On 2020-07-23 4:12 p.m., Tom Tromey wrote:
> handle_search_memory had some code after a call to error.  This code
> is dead, and this patch removes it.
> 
> 2020-07-23  Tom Tromey  <tromey@adacore.com>
> 
> 	* server.cc (handle_search_memory): Remove dead code.
> ---
>  gdbserver/ChangeLog | 4 ++++
>  gdbserver/server.cc | 8 +-------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index b050a1d8653..9af747cc0b7 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1053,11 +1053,7 @@ handle_search_memory (char *own_buf, int packet_len)
>  
>    pattern = (gdb_byte *) malloc (packet_len);
>    if (pattern == NULL)
> -    {
> -      error ("Unable to allocate memory to perform the search");
> -      strcpy (own_buf, "E00");
> -      return;
> -    }
> +    error ("Unable to allocate memory to perform the search");

Would you mind adding an empty line after this to visually separate it from the next block?

Or, can we use xmalloc/gdb::byte_vector and just not have this check?

Simon

Re: [PATCH 6/6] Add simple_search_memory unit tests

[Simon Marchi]

On 2020-07-23 4:12 p.m., Tom Tromey wrote:
> This adds some unit tests for simple_search_memory.  I tried here to
> reproduce some bugs (PR gdb/11158 and PR gdb/17756), but was unable
> to.
> 
> 2020-07-23  Tom Tromey  <tromey@adacore.com>
> 
> 	* unittests/search-memory-selftests.c: New file.
> 	* Makefile.in (SELFTESTS_SRCS): Add
> 	unittests/search-memory-selftests.c.
> ---
>  gdb/ChangeLog                           |   6 ++
>  gdb/Makefile.in                         |   1 +
>  gdb/unittests/search-memory-selftests.c | 102 ++++++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 gdb/unittests/search-memory-selftests.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 8feb5e39ff5..e279b8d944a 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -450,6 +450,7 @@ SELFTESTS_SRCS = \
>  	unittests/scoped_fd-selftests.c \
>  	unittests/scoped_mmap-selftests.c \
>  	unittests/scoped_restore-selftests.c \
> +	unittests/search-memory-selftests.c \
>  	unittests/string_view-selftests.c \
>  	unittests/style-selftests.c \
>  	unittests/tracepoint-selftests.c \
> diff --git a/gdb/unittests/search-memory-selftests.c b/gdb/unittests/search-memory-selftests.c
> new file mode 100644
> index 00000000000..4bf93756ed6
> --- /dev/null
> +++ b/gdb/unittests/search-memory-selftests.c
> @@ -0,0 +1,102 @@
> +/* Self tests for simple_search_memory for GDB, the GNU debugger.
> +
> +   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 "gdbsupport/selftest.h"
> +#include "target/target.h"
> +
> +namespace selftests {
> +namespace search_memory_tests {
> +
> +/* Note: Also defined in target/search.c.  */
> +#define SEARCH_CHUNK_SIZE 16000

I wonder if this should be defined in target/target.h instead.

> +
> +static void
> +run_tests ()
> +{
> +  size_t size = 2 * SEARCH_CHUNK_SIZE + 1;
> +
> +  std::vector<gdb_byte> data (size);
> +  data[size - 1] = 'x';
> +
> +  bool read_fully = false;
> +  bool read_off_end = false;
> +  auto read_memory = [&] (CORE_ADDR from, gdb_byte *out, size_t len)
> +    {
> +      if (from + len > data.size ())
> +	read_off_end = true;
> +      else
> +	memcpy (out, &data[from], len);
> +      if (from + len == data.size ())
> +	read_fully = true;
> +      return true;
> +    };
> +
> +  gdb_byte pattern = 'x';
> +
> +  CORE_ADDR addr = 0;
> +  int result = simple_search_memory (read_memory, 0, data.size (),
> +				     &pattern, 1, &addr);
> +  /* In this case we don't care if read_fully was set or not.  */
> +  SELF_CHECK (result == 1);
> +  SELF_CHECK (!read_off_end);
> +  SELF_CHECK (addr == size - 1);
> +
> +  addr = 0;
> +  read_fully = false;
> +  read_off_end = false;
> +  pattern = 'q';
> +  result = simple_search_memory (read_memory, 0, data.size (),
> +				 &pattern, 1, &addr);
> +  SELF_CHECK (result == 0);
> +  SELF_CHECK (!read_off_end);
> +  SELF_CHECK (read_fully);
> +  SELF_CHECK (addr == 0);
> +
> +  /* Setup from PR gdb/17756.  */
> +  size = 0x7bb00;
> +  data = std::vector<gdb_byte> (size);
> +  const CORE_ADDR base_addr = 0x08370000;
> +  const gdb_byte wpattern[] = { 0x90, 0x8b, 0x98, 0x8 };
> +  const CORE_ADDR found_addr = 0x837bac8;
> +  memcpy (&data[found_addr - base_addr], wpattern, sizeof (wpattern));
> +
> +  auto read_memory_2 = [&] (CORE_ADDR from, gdb_byte *out, size_t len)
> +    {
> +      memcpy (out, &data[from - base_addr], len);
> +      return true;
> +    };
> +
> +  result = simple_search_memory (read_memory_2, base_addr, data.size (),
> +				 wpattern, sizeof (wpattern), &addr);
> +  SELF_CHECK (result == 1);
> +  SELF_CHECK (addr == found_addr);
> +}

I compile with -D_GLIBCXX_DEBUG=1, and it aborts when running this test:

Running selftest search_memory.
/usr/include/c++/10.1.0/debug/vector:427:
In function:
    std::__debug::vector<_Tp, _Allocator>::reference
    std::__debug::vector<_Tp,
    _Allocator>::operator[](std::__debug::vector<_Tp,
    _Allocator>::size_type) [with _Tp = unsigned char; _Allocator =
    gdb::default_init_allocator<unsigned char, std::allocator<unsigned char>
    >; std::__debug::vector<_Tp, _Allocator>::reference = unsigned char&;
    std::__debug::vector<_Tp, _Allocator>::size_type = long unsigned int]

Error: attempt to subscript container with out-of-bounds index 16000, but
container only holds 16000 elements.

Objects involved in the operation:
    sequence "this" @ 0x0x7ffc6e54f870 {
      type = std::__debug::vector<unsigned char, gdb::default_init_allocator<unsigned char, std::allocator<unsigned char> > >;
    }

Simon

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

[Tom Tromey]

>>>>> "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

Re: [PATCH 6/6] Add simple_search_memory unit tests

[Tom Tromey]

>> +/* Note: Also defined in target/search.c.  */
>> +#define SEARCH_CHUNK_SIZE 16000

Simon> I wonder if this should be defined in target/target.h instead.

I'll move it when I move search.c.

Simon> I compile with -D_GLIBCXX_DEBUG=1, and it aborts when running this test:

Simon> Running selftest search_memory.
Simon> /usr/include/c++/10.1.0/debug/vector:427:
Simon> In function:
Simon>     std::__debug::vector<_Tp, _Allocator>::reference
Simon>     std::__debug::vector<_Tp,
_Allocator> ::operator[](std::__debug::vector<_Tp,
_Allocator> ::size_type) [with _Tp = unsigned char; _Allocator =
Simon>     gdb::default_init_allocator<unsigned char, std::allocator<unsigned char>
>> ; std::__debug::vector<_Tp, _Allocator>::reference = unsigned char&;
Simon>     std::__debug::vector<_Tp, _Allocator>::size_type = long unsigned int]

Simon> Error: attempt to subscript container with out-of-bounds index 16000, but
Simon> container only holds 16000 elements.

I fixed this.

Tom

Re: [PATCH 4/6] Remove some dead code from handle_search_memory

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> if (pattern == NULL)
>> -    {
>> -      error ("Unable to allocate memory to perform the search");
>> -      strcpy (own_buf, "E00");
>> -      return;
>> -    }
>> +    error ("Unable to allocate memory to perform the search");

Simon> Would you mind adding an empty line after this to visually
Simon> separate it from the next block?

No problem.

Simon> Or, can we use xmalloc/gdb::byte_vector and just not have this check?

Parts of gdbserver seem to be careful about malloc failures.
I don't know if it is important or not TBH.

Tom