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

[PATCH v2 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 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                 | 28 ++++++++---
 gdb/testsuite/gdb.python/py-inferior.exp | 83 ++++++++++++++++++++++++--------
 6 files changed, 120 insertions(+), 50 deletions(-)
---
base-commit: 606d863236197cc2fbf74edf589cbaf35ea15801
change-id: 20230707-py-inf-fixes-30615-668ef09475ab

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

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

Re: [PATCH v2 3/6] Rename Python variable in py-inferior.exp

[Pedro Alves]

On 2023-07-11 17:14, Tom Tromey via Gdb-patches wrote:
> 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.

Thanks.

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

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

Re: [PATCH v2 5/6] Introduce scoped_restore_current_inferior_for_memory

[Simon Marchi]

On 7/11/23 12:14, Tom Tromey via Gdb-patches wrote:
> 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.

OOC, why is it important to avoid invalidating the frame cache?  If
there's a good reason, it should probably be mentioned here or in a
comment.

Simon

Re: [PATCH v2 5/6] Introduce scoped_restore_current_inferior_for_memory

[Tom Tromey]

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

Simon> On 7/11/23 12:14, Tom Tromey via Gdb-patches wrote:
>> 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.

Simon> OOC, why is it important to avoid invalidating the frame cache?  If
Simon> there's a good reason, it should probably be mentioned here or in a
Simon> comment.

It depends on the caller, but in the case of the Python APIs, it is so
they can be used from an unwinder.  I'll add some comments in there.

Tom

Re: [PATCH v2 5/6] Introduce scoped_restore_current_inferior_for_memory

[Pedro Alves]

Hi!

On 2023-07-11 17:14, Tom Tromey via Gdb-patches wrote:
> 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.

Answering a question from IRC:


  > <tromey> palves: you mentioned using a new scope-restore in detach_breakpoints as well, but that code isn't
  > setting the current inferior / program space, so I'm wondering if it's really appropriate there

The reason I mentioned that spot as well was because it is another spot where we
temporarily point inferior_ptid to a ptid that is not the current thread, so that we can poke
at the memory of ptid.  In that case, it's to detach breakpoints from a fork child.  Using the
scoped_restore thing there (even if the inferior/pspace switching would effectively be no-ops) too
would be a sort of auto-documentation, and help us grep for these special cases.  But yeah, it
doesn't really need it.  So I'm OK without doing that if you prefer.

So:

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

Thanks for doing this.

Pedro Alves

[PATCH v2 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                 | 28 ++++++++++++++++++++++------
 gdb/testsuite/gdb.python/py-inferior.exp | 26 ++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index af8bd8855a3..46b9a32c87d 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,9 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
 
   try
     {
+      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 +572,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 +581,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 +601,9 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 
   try
     {
+      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 +617,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 +627,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 +638,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 +672,9 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 
   try
     {
+      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);
@@ -911,11 +930,8 @@ infpy_get_main_name (PyObject *self, void *closure)
     {
       /* 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);
+      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..eb501838117 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\])" .*
 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,9 @@ 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" ".*" "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 +136,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" ".*" "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 v2 6/6] Use correct inferior in Inferior.read_memory et al

[Pedro Alves]

On 2023-07-11 17:14, Tom Tromey via Gdb-patches wrote:
> 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.
> 

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

Thanks,
Pedro Alves

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615
> ---
>  gdb/python/py-inferior.c                 | 28 ++++++++++++++++++++++------
>  gdb/testsuite/gdb.python/py-inferior.exp | 26 ++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index af8bd8855a3..46b9a32c87d 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,9 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      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 +572,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 +581,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 +601,9 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      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 +617,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 +627,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 +638,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 +672,9 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      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);
> @@ -911,11 +930,8 @@ infpy_get_main_name (PyObject *self, void *closure)
>      {
>        /* 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);
> +      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..eb501838117 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\])" .*
>  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,9 @@ 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" ".*" "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 +136,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" ".*" "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 *"
>  
> 

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

[Pedro Alves]

On 2023-07-14 17:48, Pedro Alves wrote:
> On 2023-07-11 17:14, Tom Tromey via Gdb-patches wrote:
>> 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.
>>
> 
> Approved-By: Pedro Alves <pedro@palves.net>
> 

I wrote a bunch of text that I ended up deleting ( don't ask :-) ) and then forgot
that I also had some comments on the testcase...  :-P  I'll reply to v3, which I only
noticed now was out already.