[v4,1/7] gdb: Allow replayed threads to read and write pseudo registers

  In an effort to support AVX instructions when recording, we need to
allow replaying threads to access pseudo registers. Currently, if
we try to do that gdb will fail in a call to validate_registers_access,
because the thread is executing so GDB thinks it is unsafe to read
pseudo registers.

When replaying, the thread is really executing for all intents and
purposes, but the execution is just having GDB change values on
registers, so it will always be safe to read and write pseudo registers.
This commit changes validate_registers_access to allow access when we
are replaying. The check to whether we are replaying must not happen
when writing a core file, as record_full_list could be nullptr, so we
only check it if the thread is executing.

As of this commit, I don't know of a way to trigger this commit without
AVX support on record, so a test isn't provided. However, as soon as
record-full supports saving ymm registers, the AVX tests will test this
as well.
---
 gdb/record-full.c |  8 ++++++++
 gdb/record-full.h |  3 +++
 gdb/thread.c      | 15 +++++++++++++--
 3 files changed, 24 insertions(+), 2 deletions(-)
  

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

Guinevere> +/* see record-full.h.  */
Guinevere> +bool
Guinevere> +record_full_is_replaying ()
Guinevere> +{
Guinevere> +  return (execution_direction == EXEC_REVERSE
Guinevere> +	  || record_full_list->next != nullptr);

I was inclined to approve this, but I was curious and I found the
existing RECORD_FULL_IS_REPLAY macro, and then this led me to
target_record_is_replaying -- the same thing, I think, but in a
future-proof way in case record-full is ever multi-target-capable.

So I wonder if the new function can be dispensed with and thread.c
changed to use target_record_is_replaying?

I guess that would affect non-record-full targets?  But ... would it
matter?  I can't really tell.

Tom
  

On 10/4/24 4:54 PM, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
> Guinevere> +/* see record-full.h.  */
> Guinevere> +bool
> Guinevere> +record_full_is_replaying ()
> Guinevere> +{
> Guinevere> +  return (execution_direction == EXEC_REVERSE
> Guinevere> +	  || record_full_list->next != nullptr);
>
> I was inclined to approve this, but I was curious and I found the
> existing RECORD_FULL_IS_REPLAY macro, and then this led me to
> target_record_is_replaying -- the same thing, I think, but in a
> future-proof way in case record-full is ever multi-target-capable.
>
> So I wonder if the new function can be dispensed with and thread.c
> changed to use target_record_is_replaying?
>
> I guess that would affect non-record-full targets?  But ... would it
> matter?  I can't really tell.

I completely overlooked that! Thanks for finding it.

I don't know if it is guaranteed that we can access registers in other 
recording methods. Maybe btrace is able to, but I would expect RR to be 
unable, and if QEMU ever implements reversing in their stub, it would 
probably not be the case either. And even with btrace, the function 
btrace_fetch asserts on can_access_registers_thread, and I worry that we 
could muddle that assert (or even make it useless) if btrace executing 
could false negative the assert, if that makes sense.

I could have record_full_is_replaying in a more generic fashion by using 
the macro (and possibly checking if target.at(record_stratum) == 
record-full, and so things are ready for when record-full is able to 
handle multi-inferior cases), if you'd prefer it.

>
> Tom
>
  

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

Guinevere> I could have record_full_is_replaying in a more generic fashion by
Guinevere> using the macro (and possibly checking if target.at(record_stratum) ==
Guinevere> record-full, and so things are ready for when record-full is able to
Guinevere> handle multi-inferior cases), if you'd prefer it.

Yeah, I think that would be an improvement.

thanks,
Tom
  

On 10/7/24 2:05 PM, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
> Guinevere> I could have record_full_is_replaying in a more generic fashion by
> Guinevere> using the macro (and possibly checking if target.at(record_stratum) ==
> Guinevere> record-full, and so things are ready for when record-full is able to
> Guinevere> handle multi-inferior cases), if you'd prefer it.
>
> Yeah, I think that would be an improvement.

This is the diff I came up with. Is the patch OK with this change?

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 688a9c84a06..4ed381f8e27 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -374,8 +374,9 @@ record_full_is_used (void)
  bool
  record_full_is_replaying ()
  {
-  return (execution_direction == EXEC_REVERSE
-         || record_full_list->next != nullptr);
+  return streq (current_inferior ()->target_at 
(record_stratum)->shortname (),
+               "record-full")
+        && RECORD_FULL_IS_REPLAY;
  }
  

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

Guinevere> +  return streq (current_inferior ()->target_at 
Guinevere> (record_stratum)->shortname (),
Guinevere> +               "record-full")

I know there are a couple of instance of comparing the target shortname
in the tree already, but I think it's a bad practice.
A dynamic_cast checking for the type would be better IMO.

Tom