[v2,5/6] Introduce scoped_restore_current_inferior_for_memory
Commit Message
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
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
>>>>> "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
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
@@ -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);
}
@@ -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. */
@@ -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);