[PINGv2][PATCH v3] gdb: Introduce RAII signal handler setter

[Guinevere Larsen]

Ping :)

On 9/16/24 10:30 AM, Guinevere Larsen wrote:
> Ping :)
>
> On 8/27/24 7:52 AM, Guinevere Larsen wrote:
>> 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 3 places where this class can just be dropped in,
>> gdb/record-full.c, gdb/utils.c and gdb/extension.c. This third place
>> already had a specialized RAII signal handler setter, but it is
>> substituted for the new general purpose one.
>> ---
>>   gdb/extension.c                    | 21 ++----------
>>   gdb/record-full.c                  |  5 ++-
>>   gdb/utils.c                        |  7 ++--
>>   gdbsupport/scoped_signal_handler.h | 52 ++++++++++++++++++++++++++++++
>>   4 files changed, 58 insertions(+), 27 deletions(-)
>>   create mode 100644 gdbsupport/scoped_signal_handler.h
>>
>> diff --git a/gdb/extension.c b/gdb/extension.c
>> index c488fc77494..b1396928456 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)
>>   }
>>   

>>   -/* 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 ab854e0133f..91667d425f3 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"
>> @@ -1150,6 +1151,7 @@ record_full_wait_1 (struct target_ops *ops,
>>   {
>>     scoped_restore restore_operation_disable
>>       = record_full_gdb_operation_disable_set ();
>> +  scoped_signal_handler interrupt_handler(SIGINT, 
>> record_full_sig_handler);
>>       if (record_debug)
>>       gdb_printf (gdb_stdlog,
>> @@ -1170,7 +1172,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;
>>   @@ -1459,8 +1460,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 94310300fb5..4cff0b2b141 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 alarm_restore(SIGALRM, sigalrm_handler);
>>   #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..4b5c8bb67e7
>> --- /dev/null
>> +++ b/gdbsupport/scoped_signal_handler.h
>> @@ -0,0 +1,52 @@
>> +/* 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>
>> +
>> +class scoped_signal_handler
>> +{
>> +public:
>> +  scoped_signal_handler (int sig, sighandler_t handler)
>> +    : m_sig(sig)
>> +  {
>> +    /* 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 safe 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 (m_sig, m_prev_handler);
>> +  }
>> +
>> +  DISABLE_COPY_AND_ASSIGN (scoped_signal_handler);
>> +private:
>> +  sighandler_t m_prev_handler;
>> +  int m_sig;
>> +};
>> +
>> +#endif /* SCOPED_SIGNAL_HANDLER_H  */
>
>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers