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

  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(-)
  

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
  

>>>>> "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 ();
     }
  

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
  

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

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
  

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.
  

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