[PATCH v2 0/6] Share memory-searching code

[PATCH v2 0/6] Share memory-searching code

[Tom Tromey]

This is v2 of the series to share memory-searching code between gdb
and gdbserver.  I think this version addresses all the review
comments.

Tom

[PATCH v2 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-09-25  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 6e11b776b8b..68aef6c9935 100644
--- a/gdb/testsuite/gdb.base/find.exp
+++ b/gdb/testsuite/gdb.base/find.exp
@@ -109,11 +109,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.
 
@@ -121,11 +121,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.
 
@@ -133,11 +133,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 v2 2/6] Move simple_search_memory to gdbsupport/search.cc

[Tom Tromey]

This moves the simple_search_memory function to a new file,
gdbsupport/search.cc.  The API is slightly changed to make it more
general.  This generality is useful for wiring it to gdbserver, and
also for unit testing.

gdb/ChangeLog
2020-09-25  Tom Tromey  <tromey@adacore.com>

	* target.h (simple_search_memory): Don't declare.
	* target.c (simple_search_memory): Move to gdbsupport.
	(default_search_memory): Update.
	* remote.c (remote_target::search_memory): Update.

gdbsupport/ChangeLog
2020-09-25  Tom Tromey  <tromey@adacore.com>

	* Makefile.in: Rebuild.
	* Makefile.am (libgdbsupport_a_SOURCES): Add search.cc.
	* search.h: New file.
	* search.cc: New file.
---
 gdb/ChangeLog          |   7 +++
 gdb/remote.c           |  11 +++-
 gdb/target.c           | 110 +++----------------------------------
 gdb/target.h           |   8 ---
 gdbsupport/ChangeLog   |   7 +++
 gdbsupport/Makefile.am |   1 +
 gdbsupport/Makefile.in |   4 +-
 gdbsupport/search.cc   | 120 +++++++++++++++++++++++++++++++++++++++++
 gdbsupport/search.h    |  42 +++++++++++++++
 9 files changed, 197 insertions(+), 113 deletions(-)
 create mode 100644 gdbsupport/search.cc
 create mode 100644 gdbsupport/search.h

diff --git a/gdb/remote.c b/gdb/remote.c
index 5fc80ebc8f7..2c0fa4800f6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -75,6 +75,7 @@
 #include "gdbsupport/scoped_restore.h"
 #include "gdbsupport/environ.h"
 #include "gdbsupport/byte-vector.h"
+#include "gdbsupport/search.h"
 #include <algorithm>
 #include <unordered_map>
 #include "async-event.h"
@@ -11184,6 +11185,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
@@ -11203,7 +11210,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);
     }
 
@@ -11235,7 +11242,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 9fd6b4ba9e1..4a9f1fcd836 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -47,6 +47,7 @@
 #include "event-top.h"
 #include <algorithm>
 #include "gdbsupport/byte-vector.h"
+#include "gdbsupport/search.h"
 #include "terminal.h"
 #include <unordered_map>
 #include "target-connection.h"
@@ -2160,106 +2161,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.  */
 
@@ -2269,9 +2170,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 0cb92fa8ea8..17e1e890ef5 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2135,14 +2135,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/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index 9763c96cd3d..9f4ec938b98 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -65,6 +65,7 @@ libgdbsupport_a_SOURCES = \
     run-time-clock.cc \
     safe-strerror.cc \
     scoped_mmap.cc \
+    search.cc \
     signals.cc \
     signals-state-save-restore.cc \
     tdesc.cc \
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index 0223f6d6b35..044ef1555c2 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -156,7 +156,7 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
 	netstuff.$(OBJEXT) new-op.$(OBJEXT) pathstuff.$(OBJEXT) \
 	print-utils.$(OBJEXT) ptid.$(OBJEXT) rsp-low.$(OBJEXT) \
 	run-time-clock.$(OBJEXT) safe-strerror.$(OBJEXT) \
-	scoped_mmap.$(OBJEXT) signals.$(OBJEXT) \
+	scoped_mmap.$(OBJEXT) search.$(OBJEXT) signals.$(OBJEXT) \
 	signals-state-save-restore.$(OBJEXT) tdesc.$(OBJEXT) \
 	thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) $(am__objects_1)
 libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
@@ -388,6 +388,7 @@ libgdbsupport_a_SOURCES = \
     run-time-clock.cc \
     safe-strerror.cc \
     scoped_mmap.cc \
+    search.cc \
     signals.cc \
     signals-state-save-restore.cc \
     tdesc.cc \
@@ -492,6 +493,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/run-time-clock.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/safe-strerror.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/scoped_mmap.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/search.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/selftest.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/signals-state-save-restore.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/signals.Po@am__quote@
diff --git a/gdbsupport/search.cc b/gdbsupport/search.cc
new file mode 100644
index 00000000000..f10c57046cc
--- /dev/null
+++ b/gdbsupport/search.cc
@@ -0,0 +1,120 @@
+/* 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 "gdbsupport/search.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)
+{
+  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);
+	  if (keep_len > 0)
+	    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/gdbsupport/search.h b/gdbsupport/search.h
new file mode 100644
index 00000000000..886d80feaeb
--- /dev/null
+++ b/gdbsupport/search.h
@@ -0,0 +1,42 @@
+/* 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/>.  */
+
+#ifndef COMMON_SEARCH_H
+#define COMMON_SEARCH_H
+
+#include "gdbsupport/function-view.h"
+
+/* This is needed by the unit test, so appears here.  */
+#define SEARCH_CHUNK_SIZE 16000
+
+/* 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);
+
+#endif /* COMMON_SEARCH_H */
-- 
2.26.2

[PATCH v2 3/6] Use simple_search_memory in gdbserver

[Tom Tromey]

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

gdbserver/ChangeLog
2020-09-25  Tom Tromey  <tromey@adacore.com>

	* server.cc (handle_search_memory_1): Remove.
	(handle_search_memory): Use simple_search_memory.
---
 gdbserver/ChangeLog |   5 ++
 gdbserver/server.cc | 113 +++-----------------------------------------
 2 files changed, 11 insertions(+), 107 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index d45154d1f54..1c21660027e 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -49,6 +49,7 @@
 #include "gdbsupport/scope-exit.h"
 #include "gdbsupport/gdb_select.h"
 #include "gdbsupport/scoped_restore.h"
+#include "gdbsupport/search.h"
 
 #define require_running_or_return(BUF)		\
   if (!target_running ())			\
@@ -1038,89 +1039,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 +1048,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 +1070,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 +1085,6 @@ handle_search_memory (char *own_buf, int packet_len)
   else
     strcpy (own_buf, "E00");
 
-  free (search_buf);
   free (pattern);
 }
 
-- 
2.26.2

[PATCH v2 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.

gdbserver/ChangeLog
2020-09-25  Tom Tromey  <tromey@adacore.com>

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

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 1c21660027e..674462c9ae6 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1054,11 +1054,8 @@ 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,
@@ -1066,8 +1063,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 v2 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-09-25  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 v2 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-09-25  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 | 99 +++++++++++++++++++++++++
 3 files changed, 106 insertions(+)
 create mode 100644 gdb/unittests/search-memory-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index dbede7a9cfc..fcc518045b4 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -454,6 +454,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..5d2a1b9f906
--- /dev/null
+++ b/gdb/unittests/search-memory-selftests.c
@@ -0,0 +1,99 @@
+/* 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 "gdbsupport/search.h"
+
+namespace selftests {
+namespace search_memory_tests {
+
+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 v2 4/6] Remove some dead code from handle_search_memory

[Christian Biesinger]

On Fri, Sep 25, 2020 at 9:49 PM Tom Tromey <tromey@adacore.com> wrote:
> +++ b/gdbserver/server.cc
> @@ -1054,11 +1054,8 @@ 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;

I don't know much about gdbserver but if this code was trying to send
an error back maybe that code is actually needed?

Christian

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

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger <cbiesinger@google.com> writes:

Christian> On Fri, Sep 25, 2020 at 9:49 PM Tom Tromey <tromey@adacore.com> wrote:
>> +++ b/gdbserver/server.cc
>> @@ -1054,11 +1054,8 @@ 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;

Christian> I don't know much about gdbserver but if this code was trying to send
Christian> an error back maybe that code is actually needed?

'error' is noreturn, so I removed that code since it was dead.
Maybe whatever catches the exception sends an error.

Tom

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

[Christian Biesinger]

On Mon, Sep 28, 2020 at 9:46 PM Tom Tromey <tromey@adacore.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger <cbiesinger@google.com> writes:
>
> Christian> On Fri, Sep 25, 2020 at 9:49 PM Tom Tromey <tromey@adacore.com> wrote:
> >> +++ b/gdbserver/server.cc
> >> @@ -1054,11 +1054,8 @@ 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;
>
> Christian> I don't know much about gdbserver but if this code was trying to send
> Christian> an error back maybe that code is actually needed?
>
> 'error' is noreturn, so I removed that code since it was dead.
> Maybe whatever catches the exception sends an error.

Yeah sorry, I understand that your patch doesn't change what this
function does, I was just wondering if both the old and new code have
a bug that it should send an error but doesn't.

Christian

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

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger via Gdb-patches <gdb-patches@sourceware.org> writes:

>> 'error' is noreturn, so I removed that code since it was dead.
>> Maybe whatever catches the exception sends an error.

Christian> Yeah sorry, I understand that your patch doesn't change what this
Christian> function does, I was just wondering if both the old and new code have
Christian> a bug that it should send an error but doesn't.

Yeah, sorry about that - I should have realized you knew.

I think the try/catch in server.cc:captured_main will catch this
exception and write the error:

      catch (const gdb_exception_error &exception)
	{
	  fflush (stdout);
	  fprintf (stderr, "gdbserver: %s\n", exception.what ());

	  if (response_needed)
	    {
	      write_enn (cs.own_buf);
	      putpkt (cs.own_buf);
	    }

Tom

Re: [PATCH v2 0/6] Share memory-searching code

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This is v2 of the series to share memory-searching code between gdb
Tom> and gdbserver.  I think this version addresses all the review
Tom> comments.

I'm checking this in now.

Tom