[PATCH 0/4] Fixes to record-full replay issues

[PATCH 0/4] Fixes to record-full replay issues

[Guinevere Larsen]

While trying to properly support recording AVX instructions, by
recording the full YMM registers instead of just XMM registers, I found
several issues in record_full_wait_1 and its interactions with other
areas of GDB.

The first patch is just a minor refactor/cosmetic change.

The second fixes the issue that if there ever was an exception while
record_full_wait_1 was recording or replaying, we would leave the wrong
signal handler in place. Fixed by introducing an RAII class for signal
handlers. I went ahead and used it in the 2 other places where it was an
obvious replacement.

Third patch fixes an issue where, when replaying a thread, if an
exception happened, the inferior would be left in an "instruction half
executed state", for instance changing the PC but not a memory region.

The fourth patch is half of a fix to read and change pseudo-registers
while replaying a thread. The other half of the fix was sent separately as
https://inbox.sourceware.org/gdb-patches/20240621180730.859487-1-blarsen@redhat.com/T/#u,
since that fix feels more general than something specific to
record-full.

Guinevere Larsen (4):
  gdb, record: Introduce record_full_following_entry function
  gdb: Introduce RAII signal handler setter
  gdb, record: Fix exception handling in record_full_wait_1
  gdb: Stop considering replaying a thread as executing it

 gdb/extension.c                    | 21 +---------
 gdb/record-full.c                  | 63 ++++++++++++++++++++----------
 gdb/record-full.h                  |  3 ++
 gdb/thread.c                       |  8 ++++
 gdb/utils.c                        |  7 +---
 gdbsupport/scoped_signal_handler.h | 49 +++++++++++++++++++++++
 6 files changed, 106 insertions(+), 45 deletions(-)
 create mode 100644 gdbsupport/scoped_signal_handler.h

-- 
2.45.2

[PATCH 1/4] gdb, record: Introduce record_full_following_entry function

[Guinevere Larsen]

While replaying the inferior, there are a couple repetitions of a code
pattern:
  1. Check the direction of execution of the inferior
  2. Check if there is another entry that direction
  3. Advance the current location to the next entry

The following commit would add one more use of this pattern, and it felt
like a good idea to separate this into it's own small function, so this
commit does that.  I also changed the checks into asserts, since not
having a following entry would lead to an infinite loop in waiting for
the inferior, so this failure should be easier to spot.

This patch shouldn't have any user-visible changes.
---
 gdb/record-full.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index eb62d186fa5..1de3ae0a1c8 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -462,6 +462,21 @@ record_full_end_release (struct record_full_entry *rec)
   xfree (rec);
 }
 
+/* Convenience function to find the next entry in the history, based on
+   the current execution direction.  This should be called only when we
+   are sure there is a following entry.  */
+static struct record_full_entry *
+record_full_following_entry (struct record_full_entry *curr)
+{
+  if (execution_direction == EXEC_REVERSE)
+    {
+      gdb_assert (curr->prev);
+      return curr->prev;
+    }
+  gdb_assert (curr->next);
+  return curr->next;
+}
+
 /* Free one record entry, any type.
    Return entry->type, in case caller wants to know.  */
 
@@ -1418,18 +1433,7 @@ record_full_wait_1 (struct target_ops *ops,
 		}
 
 	      if (continue_flag)
-		{
-		  if (execution_direction == EXEC_REVERSE)
-		    {
-		      if (record_full_list->prev)
-			record_full_list = record_full_list->prev;
-		    }
-		  else
-		    {
-		      if (record_full_list->next)
-			record_full_list = record_full_list->next;
-		    }
-		}
+		record_full_list = record_full_following_entry (record_full_list);
 	    }
 	  while (continue_flag);
 
@@ -1447,13 +1451,7 @@ record_full_wait_1 (struct target_ops *ops,
 	}
       catch (const gdb_exception &ex)
 	{
-	  if (execution_direction == EXEC_REVERSE)
-	    {
-	      if (record_full_list->next)
-		record_full_list = record_full_list->next;
-	    }
-	  else
-	    record_full_list = record_full_list->prev;
+	  record_full_list = record_full_following_entry (record_full_list);
 
 	  throw;
 	}
-- 
2.45.2

[PATCH 2/4] gdb: Introduce RAII signal handler setter

[Guinevere Larsen]

This patch is motivated by the wait function for the record-full target,
that would install a custom signal handler for SIGINT, but could throw
an exception and never reset the SIGINT handler.  This is clearly a bad
idea, so this patch introduces the class scoped_signal_handler in a new
.h file.  The file is added to gdbsupport, even though only gdb code is
using it, because it feels like an addition that would be useful for
more than just directly gdb.

There are 2 places where this class can just be dropped in, one in
record-full.c and the other in utils.c. The third place where this class
can be used is in extension.c.  It already has an RAII class, but the
new one is just a generalization, so the specialized one is removed.
---
 gdb/extension.c                    | 21 ++-----------
 gdb/record-full.c                  |  5 ++-
 gdb/utils.c                        |  7 ++---
 gdbsupport/scoped_signal_handler.h | 49 ++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 27 deletions(-)
 create mode 100644 gdbsupport/scoped_signal_handler.h

diff --git a/gdb/extension.c b/gdb/extension.c
index 99e7190d80b..529d7a40bfc 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -33,6 +33,7 @@
 #include "guile/guile.h"
 #include <array>
 #include "inferior.h"
+#include "gdbsupport/scoped_signal_handler.h"
 
 static script_sourcer_func source_gdb_script;
 static objfile_script_sourcer_func source_gdb_objfile_script;
@@ -297,27 +298,9 @@ ext_lang_auto_load_enabled (const struct extension_language_defn *extlang)
 }
 \f
 
-/* RAII class used to temporarily return SIG to its default handler.  */
-
-template<int SIG>
-struct scoped_default_signal
-{
-  scoped_default_signal ()
-  { m_old_sig_handler = signal (SIG, SIG_DFL); }
-
-  ~scoped_default_signal ()
-  { signal (SIG, m_old_sig_handler); }
-
-  DISABLE_COPY_AND_ASSIGN (scoped_default_signal);
-
-private:
-  /* The previous signal handler that needs to be restored.  */
-  sighandler_t m_old_sig_handler;
-};
-
 /* Class to temporarily return SIGINT to its default handler.  */
 
-using scoped_default_sigint = scoped_default_signal<SIGINT>;
+using scoped_default_sigint = scoped_signal_handler<SIGINT, SIG_DFL>;
 
 /* Functions that iterate over all extension languages.
    These only iterate over external extension languages, not including
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 1de3ae0a1c8..0851e3c0d21 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -39,6 +39,7 @@
 #include "infrun.h"
 #include "gdbsupport/gdb_unlinker.h"
 #include "gdbsupport/byte-vector.h"
+#include "gdbsupport/scoped_signal_handler.h"
 #include "async-event.h"
 #include "top.h"
 #include "valprint.h"
@@ -1165,6 +1166,7 @@ record_full_wait_1 (struct target_ops *ops,
 {
   scoped_restore restore_operation_disable
     = record_full_gdb_operation_disable_set ();
+  scoped_signal_handler<SIGINT, record_full_sig_handler> interrupt_handler;
 
   if (record_debug)
     gdb_printf (gdb_stdlog,
@@ -1185,7 +1187,6 @@ record_full_wait_1 (struct target_ops *ops,
     }
 
   record_full_get_sig = 0;
-  signal (SIGINT, record_full_sig_handler);
 
   record_full_stop_reason = TARGET_STOPPED_BY_NO_REASON;
 
@@ -1457,8 +1458,6 @@ record_full_wait_1 (struct target_ops *ops,
 	}
     }
 
-  signal (SIGINT, handle_sigint);
-
   return inferior_ptid;
 }
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 17498e04312..c49e28c5af8 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -19,6 +19,7 @@
 
 #include <ctype.h>
 #include "gdbsupport/gdb_wait.h"
+#include "gdbsupport/scoped_signal_handler.h"
 #include "event-top.h"
 #include "gdbthread.h"
 #include "fnmatch.h"
@@ -3424,9 +3425,7 @@ wait_to_die_with_timeout (pid_t pid, int *status, int timeout)
       sa.sa_flags = 0;
       sigaction (SIGALRM, &sa, &old_sa);
 #else
-      sighandler_t ofunc;
-
-      ofunc = signal (SIGALRM, sigalrm_handler);
+      scoped_signal_handler<SIGALRM, sigalrm_handler> alarm_restore;
 #endif
 
       alarm (timeout);
@@ -3438,8 +3437,6 @@ wait_to_die_with_timeout (pid_t pid, int *status, int timeout)
       alarm (0);
 #if defined (HAVE_SIGACTION) && defined (SA_RESTART)
       sigaction (SIGALRM, &old_sa, NULL);
-#else
-      signal (SIGALRM, ofunc);
 #endif
 #endif
     }
diff --git a/gdbsupport/scoped_signal_handler.h b/gdbsupport/scoped_signal_handler.h
new file mode 100644
index 00000000000..7da3b34f367
--- /dev/null
+++ b/gdbsupport/scoped_signal_handler.h
@@ -0,0 +1,49 @@
+/* RAII class to install a separate handler for a given signal
+
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef SCOPED_SIGNAL_HANDLER_H
+#define SCOPED_SIGNAL_HANDLER_H
+
+#include <signal.h>
+
+template<int SIG, sighandler_t HANDLER>
+class scoped_signal_handler {
+  public:
+    scoped_signal_handler ()
+    {
+      /* The return of the function call is the previous signal handler, or
+	 SIG_ERR if the function doesn't succeed.  */
+      m_prev_handler = signal (SIG, HANDLER);
+      /* According to the GNU libc manual, the only way signal fails is if
+	 the signum given is invalid, so we should be save to assert.  */
+      gdb_assert (m_prev_handler != SIG_ERR);
+    }
+    ~scoped_signal_handler ()
+    {
+      /* No need to save previous handler, and per the comment on the previous
+	 assert, there should be no way this call fails here.  */
+      signal (SIG, m_prev_handler);
+    }
+
+    DISABLE_COPY_AND_ASSIGN (scoped_signal_handler);
+  private:
+    sighandler_t m_prev_handler;
+};
+
+#endif /* SCOPED_SIGNAL_HANDLER_H  */
-- 
2.45.2

[PATCH 3/4] gdb, record: Fix exception handling in record_full_wait_1

[Guinevere Larsen]

When the record target is replaying the history, there is a try/catch
block there to handle exceptions.  While I can't see a way to trigger an
exception with the current GDB state, when attempting to support YMM
registers for AVX instructions, I hit it and found a nasty infinite
loop.

There are a couple remaining problems with that exception handling:
* We just move the current history one over to the last replayed entry,
  which can leave the inferior in an "instruction half-executed" way.
* We don't set the inferior thread to have requested a stop, so when the
  exception is done bubbling up, GDB is back to attempting to resume the
  inferior, which leads to the infinite loop mentioned above.

This commit fixes the first point by undoing the replay of the last
instruction, so we are stopped on a reasonable state, and the second by
setting the thread to be stopped by a spurious signal and requesting a
stop, so the loop doesn't get going.
---
 gdb/record-full.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 0851e3c0d21..f81cf79903e 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1454,7 +1454,23 @@ record_full_wait_1 (struct target_ops *ops,
 	{
 	  record_full_list = record_full_following_entry (record_full_list);
 
-	  throw;
+	  /* Undo everything that the previous step did, so we don't end up
+	     in between the execution of an instruction.  */
+	  while (record_full_list->type != record_full_end)
+	    {
+	      record_full_exec_insn (regcache, gdbarch, record_full_list);
+
+	      record_full_list = record_full_following_entry (record_full_list);
+	    }
+
+	  /* Make sure we don't try to execute the inferior again.  If this
+	     error happens every time, we'll get an infinite loop if we don't
+	     stop the inferior now.  */
+	  status->set_spurious ();
+	  gdb_printf(gdb_stderr, "Replay execution failed: ");
+	  exception_print (gdb_stderr, ex);
+	  record_full_resume_step = 1;
+	  inferior_thread ()->stop_requested = 1;
 	}
     }
 
-- 
2.45.2

[PATCH 4/4] gdb: Stop considering replaying a thread as executing it

[Guinevere Larsen]

In the previous commit, I mentioned an exception triggering while
replaying a thread. What happened was that GDB tried to read the
pseudo-register ymm0 while replaying, and when passing the read call
down to the gdbarch, we fail a validate_registers_access check, since
that part of GDB thinks the thread is executing uncontrollably and
therefore reading registers is a bad idea.

While that validation makes sense in most cases, GDB's replaying is not
execution in that sense, it is just the record subsystem changing
register and memory values, so it is always safe to read registers.  In
order to not litter the code with checks about whether GDB is replaying,
this patch just makes it so calls to thread::set_executing will always
set it to not executing if the inferior is being replayed.

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      | 8 ++++++++
 3 files changed, 19 insertions(+)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index f81cf79903e..c0212abd7c8 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -371,6 +371,14 @@ record_full_is_used (void)
 	  || t == &record_full_core_ops);
 }
 
+/* see record-full.h.  */
+bool
+record_full_is_replaying ()
+{
+  return (execution_direction == EXEC_REVERSE
+	  || record_full_list->next != nullptr);
+}
+
 
 /* Command lists for "set/show record full".  */
 static struct cmd_list_element *set_record_full_cmdlist;
diff --git a/gdb/record-full.h b/gdb/record-full.h
index e8eb041311b..07f97d241a2 100644
--- a/gdb/record-full.h
+++ b/gdb/record-full.h
@@ -31,6 +31,9 @@ extern int record_full_arch_list_add_end (void);
 /* Returns true if the process record target is open.  */
 extern int record_full_is_used (void);
 
+/* Whether the inferior is being replayed, or is executing normally.  */
+extern bool record_full_is_replaying ();
+
 extern scoped_restore_tmpl<int> record_full_gdb_operation_disable_set ();
 
 #endif /* RECORD_FULL_H */
diff --git a/gdb/thread.c b/gdb/thread.c
index 4ee46936861..53aaa825941 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -50,6 +50,7 @@
 #include "inline-frame.h"
 #include "stack.h"
 #include "interps.h"
+#include "record-full.h"
 
 /* See gdbthread.h.  */
 
@@ -374,6 +375,13 @@ thread_info::deletable () const
 void
 thread_info::set_executing (bool executing)
 {
+  /* If we are replaying with the record-full subsystem, even though
+     the thread seems to be executing for the user, it is stopped for
+     GDB, and we can fiddle with it's registers as we'd like.  So to
+     avoid getting "can't read register" issues, let's say the thread
+     isn't executing.  */
+  if (record_full_is_replaying ())
+    executing = false;
   m_executing = executing;
   if (executing)
     this->clear_stop_pc ();
-- 
2.45.2