[PATCH v4 0/6] Fix some Python Inferior methods

[PATCH v4 0/6] Fix some Python Inferior methods

[Tom Tromey]

A user pointed out a bug in Inferior.search_memory.  This series is
the result.  Most of the patches are minor things I noticed while
working on the final patch, which fixes the bug.

Regression tested on x86-64 Fedora 36.

---
Changes in v4:
- Addressed review comments: test case output checking
- Link to v3: https://inbox.sourceware.org/gdb-patches/20230713-py-inf-fixes-30615-v3-0-26a024f30553@adacore.com

Changes in v3:
- Added some comments to the final patch
- Link to v2: https://inbox.sourceware.org/gdb-patches/20230711-py-inf-fixes-30615-v2-0-64a2540864e6@adacore.com

Changes in v2:
- Addressed review comments
- Added scoped_restore_current_inferior_for_memory patch
- Link to v1: https://inbox.sourceware.org/gdb-patches/20230707-py-inf-fixes-30615-v1-0-7792ab559530@adacore.com

---
Tom Tromey (6):
      Minor cleanups in py-inferior.exp
      Refactor py-inferior.exp
      Rename Python variable in py-inferior.exp
      Remove obsolete comment from gdbthread.h
      Introduce scoped_restore_current_inferior_for_memory
      Use correct inferior in Inferior.read_memory et al

 gdb/aix-thread.c                         | 18 ++-----
 gdb/gdbthread.h                          |  2 +-
 gdb/inferior.h                           | 29 +++++++++++
 gdb/proc-service.c                       | 10 +---
 gdb/python/py-inferior.c                 | 43 +++++++++++++---
 gdb/testsuite/gdb.python/py-inferior.exp | 84 ++++++++++++++++++++++++--------
 6 files changed, 135 insertions(+), 51 deletions(-)
---
base-commit: 23e46b680f6fa6fce45aaf6c004cab6be322fbf1
change-id: 20230707-py-inf-fixes-30615-668ef09475ab

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH v4 1/6] Minor cleanups in py-inferior.exp

[Tom Tromey]

While working on this series, I noticed a some oddities in
py-inferior.exp.  One is an obviously incorrect comment, and the
others are confusing test names.  This patch fixes these.
---
 gdb/testsuite/gdb.python/py-inferior.exp | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 58deece5d5f..576c2b716e6 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -206,8 +206,7 @@ if [isnative] {
     }
 }
 
-# Test Inferior is_valid.  This must always be the last test in
-# this testcase as it kills the inferior.
+# Test Inferior is_valid.
 
 with_test_prefix "is_valid" {
     gdb_py_test_silent_cmd "python inf_list = gdb.inferiors()" "get initial list" 1
@@ -247,7 +246,7 @@ with_test_prefix "is_valid" {
     gdb_test "python print (inf_list\[1\].is_valid())" "True" \
 	"check inferior validity 3"
 
-    gdb_test_no_output "remove-inferiors 2" "remove-inferiors 3"
+    gdb_test_no_output "remove-inferiors 2"
     gdb_test "python print (inf_list\[0\].is_valid())" "True" \
 	"check inferior validity 4"
 
@@ -318,7 +317,7 @@ with_test_prefix "selected_inferior" {
 	"print a connection object"
 
     gdb_test "inferior 1" ".*" "switch back to first inferior"
-    gdb_test_no_output "remove-inferiors 3" "remove second inferior"
+    gdb_test_no_output "remove-inferiors 3"
 }
 
 # Test repr()/str()

-- 
2.40.1

[PATCH v4 2/6] Refactor py-inferior.exp

[Tom Tromey]

This changes py-inferior.exp to make it a bit more robust when adding
new inferiors during the course of the test.

Approved-By: Pedro Alves <pedro@palves.net>
---
 gdb/testsuite/gdb.python/py-inferior.exp | 43 +++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 576c2b716e6..723f212ecc3 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -40,6 +40,20 @@ if {![runto_main]} {
     return 0
 }
 
+# The most recently added inferior.
+set most_recent_inf 1
+
+# A helper function that adds a new inferior.  It returns the expected
+# number of the new inferior.  ARG is a string to pass to
+# add-inferior.
+proc add_inferior {{arg ""}} {
+    global most_recent_inf
+    incr most_recent_inf
+    gdb_test "add-inferior $arg" "Added inferior $most_recent_inf.*" \
+	"add inferior $most_recent_inf"
+    return $most_recent_inf
+}
+
 # Test basic gdb.Inferior attributes and methods.
 
 gdb_py_test_silent_cmd "python inferiors = gdb.inferiors ()" "get inferiors list" 1
@@ -234,7 +248,7 @@ with_test_prefix "is_valid" {
 	"gdb.events.inferior_deleted.connect(del_inf_handler)" "" \
 	"end" ""
 
-    gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
+    set num [add_inferior]
     gdb_py_test_silent_cmd "python inf_list = gdb.inferiors()" "get new list" 1
     gdb_test "python print (len(inf_list))" "2" "get inferior list length 2"
     gdb_test "python print (inf_list\[0\].is_valid())" "True" \
@@ -246,7 +260,7 @@ with_test_prefix "is_valid" {
     gdb_test "python print (inf_list\[1\].is_valid())" "True" \
 	"check inferior validity 3"
 
-    gdb_test_no_output "remove-inferiors 2"
+    gdb_test_no_output "remove-inferiors $num"
     gdb_test "python print (inf_list\[0\].is_valid())" "True" \
 	"check inferior validity 4"
 
@@ -287,15 +301,16 @@ with_test_prefix "selected_inferior" {
     # Figure out if inf 1 has a native target.
     set inf_1_is_native [gdb_is_target_native]
 
-    gdb_test "add-inferior -no-connection" "Added inferior 3" "create new inferior"
-    gdb_test "inferior 3" ".*" "switch to third inferior"
-    gdb_test "py print (gdb.selected_inferior().num)" "3" "third inferior selected"
+    set num [add_inferior "-no-connection"]
+    gdb_test "inferior $num" ".*" "switch to inferior $num"
+    gdb_test "py print (gdb.selected_inferior().num)" "$num" \
+	"inferior $num selected"
     gdb_test "py print (gdb.selected_inferior().connection_num)" "None" \
-	"third inferior's None connection number"
+	"inferior $num's None connection number"
     gdb_test "py print (gdb.selected_inferior().connection)" "None" \
-	"third inferior's None connection"
+	"inferior $num's None connection"
     gdb_test "target native" "Done.  Use the \"run\" command to start a process." \
-	"target for the third inferior"
+	"target for inferior $num"
 
     # If inf 1 has a native target, inf 3's target is shared with 1's.
     # Otherwise, it must have created a new target with a new number.
@@ -306,10 +321,10 @@ with_test_prefix "selected_inferior" {
     }
     gdb_test "py print (gdb.selected_inferior().connection_num)" \
 	"$expected_connection_num" \
-	"third inferior's native connection number"
+	"inferior $num's native connection number"
     gdb_test "py print (gdb.selected_inferior().connection.num)" \
 	"$expected_connection_num" \
-	"third inferior's native connection number, though connection object"
+	"inferior $num's native connection number, though connection object"
 
     # Test printing of gdb.TargetConnection object.
     gdb_test "py print (gdb.selected_inferior().connection)" \
@@ -317,18 +332,18 @@ with_test_prefix "selected_inferior" {
 	"print a connection object"
 
     gdb_test "inferior 1" ".*" "switch back to first inferior"
-    gdb_test_no_output "remove-inferiors 3"
+    gdb_test_no_output "remove-inferiors $num"
 }
 
 # Test repr()/str()
 with_test_prefix "__repr__" {
-    gdb_test "add-inferior" "Added inferior 4 on connection .*" "add inferior 4"
+    set num [add_inferior]
     gdb_py_test_silent_cmd "python infs = gdb.inferiors()" "get inferior list" 1
     gdb_test "python print (infs\[0\])" "<gdb.Inferior num=1, pid=$decimal>"
     gdb_test "python print (infs)" \
-	"\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior num=4, pid=$decimal>\\\)" \
+	"\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior num=$num, pid=$decimal>\\\)" \
 	"print all inferiors 1"
-    gdb_test_no_output "remove-inferiors 4"
+    gdb_test_no_output "remove-inferiors $num"
     gdb_test "python print (infs)" \
 	"\\\(<gdb.Inferior num=1, pid=$decimal>, <gdb.Inferior \\\(invalid\\\)>\\\)" \
 	"print all inferiors 2"

-- 
2.40.1

[PATCH v4 3/6] Rename Python variable in py-inferior.exp

[Tom Tromey]

py-inferior.exp creates a Python variable named 'str'.  This clashes
with the built-in type of the same name and can be confusing when
trying to evaluate Python code when debugging the test case.  This
patch renames it.

Approved-By: Pedro Alves <pedro@palves.net>
---
 gdb/testsuite/gdb.python/py-inferior.exp | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 723f212ecc3..8762b6992ca 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -88,13 +88,14 @@ gdb_continue_to_breakpoint "cont to Break here." ".*Break here\..*"
 
 gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \
   "read str address" 0
-gdb_py_test_silent_cmd "python str = gdb.inferiors()\[0\].read_memory (addr, 5); print(str)" \
-  "read str contents" 1
+gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
+    "<memory at $hex>" \
+    "read str contents"
 gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0
-gdb_py_test_silent_cmd "python str\[1\] = a" "change str" 0
-gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, str)" \
+gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0
+gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
   "write str" 1
-gdb_test "print (str)" " = \"hallo, testsuite\"" \
+gdb_test "print str" " = \"hallo, testsuite\"" \
   "ensure str was changed in the inferior"
 
 # Test memory search.

-- 
2.40.1

[PATCH v4 4/6] Remove obsolete comment from gdbthread.h

[Tom Tromey]

A comment in gdbthread.h refers to a global that no longer exists.

Approved-By: Pedro Alves <pedro@palves.net>
---
 gdb/gdbthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 7135515bf45..d294be6762b 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -764,7 +764,7 @@ extern int thread_count (process_stratum_target *proc_target);
 /* Return true if we have any thread in any inferior.  */
 extern bool any_thread_p ();
 
-/* Switch context to thread THR.  Also sets the STOP_PC global.  */
+/* Switch context to thread THR.  */
 extern void switch_to_thread (struct thread_info *thr);
 
 /* Switch context to no thread selected.  */

-- 
2.40.1

[PATCH v4 5/6] Introduce scoped_restore_current_inferior_for_memory

[Tom Tromey]

This introduces a new class,
scoped_restore_current_inferior_for_memory, and arranges to use it in
a few places.  This class is intended to handle setting up and
restoring the various globals that are needed to read or write memory
-- but without invalidating the frame cache.

I wasn't able to test the change to aix-thread.c.

Approved-By: Pedro Alves <pedro@palves.net>
---
 gdb/aix-thread.c   | 18 ++++--------------
 gdb/inferior.h     | 29 +++++++++++++++++++++++++++++
 gdb/proc-service.c | 10 ++--------
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index fbe80d656c2..74cc67c942b 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -615,13 +615,8 @@ pdc_read_data (pthdb_user_t user_current_pid, void *buf,
   /* This is needed to eliminate the dependency of current thread
      which is null so that thread reads the correct target memory.  */
   {
-    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-    inferior_ptid = ptid_t (user_current_pid);
-    scoped_restore_current_inferior restore_inferior;
-    set_current_inferior (inf);
-
-    scoped_restore_current_program_space restore_current_progspace;
-    set_current_program_space (inf->pspace);
+    scoped_restore_current_inferior_for_memory save_inferior
+      (inf, ptid_t (user_current_pid));
     status = target_read_memory (addr, (gdb_byte *) buf, len);
   }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -648,13 +643,8 @@ pdc_write_data (pthdb_user_t user_current_pid, void *buf,
 		user_current_pid, (long) buf, hex_string (addr), len);
 
   {
-    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-    inferior_ptid = ptid_t (user_current_pid);
-    scoped_restore_current_inferior restore_inferior;
-    set_current_inferior (inf);
-
-    scoped_restore_current_program_space restore_current_progspace;
-    set_current_program_space (inf->pspace);
+    scoped_restore_current_inferior_for_memory save_inferior
+      (inf, ptid_t (user_current_pid));
     status = target_write_memory (addr, (gdb_byte *) buf, len);
   }
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index caa8e4d494a..be76c456c8c 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -761,6 +761,35 @@ class scoped_restore_current_inferior
   inferior *m_saved_inf;
 };
 
+/* When reading memory from an inferior, the global inferior_ptid must
+   also be set.  This class arranges to save and restore the necessary
+   state for reading or writing memory, but without invalidating the
+   frame cache.  */
+
+class scoped_restore_current_inferior_for_memory
+{
+public:
+
+  /* Save the current globals and switch to the given inferior and the
+     inferior's program space.  PTID must name a thread in INF, it is
+     used as the new inferior_ptid.  */
+  scoped_restore_current_inferior_for_memory (inferior *inf, ptid_t ptid)
+    : m_save_ptid (&inferior_ptid)
+  {
+    set_current_inferior (inf);
+    set_current_program_space (inf->pspace);
+    inferior_ptid = ptid;
+  }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_restore_current_inferior_for_memory);
+
+private:
+
+  scoped_restore_current_inferior m_save_inferior;
+  scoped_restore_current_program_space m_save_progspace;
+  scoped_restore_tmpl<ptid_t> m_save_ptid;
+};
+
 
 /* Traverse all inferiors.  */
 
diff --git a/gdb/proc-service.c b/gdb/proc-service.c
index 509836ec1a8..366e0510070 100644
--- a/gdb/proc-service.c
+++ b/gdb/proc-service.c
@@ -72,14 +72,8 @@ static ps_err_e
 ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
 		gdb_byte *buf, size_t len, int write)
 {
-  scoped_restore_current_inferior restore_inferior;
-  set_current_inferior (ph->thread->inf);
-
-  scoped_restore_current_program_space restore_current_progspace;
-  set_current_program_space (ph->thread->inf->pspace);
-
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = ph->thread->ptid;
+  scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf,
+							    ph->thread->ptid);
 
   CORE_ADDR core_addr = ps_addr_to_core_addr (addr);
 

-- 
2.40.1

[PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Tom Tromey]

A user noticed that Inferior.read_memory and a few other Python APIs
will always use the currently selected inferior, not the one passed to
the call.

This patch fixes the bug by arranging to switch to the inferior.  I
found this same issue in several APIs, so this fixes them all.

I also added a few missing calls to INFPY_REQUIRE_VALID to these
methods.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615
---
 gdb/python/py-inferior.c                 | 43 ++++++++++++++++++++++++++------
 gdb/testsuite/gdb.python/py-inferior.exp | 27 ++++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index af8bd8855a3..f6000b944da 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -30,6 +30,7 @@
 #include "gdbsupport/gdb_signals.h"
 #include "py-event.h"
 #include "py-stopevent.h"
+#include "progspace-and-thread.h"
 #include <unordered_map>
 
 using thread_map_t
@@ -528,11 +529,14 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
 static PyObject *
 infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
 {
+  inferior_object *inf = (inferior_object *) self;
   CORE_ADDR addr, length;
   gdb::unique_xmalloc_ptr<gdb_byte> buffer;
   PyObject *addr_obj, *length_obj;
   static const char *keywords[] = { "address", "length", NULL };
 
+  INFPY_REQUIRE_VALID (inf);
+
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OO", keywords,
 					&addr_obj, &length_obj))
     return NULL;
@@ -543,6 +547,11 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
 
   try
     {
+      /* Use this scoped-restore because we want to be able to read
+	 memory from an unwinder.  */
+      scoped_restore_current_inferior_for_memory restore_inferior
+	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+
       buffer.reset ((gdb_byte *) xmalloc (length));
 
       read_memory (addr, buffer.get (), length);
@@ -565,6 +574,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
 static PyObject *
 infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 {
+  inferior_object *inf = (inferior_object *) self;
   struct gdb_exception except;
   Py_ssize_t buf_len;
   const gdb_byte *buffer;
@@ -573,6 +583,8 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
   static const char *keywords[] = { "address", "buffer", "length", NULL };
   Py_buffer pybuf;
 
+  INFPY_REQUIRE_VALID (inf);
+
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords,
 					&addr_obj, &pybuf, &length_obj))
     return NULL;
@@ -591,6 +603,13 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 
   try
     {
+      /* It's probably not too important to avoid invalidating the
+	 frame cache when writing memory, but this scoped-restore is
+	 still used here, just to keep the code similar to other code
+	 in this file.  */
+      scoped_restore_current_inferior_for_memory restore_inferior
+	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+
       write_memory_with_notification (addr, buffer, length);
     }
   catch (gdb_exception &ex)
@@ -604,7 +623,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 }
 
 /* Implementation of
-   gdb.search_memory (address, length, pattern).  ADDRESS is the
+   Inferior.search_memory (address, length, pattern).  ADDRESS is the
    address to start the search.  LENGTH specifies the scope of the
    search from ADDRESS.  PATTERN is the pattern to search for (and
    must be a Python object supporting the buffer protocol).
@@ -614,6 +633,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 static PyObject *
 infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 {
+  inferior_object *inf = (inferior_object *) self;
   struct gdb_exception except;
   CORE_ADDR start_addr, length;
   static const char *keywords[] = { "address", "length", "pattern", NULL };
@@ -624,6 +644,8 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
   int found = 0;
   Py_buffer pybuf;
 
+  INFPY_REQUIRE_VALID (inf);
+
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OOs*", keywords,
 					&start_addr_obj, &length_obj,
 					&pybuf))
@@ -656,6 +678,13 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 
   try
     {
+      /* It's probably not too important to avoid invalidating the
+	 frame cache when searching memory, but this scoped-restore is
+	 still used here, just to keep the code similar to other code
+	 in this file.  */
+      scoped_restore_current_inferior_for_memory restore_inferior
+	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+
       found = target_search_memory (start_addr, length,
 				    buffer, pattern_size,
 				    &found_addr);
@@ -910,12 +939,12 @@ infpy_get_main_name (PyObject *self, void *closure)
   try
     {
       /* This is unfortunate but the implementation of main_name can
-	 reach into memory.  */
-      scoped_restore_current_inferior restore_inferior;
-      set_current_inferior (inf->inferior);
-
-      scoped_restore_current_program_space restore_current_progspace;
-      set_current_program_space (inf->inferior->pspace);
+	 reach into memory.  It's probably not too important to avoid
+	 invalidating the frame cache here, but this scoped-restore is
+	 still used, just to keep the code similar to other code in
+	 this file.  */
+      scoped_restore_current_inferior_for_memory restore_inferior
+	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
 
       name = main_name ();
     }
diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 8762b6992ca..13beebd08cc 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -91,6 +91,7 @@ gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \
 gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
     "<memory at $hex>" \
     "read str contents"
+gdb_test "python print(astr\[0\])" "b'h'"
 gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0
 gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0
 gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
@@ -98,6 +99,10 @@ gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
 gdb_test "print str" " = \"hallo, testsuite\"" \
   "ensure str was changed in the inferior"
 
+# Add a new inferior here, so we can test that operations work on the
+# correct inferior.
+set num [add_inferior]
+
 # Test memory search.
 
 set hex_number {0x[0-9a-fA-F][0-9a-fA-F]*}
@@ -115,6 +120,10 @@ with_test_prefix "string" {
     gdb_test_no_output "py start_addr = search_buf.address"
     gdb_test_no_output "py length = search_buf.type.sizeof"
 
+    # Switch to the new inferior before testing.
+    gdb_test "inferior $num" "Switching to inferior $num.*" \
+	"switch to inferior $num"
+
     gdb_test "py print (gdb.inferiors()\[0\].search_memory (start_addr, length, 'aaa'))" \
 	"${one_pattern_found}" "find string pattern"
 
@@ -128,6 +137,24 @@ with_test_prefix "string" {
 	"${one_pattern_found}" "pattern found at end of range"
 }
 
+# While still in the new inferior, test reading and writing memory
+# again.
+gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
+    "<memory at $hex>" \
+    "read str while other inferior selected"
+gdb_test "python print(astr\[1\])" "b'a'" \
+    "print a character from the string"
+gdb_py_test_silent_cmd "python astr\[1\] = b'X'" "change str again" 0
+gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
+    "write str while other inferior selected" 1
+
+gdb_test "inferior 1" "Switching to inferior 1.*" "switch back to inferior 1"
+
+gdb_test "print str" " = \"hXllo, testsuite\"" \
+    "ensure str was changed while other inferior selected"
+
+gdb_test_no_output "remove-inferiors $num" "remove-inferiors $num"
+
 # Import struct to pack the following patterns.
 gdb_test_no_output "py from struct import *"
 

-- 
2.40.1

Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Andrew Burgess]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> A user noticed that Inferior.read_memory and a few other Python APIs
> will always use the currently selected inferior, not the one passed to
> the call.
>
> This patch fixes the bug by arranging to switch to the inferior.  I
> found this same issue in several APIs, so this fixes them all.
>
> I also added a few missing calls to INFPY_REQUIRE_VALID to these
> methods.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615
> ---
>  gdb/python/py-inferior.c                 | 43 ++++++++++++++++++++++++++------
>  gdb/testsuite/gdb.python/py-inferior.exp | 27 ++++++++++++++++++++
>  2 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index af8bd8855a3..f6000b944da 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -30,6 +30,7 @@
>  #include "gdbsupport/gdb_signals.h"
>  #include "py-event.h"
>  #include "py-stopevent.h"
> +#include "progspace-and-thread.h"
>  #include <unordered_map>
>  
>  using thread_map_t
> @@ -528,11 +529,14 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
>  static PyObject *
>  infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  {
> +  inferior_object *inf = (inferior_object *) self;
>    CORE_ADDR addr, length;
>    gdb::unique_xmalloc_ptr<gdb_byte> buffer;
>    PyObject *addr_obj, *length_obj;
>    static const char *keywords[] = { "address", "length", NULL };
>  
> +  INFPY_REQUIRE_VALID (inf);
> +
>    if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OO", keywords,
>  					&addr_obj, &length_obj))
>      return NULL;
> @@ -543,6 +547,11 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      /* Use this scoped-restore because we want to be able to read
> +	 memory from an unwinder.  */
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
> +
>        buffer.reset ((gdb_byte *) xmalloc (length));
>  
>        read_memory (addr, buffer.get (), length);
> @@ -565,6 +574,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  static PyObject *
>  infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  {
> +  inferior_object *inf = (inferior_object *) self;
>    struct gdb_exception except;
>    Py_ssize_t buf_len;
>    const gdb_byte *buffer;
> @@ -573,6 +583,8 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>    static const char *keywords[] = { "address", "buffer", "length", NULL };
>    Py_buffer pybuf;
>  
> +  INFPY_REQUIRE_VALID (inf);
> +
>    if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords,
>  					&addr_obj, &pybuf, &length_obj))
>      return NULL;
> @@ -591,6 +603,13 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      /* It's probably not too important to avoid invalidating the
> +	 frame cache when writing memory, but this scoped-restore is
> +	 still used here, just to keep the code similar to other code
> +	 in this file.  */
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
> +
>        write_memory_with_notification (addr, buffer, length);
>      }
>    catch (gdb_exception &ex)
> @@ -604,7 +623,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  }
>  
>  /* Implementation of
> -   gdb.search_memory (address, length, pattern).  ADDRESS is the
> +   Inferior.search_memory (address, length, pattern).  ADDRESS is the
>     address to start the search.  LENGTH specifies the scope of the
>     search from ADDRESS.  PATTERN is the pattern to search for (and
>     must be a Python object supporting the buffer protocol).
> @@ -614,6 +633,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  static PyObject *
>  infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>  {
> +  inferior_object *inf = (inferior_object *) self;
>    struct gdb_exception except;
>    CORE_ADDR start_addr, length;
>    static const char *keywords[] = { "address", "length", "pattern", NULL };
> @@ -624,6 +644,8 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>    int found = 0;
>    Py_buffer pybuf;
>  
> +  INFPY_REQUIRE_VALID (inf);
> +
>    if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OOs*", keywords,
>  					&start_addr_obj, &length_obj,
>  					&pybuf))
> @@ -656,6 +678,13 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      /* It's probably not too important to avoid invalidating the
> +	 frame cache when searching memory, but this scoped-restore is
> +	 still used here, just to keep the code similar to other code
> +	 in this file.  */
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
> +
>        found = target_search_memory (start_addr, length,
>  				    buffer, pattern_size,
>  				    &found_addr);
> @@ -910,12 +939,12 @@ infpy_get_main_name (PyObject *self, void *closure)
>    try
>      {
>        /* This is unfortunate but the implementation of main_name can
> -	 reach into memory.  */
> -      scoped_restore_current_inferior restore_inferior;
> -      set_current_inferior (inf->inferior);
> -
> -      scoped_restore_current_program_space restore_current_progspace;
> -      set_current_program_space (inf->inferior->pspace);
> +	 reach into memory.  It's probably not too important to avoid
> +	 invalidating the frame cache here, but this scoped-restore is
> +	 still used, just to keep the code similar to other code in
> +	 this file.  */
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);

I'm seeing this change break gdb.dap/stop-at-main.exp.  The reason is
that Inferior.main_name is called before the inferior is started (in
order to place a temporary breakpoint on e.g. 'main').

However, any_thread_of_inferior asserts that inf->inferior has a
non-zero pid, i.e. the inferior is associated with an actual process.

Before the inferior starts this is not the case.

Which, I feels leads to the question: how does the 'start' command work?
It also calls main_name, and does so before the inferior has started.

Turns out, 'start' doesn't do anything special.  You call
scoped_restore_current_inferior_for_memory in order to setup
inferior_ptid, but at the point 'start' calls 'main_name', inferior_ptid
is null_ptid, so any memory accesses will fail.

I think Ada is the only language where we might try to read memory in
order to resolve the main_name, so I tried (at random)
gdb.ada/bp_on_var.exp, I built the executable, stripped the debug, and
started GDB.

When I see is:

  Thread 1 "gdb" hit Breakpoint 3, ada_main_name () at ../../src.dev-7/gdb/ada-lang.c:800
  800	  struct bound_minimal_symbol msym;
  (top-gdb) n
  801	  static gdb::unique_xmalloc_ptr<char> main_program_name;
  (top-gdb) p inferior_ptid
  $1 = {
    m_pid = 0,
    m_lwp = 0,
    m_tid = 0
  }
  (top-gdb) n
  808	  msym = lookup_minimal_symbol (ADA_MAIN_PROGRAM_SYMBOL_NAME, NULL, NULL);
  (top-gdb) n
  810	  if (msym.minsym != NULL)
  (top-gdb) n
  812	      CORE_ADDR main_program_name_addr = msym.value_address ();
  (top-gdb) n
  813	      if (main_program_name_addr == 0)
  (top-gdb) n
  816	      main_program_name = target_read_string (main_program_name_addr, 1024);
  (top-gdb) p inferior_ptid
  $2 = {
    m_pid = 0,
    m_lwp = 0,
    m_tid = 0
  }
  (top-gdb) n
  817	      return main_program_name.get ();
  (top-gdb) p main_program_name.get ()
  $3 = 0x3851250 "_ada_foo"
  (top-gdb) 

I suspect the reason this is OK is that the memory read is actually
hitting the objfile, I did try 'set trust-readonly-sections off', but
everything still appears to work, I'm not sure why.

Anyway, where I'm going with all this is: I think we really need to try
and have Inferior.main_name work even when the inferior is not yet
started -- even if in some edge cases we end up returning None (due to a
failed memory read) -- because in the vast majority of cases, we'll want
to know the main name _before_ starting the inferior, and in all the
cases I've seen this appears to work.

We could just switch back to 'scoped_restore_current_inferior' in this
case and trust that any memory reads are always going to hit the objfile
and that inferior_ptid is not needed.

Or we could use 'scoped_restore_current_inferior' or
'scoped_restore_current_inferior_for_memory' depending on whether
inferior::pid is 0 or not.

Also, on a related not, all the methods that now use
scoped_restore_current_inferior_for_memory will cause GDB to crash with
an assert if they are run on an inferior that is not yet started, e.g.:

  (gdb) python i = gdb.selected_inferior ()
  (gdb) python i.read_memory (4,4)
  ../../src.dev-7/gdb/thread.c:626: internal-error: any_thread_of_inferior: Assertion `inf->pid != 0' failed.

I think for things like 'read_memory' it is fine if we just add a check
and raise a Python error if the inferior is not yet active, but as I
argue above, I don't think this is good enough for 'main_name'.

Thanks,
Andrew


>  
>        name = main_name ();
>      }
> diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
> index 8762b6992ca..13beebd08cc 100644
> --- a/gdb/testsuite/gdb.python/py-inferior.exp
> +++ b/gdb/testsuite/gdb.python/py-inferior.exp
> @@ -91,6 +91,7 @@ gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \
>  gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
>      "<memory at $hex>" \
>      "read str contents"
> +gdb_test "python print(astr\[0\])" "b'h'"
>  gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0
>  gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0
>  gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
> @@ -98,6 +99,10 @@ gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
>  gdb_test "print str" " = \"hallo, testsuite\"" \
>    "ensure str was changed in the inferior"
>  
> +# Add a new inferior here, so we can test that operations work on the
> +# correct inferior.
> +set num [add_inferior]
> +
>  # Test memory search.
>  
>  set hex_number {0x[0-9a-fA-F][0-9a-fA-F]*}
> @@ -115,6 +120,10 @@ with_test_prefix "string" {
>      gdb_test_no_output "py start_addr = search_buf.address"
>      gdb_test_no_output "py length = search_buf.type.sizeof"
>  
> +    # Switch to the new inferior before testing.
> +    gdb_test "inferior $num" "Switching to inferior $num.*" \
> +	"switch to inferior $num"
> +
>      gdb_test "py print (gdb.inferiors()\[0\].search_memory (start_addr, length, 'aaa'))" \
>  	"${one_pattern_found}" "find string pattern"
>  
> @@ -128,6 +137,24 @@ with_test_prefix "string" {
>  	"${one_pattern_found}" "pattern found at end of range"
>  }
>  
> +# While still in the new inferior, test reading and writing memory
> +# again.
> +gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
> +    "<memory at $hex>" \
> +    "read str while other inferior selected"
> +gdb_test "python print(astr\[1\])" "b'a'" \
> +    "print a character from the string"
> +gdb_py_test_silent_cmd "python astr\[1\] = b'X'" "change str again" 0
> +gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
> +    "write str while other inferior selected" 1
> +
> +gdb_test "inferior 1" "Switching to inferior 1.*" "switch back to inferior 1"
> +
> +gdb_test "print str" " = \"hXllo, testsuite\"" \
> +    "ensure str was changed while other inferior selected"
> +
> +gdb_test_no_output "remove-inferiors $num" "remove-inferiors $num"
> +
>  # Import struct to pack the following patterns.
>  gdb_test_no_output "py from struct import *"
>  
>
> -- 
> 2.40.1

Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Turns out, 'start' doesn't do anything special.  You call
Andrew> scoped_restore_current_inferior_for_memory in order to setup
Andrew> inferior_ptid, but at the point 'start' calls 'main_name', inferior_ptid
Andrew> is null_ptid, so any memory accesses will fail.

Andrew> I suspect the reason this is OK is that the memory read is actually
Andrew> hitting the objfile, I did try 'set trust-readonly-sections off', but
Andrew> everything still appears to work, I'm not sure why.

I think it's because, before the inferior is started, memory accesses
reach exec_target::xfer_partial, which calls section_table_xfer_memory_partial.

Andrew> Or we could use 'scoped_restore_current_inferior' or
Andrew> 'scoped_restore_current_inferior_for_memory' depending on whether
Andrew> inferior::pid is 0 or not.
...
Andrew> I think for things like 'read_memory' it is fine if we just add a check
Andrew> and raise a Python error if the inferior is not yet active, but as I
Andrew> argue above, I don't think this is good enough for 'main_name'.

I think it's valid to read or search memory before the inferior started.
It's even valid to write memory when 'set write on'.

So, let me know what you think of the appended, which tries to handle
the pid==0 case in all these spots.

Tom

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index f6000b944da..3b5d8378c3a 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -520,6 +520,19 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
   return PyList_AsTuple (list.get ());
 }
 
+/* A helper for scoped_restore_current_inferior_for_memory that
+   handles the situation where the desired inferior has not yet run,
+   so has a PID of 0.  This takes an inferior and returns the ptid of
+   a thread to use when accessing memory.  */
+
+static ptid_t
+ptid_for_memory (inferior *inf)
+{
+  if (inf->pid == 0)
+    return {};
+  return any_thread_of_inferior (inf)->ptid;
+}
+
 /* Membuf and memory manipulation.  */
 
 /* Implementation of Inferior.read_memory (address, length).
@@ -550,7 +563,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
       /* Use this scoped-restore because we want to be able to read
 	 memory from an unwinder.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior, ptid_for_memory (inf->inferior));
 
       buffer.reset ((gdb_byte *) xmalloc (length));
 
@@ -608,7 +621,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 	 still used here, just to keep the code similar to other code
 	 in this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior, ptid_for_memory (inf->inferior));
 
       write_memory_with_notification (addr, buffer, length);
     }
@@ -683,7 +696,7 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 	 still used here, just to keep the code similar to other code
 	 in this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior, ptid_for_memory (inf->inferior));
 
       found = target_search_memory (start_addr, length,
 				    buffer, pattern_size,
@@ -944,7 +957,7 @@ infpy_get_main_name (PyObject *self, void *closure)
 	 still used, just to keep the code similar to other code in
 	 this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior, ptid_for_memory (inf->inferior));
 
       name = main_name ();
     }

Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Pedro Alves]

On 2023-07-17 18:12, Tom Tromey via Gdb-patches wrote:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> 
> Andrew> Turns out, 'start' doesn't do anything special.  You call
> Andrew> scoped_restore_current_inferior_for_memory in order to setup
> Andrew> inferior_ptid, but at the point 'start' calls 'main_name', inferior_ptid
> Andrew> is null_ptid, so any memory accesses will fail.
> 
> Andrew> I suspect the reason this is OK is that the memory read is actually
> Andrew> hitting the objfile, I did try 'set trust-readonly-sections off', but
> Andrew> everything still appears to work, I'm not sure why.
> 
> I think it's because, before the inferior is started, memory accesses
> reach exec_target::xfer_partial, which calls section_table_xfer_memory_partial.
> 
> Andrew> Or we could use 'scoped_restore_current_inferior' or
> Andrew> 'scoped_restore_current_inferior_for_memory' depending on whether
> Andrew> inferior::pid is 0 or not.
> ...
> Andrew> I think for things like 'read_memory' it is fine if we just add a check
> Andrew> and raise a Python error if the inferior is not yet active, but as I
> Andrew> argue above, I don't think this is good enough for 'main_name'.
> 
> I think it's valid to read or search memory before the inferior started.
> It's even valid to write memory when 'set write on'.
> 
> So, let me know what you think of the appended, which tries to handle
> the pid==0 case in all these spots.


I'm thinking that we should just remove the ptid parameter and the
any_thread_of_inferior calls completely, and make
scoped_restore_current_inferior_for_memory switch inferior_ptid to
a pid ptid.

I was thinking of suggesting this before, but stopped short because I was a
little worried that some port might be assuming inferior_ptid points at a
thread in the xfer_partial memory access routines.  We know that anything
that supports forks must not assume that, due to how detach_breakpoints works.
I looked at a number of xfer_partial implementations, and didn't see
anything that is looking at inferior_ptid in a way that would misbehave.  I'm
thinking that we could go forward with this and just fix ports if they break.

While on some ports like on AMD GPU we have things like thread-specific
address spaces, and so when accessing memory for those address spaces,
we must have the right thread context (via inferior_ptid) selected, in
Inferior.read_memory, we only have the inferior, so this means that this API as
is can't be used to access such address spaces.  IOW, it can only be used
to access the global address space that is visible to both the
CPU and the GPUs.  If we end up using scoped_restore_current_inferior_for_memory
later to set up the context to read memory from a specific thread, then
we can add an alternative ctor that takes a thread_info pointer, and make
inferior_ptid point to the thread, for example.

As for pointing inferior_ptid to a pid-ptid, I don't think it is strange,
we do it in other places.  Note that gdbserver handles memory accesses
in a similar "without a thread" way, see:

    commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
    gdbserver: track current process as well as current thread

Note how gdb_read_memory uses set_desired_process, and then switch_to_process
switches current_thread to nullptr.

I ran the testsuite on native and extended-gdbserver (x86_64 GNU/Linux),
and saw no regressions.

From a63bdf461caac057fac40aab5533d4372d8e1a3d Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 17 Jul 2023 18:31:02 +0100
Subject: [PATCH] No ptid

Change-Id: I11309c5ddbbb51a4594cf63c21b3858bfd9aed19
---
 gdb/inferior.h           | 4 ++--
 gdb/proc-service.c       | 3 +--
 gdb/python/py-inferior.c | 8 ++++----
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 98501652a27..94387ff4792 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -771,12 +771,12 @@ class scoped_restore_current_inferior_for_memory
   /* Save the current globals and switch to the given inferior and the
      inferior's program space.  PTID must name a thread in INF, it is
      used as the new inferior_ptid.  */
-  scoped_restore_current_inferior_for_memory (inferior *inf, ptid_t ptid)
+  scoped_restore_current_inferior_for_memory (inferior *inf)
     : m_save_ptid (&inferior_ptid)
   {
     set_current_inferior (inf);
     set_current_program_space (inf->pspace);
-    inferior_ptid = ptid;
+    inferior_ptid = ptid_t (inf->pid);
   }
 
   DISABLE_COPY_AND_ASSIGN (scoped_restore_current_inferior_for_memory);
diff --git a/gdb/proc-service.c b/gdb/proc-service.c
index 366e0510070..f735eb0ac81 100644
--- a/gdb/proc-service.c
+++ b/gdb/proc-service.c
@@ -72,8 +72,7 @@ static ps_err_e
 ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
 		gdb_byte *buf, size_t len, int write)
 {
-  scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf,
-							    ph->thread->ptid);
+  scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf);
 
   CORE_ADDR core_addr = ps_addr_to_core_addr (addr);
 
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index f6000b944da..792f05b118e 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -550,7 +550,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
       /* Use this scoped-restore because we want to be able to read
 	 memory from an unwinder.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       buffer.reset ((gdb_byte *) xmalloc (length));
 
@@ -608,7 +608,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 	 still used here, just to keep the code similar to other code
 	 in this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       write_memory_with_notification (addr, buffer, length);
     }
@@ -683,7 +683,7 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 	 still used here, just to keep the code similar to other code
 	 in this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       found = target_search_memory (start_addr, length,
 				    buffer, pattern_size,
@@ -944,7 +944,7 @@ infpy_get_main_name (PyObject *self, void *closure)
 	 still used, just to keep the code similar to other code in
 	 this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       name = main_name ();
     }

base-commit: 4676d03804285d76a51cb7f98ebe72374321a1f9
-- 
2.34.1

Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> I'm thinking that we should just remove the ptid parameter and the
Pedro> any_thread_of_inferior calls completely, and make
Pedro> scoped_restore_current_inferior_for_memory switch inferior_ptid to
Pedro> a pid ptid.
...

Pedro> While on some ports like on AMD GPU we have things like thread-specific
Pedro> address spaces, and so when accessing memory for those address spaces,
Pedro> we must have the right thread context (via inferior_ptid) selected, in
Pedro> Inferior.read_memory, we only have the inferior, so this means that this API as
Pedro> is can't be used to access such address spaces.

Yeah.  Probably for this case we need to add Thread.read_memory et al,
then make note of this in the gdb.Inferior docs.

Pedro> I ran the testsuite on native and extended-gdbserver (x86_64 GNU/Linux),
Pedro> and saw no regressions.

+1 on this from me.  Thank you.

BTW this is https://sourceware.org/bugzilla/show_bug.cgi?id=30644

Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

[PATCH] Fix gdb.Inferior.read_memory without execution (PR dap/30644) (was: Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al)

[Pedro Alves]

On 2023-07-17 19:44, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> I'm thinking that we should just remove the ptid parameter and the
> Pedro> any_thread_of_inferior calls completely, and make
> Pedro> scoped_restore_current_inferior_for_memory switch inferior_ptid to
> Pedro> a pid ptid.
> ...
> 
> Pedro> While on some ports like on AMD GPU we have things like thread-specific
> Pedro> address spaces, and so when accessing memory for those address spaces,
> Pedro> we must have the right thread context (via inferior_ptid) selected, in
> Pedro> Inferior.read_memory, we only have the inferior, so this means that this API as
> Pedro> is can't be used to access such address spaces.
> 
> Yeah.  Probably for this case we need to add Thread.read_memory et al,
> then make note of this in the gdb.Inferior docs.
> 
> Pedro> I ran the testsuite on native and extended-gdbserver (x86_64 GNU/Linux),
> Pedro> and saw no regressions.
> 
> +1 on this from me.  Thank you.
> 
> BTW this is https://sourceware.org/bugzilla/show_bug.cgi?id=30644
> 
> Reviewed-By: Tom Tromey <tom@tromey.com>

Alright, now as a proper patch, with ctor intro comment tweaked, and with new test
added to gdb.python/py-inferior.exp.  WDYT?

---8<---
From e15ab1d7209b65f344ec20931452861990706742 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 17 Jul 2023 18:31:02 +0100
Subject: [PATCH] Fix gdb.Inferior.read_memory without execution (PR dap/30644)

Andrew reported that the previous change to gdb.Inferior.read_memory &
friends introducing scoped_restore_current_inferior_for_memory broke
gdb.dap/stop-at-main.exp.  This is also reported as PR dap/30644.

The root of the problem is that all the methods that now use
scoped_restore_current_inferior_for_memory cause GDB to crash with a
failed assert if they are run on an inferior that is not yet started.

E.g.:

 (gdb) python i = gdb.selected_inferior ()
 (gdb) python i.read_memory (4,4)
 gdb/thread.c:626: internal-error: any_thread_of_inferior: Assertion `inf->pid != 0' failed.

This patch fixes the problem by removing
scoped_restore_current_inferior_for_memory's ctor ptid parameter and
the any_thread_of_inferior calls completely, and making
scoped_restore_current_inferior_for_memory switch inferior_ptid to a
pid ptid.

I was a little worried that some port might be assuming inferior_ptid
points at a thread in the xfer_partial memory access routines.  We
know that anything that supports forks must not assume that, due to
how detach_breakpoints works.  I looked at a number of xfer_partial
implementations, and didn't see anything that is looking at
inferior_ptid in a way that would misbehave.  I'm thinking that we
could go forward with this and just fix ports if they break.

While on some ports like on AMD GPU we have thread-specific address
spaces, and so when accessing memory for those address spaces, we must
have the right thread context (via inferior_ptid) selected, in
Inferior.read_memory, we only have the inferior to work with, so this
API as is can't be used to access thread-specific address spaces.
IOW, it can only be used to access the global address space that is
visible to both the CPU and the GPUs.

In proc-service.c:ps_xfer_memory, the other spot using
scoped_restore_current_inferior_for_memory, we're always accessing
per-inferior memory.

If we end up using scoped_restore_current_inferior_for_memory later to
set up the context to read memory from a specific thread, then we can
add an alternative ctor that takes a thread_info pointer, and make
inferior_ptid point to the thread, for example.

New test added to gdb.python/py-inferior.exp, exercising
Inferior.read_memory without execution.

No regressions on native and extended-gdbserver x86_64 GNU/Linux.

Reviewed-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30644
Change-Id: I11309c5ddbbb51a4594cf63c21b3858bfd9aed19
---
 gdb/inferior.h                           |  8 ++++----
 gdb/proc-service.c                       |  3 +--
 gdb/python/py-inferior.c                 |  8 ++++----
 gdb/testsuite/gdb.python/py-inferior.c   |  1 +
 gdb/testsuite/gdb.python/py-inferior.exp | 10 ++++++++++
 5 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 98501652a27..df586701612 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -769,14 +769,14 @@ class scoped_restore_current_inferior_for_memory
 public:
 
   /* Save the current globals and switch to the given inferior and the
-     inferior's program space.  PTID must name a thread in INF, it is
-     used as the new inferior_ptid.  */
-  scoped_restore_current_inferior_for_memory (inferior *inf, ptid_t ptid)
+     inferior's program space.  inferior_ptid is set to point to the
+     inferior's process id (and not to any particular thread).  */
+  scoped_restore_current_inferior_for_memory (inferior *inf)
     : m_save_ptid (&inferior_ptid)
   {
     set_current_inferior (inf);
     set_current_program_space (inf->pspace);
-    inferior_ptid = ptid;
+    inferior_ptid = ptid_t (inf->pid);
   }
 
   DISABLE_COPY_AND_ASSIGN (scoped_restore_current_inferior_for_memory);
diff --git a/gdb/proc-service.c b/gdb/proc-service.c
index 366e0510070..f735eb0ac81 100644
--- a/gdb/proc-service.c
+++ b/gdb/proc-service.c
@@ -72,8 +72,7 @@ static ps_err_e
 ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
 		gdb_byte *buf, size_t len, int write)
 {
-  scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf,
-							    ph->thread->ptid);
+  scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf);
 
   CORE_ADDR core_addr = ps_addr_to_core_addr (addr);
 
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index f6000b944da..792f05b118e 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -550,7 +550,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
       /* Use this scoped-restore because we want to be able to read
 	 memory from an unwinder.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       buffer.reset ((gdb_byte *) xmalloc (length));
 
@@ -608,7 +608,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 	 still used here, just to keep the code similar to other code
 	 in this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       write_memory_with_notification (addr, buffer, length);
     }
@@ -683,7 +683,7 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 	 still used here, just to keep the code similar to other code
 	 in this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       found = target_search_memory (start_addr, length,
 				    buffer, pattern_size,
@@ -944,7 +944,7 @@ infpy_get_main_name (PyObject *self, void *closure)
 	 still used, just to keep the code similar to other code in
 	 this file.  */
       scoped_restore_current_inferior_for_memory restore_inferior
-	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
+	(inf->inferior);
 
       name = main_name ();
     }
diff --git a/gdb/testsuite/gdb.python/py-inferior.c b/gdb/testsuite/gdb.python/py-inferior.c
index 3ee9a462060..870f7196444 100644
--- a/gdb/testsuite/gdb.python/py-inferior.c
+++ b/gdb/testsuite/gdb.python/py-inferior.c
@@ -16,6 +16,7 @@ int64_t int64_search_buf[100];
 static char *search_buf;
 static int search_buf_size;
 
+int8_t int8_global = 42;
 
 int f2 (int a)
 {
diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 13beebd08cc..6fbcdd6822f 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -34,6 +34,16 @@ switch [get_endianness] {
     big { set python_pack_char ">" }
 }
 
+# Test memory read operations without execution.
+
+gdb_py_test_silent_cmd "python addr = gdb.lookup_global_symbol ('int8_global').value().address" \
+  "get global variable address" 0
+gdb_test "python \
+	    int8_global_mv = gdb.selected_inferior().read_memory (addr, 1); \
+	    print(int.from_bytes(int8_global_mv\[0\], byteorder='little'))" \
+    "\r\n42" \
+    "read memory without execution"
+
 # The following tests require execution.
 
 if {![runto_main]} {

base-commit: 4676d03804285d76a51cb7f98ebe72374321a1f9
-- 
2.34.1

Re: [PATCH] Fix gdb.Inferior.read_memory without execution (PR dap/30644)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Alright, now as a proper patch, with ctor intro comment tweaked, and with new test
Pedro> added to gdb.python/py-inferior.exp.  WDYT?

Looks good to me, thank you.

Tom

Re: [PATCH] Fix gdb.Inferior.read_memory without execution (PR dap/30644)

[Pedro Alves]

On 2023-07-18 18:25, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> Alright, now as a proper patch, with ctor intro comment tweaked, and with new test
> Pedro> added to gdb.python/py-inferior.exp.  WDYT?
> 
> Looks good to me, thank you.

I pushed this with a tiny change -- since scoped_restore_current_inferior_for_memory's ctor
ends up with one parameter, I made it explicit.

RE: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Aktemur, Tankut Baris]

On Monday, July 17, 2023 8:16 PM, Pedro Alves wrote:
> On 2023-07-17 18:12, Tom Tromey via Gdb-patches wrote:
> >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> >
> > Andrew> Turns out, 'start' doesn't do anything special.  You call
> > Andrew> scoped_restore_current_inferior_for_memory in order to setup
> > Andrew> inferior_ptid, but at the point 'start' calls 'main_name', inferior_ptid
> > Andrew> is null_ptid, so any memory accesses will fail.
> >
> > Andrew> I suspect the reason this is OK is that the memory read is actually
> > Andrew> hitting the objfile, I did try 'set trust-readonly-sections off', but
> > Andrew> everything still appears to work, I'm not sure why.
> >
> > I think it's because, before the inferior is started, memory accesses
> > reach exec_target::xfer_partial, which calls section_table_xfer_memory_partial.
> >
> > Andrew> Or we could use 'scoped_restore_current_inferior' or
> > Andrew> 'scoped_restore_current_inferior_for_memory' depending on whether
> > Andrew> inferior::pid is 0 or not.
> > ...
> > Andrew> I think for things like 'read_memory' it is fine if we just add a check
> > Andrew> and raise a Python error if the inferior is not yet active, but as I
> > Andrew> argue above, I don't think this is good enough for 'main_name'.
> >
> > I think it's valid to read or search memory before the inferior started.
> > It's even valid to write memory when 'set write on'.
> >
> > So, let me know what you think of the appended, which tries to handle
> > the pid==0 case in all these spots.
> 
> 
> I'm thinking that we should just remove the ptid parameter and the
> any_thread_of_inferior calls completely, and make
> scoped_restore_current_inferior_for_memory switch inferior_ptid to
> a pid ptid.
> 
> I was thinking of suggesting this before, but stopped short because I was a
> little worried that some port might be assuming inferior_ptid points at a
> thread in the xfer_partial memory access routines.  We know that anything
> that supports forks must not assume that, due to how detach_breakpoints works.
> I looked at a number of xfer_partial implementations, and didn't see
> anything that is looking at inferior_ptid in a way that would misbehave.  I'm
> thinking that we could go forward with this and just fix ports if they break.
> 
> While on some ports like on AMD GPU we have things like thread-specific
> address spaces, and so when accessing memory for those address spaces,
> we must have the right thread context (via inferior_ptid) selected, in
> Inferior.read_memory, we only have the inferior, so this means that this API as
> is can't be used to access such address spaces.  IOW, it can only be used
> to access the global address space that is visible to both the
> CPU and the GPUs.  If we end up using scoped_restore_current_inferior_for_memory
> later to set up the context to read memory from a specific thread, then
> we can add an alternative ctor that takes a thread_info pointer, and make
> inferior_ptid point to the thread, for example.
> 
> As for pointing inferior_ptid to a pid-ptid, I don't think it is strange,
> we do it in other places.  Note that gdbserver handles memory accesses
> in a similar "without a thread" way, see:
> 
>     commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
>     gdbserver: track current process as well as current thread
> 
> Note how gdb_read_memory uses set_desired_process, and then switch_to_process
> switches current_thread to nullptr.

On a related topic, would you mind commenting on 

  https://sourceware.org/pipermail/gdb-patches/2023-June/200347.html ?

In the downstream debugger of Intel, we see a similar problem.  For a
certain address space, a thread context is needed.  Are you in more favor
of letting the core leave the thread unset and making the targets 
responsible for setting the thread context?

> 
> I ran the testsuite on native and extended-gdbserver (x86_64 GNU/Linux),
> and saw no regressions.
> 
> From a63bdf461caac057fac40aab5533d4372d8e1a3d Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Mon, 17 Jul 2023 18:31:02 +0100
> Subject: [PATCH] No ptid
> 
> Change-Id: I11309c5ddbbb51a4594cf63c21b3858bfd9aed19
> ---
>  gdb/inferior.h           | 4 ++--
>  gdb/proc-service.c       | 3 +--
>  gdb/python/py-inferior.c | 8 ++++----
>  3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 98501652a27..94387ff4792 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -771,12 +771,12 @@ class scoped_restore_current_inferior_for_memory
>    /* Save the current globals and switch to the given inferior and the
>       inferior's program space.  PTID must name a thread in INF, it is
>       used as the new inferior_ptid.  */

Nit: Reference to PTID in the comment should be removed.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Pedro Alves]

On 2023-07-18 09:48, Aktemur, Tankut Baris wrote:
> On Monday, July 17, 2023 8:16 PM, Pedro Alves wrote:

>> As for pointing inferior_ptid to a pid-ptid, I don't think it is strange,
>> we do it in other places.  Note that gdbserver handles memory accesses
>> in a similar "without a thread" way, see:
>>
>>     commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
>>     gdbserver: track current process as well as current thread
>>
>> Note how gdb_read_memory uses set_desired_process, and then switch_to_process
>> switches current_thread to nullptr.
> 
> On a related topic, would you mind commenting on 
> 
>   https://sourceware.org/pipermail/gdb-patches/2023-June/200347.html ?
> 
> In the downstream debugger of Intel, we see a similar problem.  For a
> certain address space, a thread context is needed.  

Ah.

> Are you in more favor
> of letting the core leave the thread unset and making the targets 
> responsible for setting the thread context?

Hmm, I don't see how that would work -- how would the target know which
is the right thread to use?

But let's move the discussion there.  I'll reply to your patch with my
questions.

RE: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al

[Aktemur, Tankut Baris]

On Tuesday, July 18, 2023 2:26 PM, Pedro Alves wrote:
> On 2023-07-18 09:48, Aktemur, Tankut Baris wrote:
> > On Monday, July 17, 2023 8:16 PM, Pedro Alves wrote:
> 
> >> As for pointing inferior_ptid to a pid-ptid, I don't think it is strange,
> >> we do it in other places.  Note that gdbserver handles memory accesses
> >> in a similar "without a thread" way, see:
> >>
> >>     commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
> >>     gdbserver: track current process as well as current thread
> >>
> >> Note how gdb_read_memory uses set_desired_process, and then switch_to_process
> >> switches current_thread to nullptr.
> >
> > On a related topic, would you mind commenting on
> >
> >   https://sourceware.org/pipermail/gdb-patches/2023-June/200347.html ?
> >
> > In the downstream debugger of Intel, we see a similar problem.  For a
> > certain address space, a thread context is needed.
> 
> Ah.
> 
> > Are you in more favor
> > of letting the core leave the thread unset and making the targets
> > responsible for setting the thread context?
> 
> Hmm, I don't see how that would work -- how would the target know which
> is the right thread to use?

GDB sets the general thread via an 'Hg' packet first.  It then sends a
memory read request as an 'm' packet.  The target could do `set_desired_thread`
to set current_thread back to non-null, essentially overwriting the core server's
`set_desired_process`.

> But let's move the discussion there.  I'll reply to your patch with my
> questions.

Ok, sure.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v4 0/6] Fix some Python Inferior methods

[Pedro Alves]

On 2023-07-14 18:06, Tom Tromey wrote:
> A user pointed out a bug in Inferior.search_memory.  This series is
> the result.  Most of the patches are minor things I noticed while
> working on the final patch, which fixes the bug.
> 
> Regression tested on x86-64 Fedora 36.

For whole series:

Approved-By: Pedro Alves <pedro@palves.net>

Thanks.