diff mbox series

[v2,5/6] Introduce scoped_restore_current_inferior_for_memory

Message ID 20230711-py-inf-fixes-30615-v2-5-64a2540864e6@adacore.com
State New
Headers
Series Fix some Python Inferior methods |

Commit Message

Tom Tromey July 11, 2023, 4:14 p.m. UTC 
  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(-)

Comments

Simon Marchi July 12, 2023, 8:35 p.m. UTC | #1 
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
Tom Tromey July 13, 2023, 2:01 p.m. UTC | #2 
>>>>> "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
Pedro Alves July 14, 2023, 4:33 p.m. UTC | #3 
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
diff mbox series

Patch

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