[v3] gdb: Introduce RAII signal handler setter

  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
  

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  */
  

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  */
>
>
  

Guinevere Larsen <blarsen@redhat.com> writes:

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

Does this compile for you?  I see this:

    CXX    extension.o
  ../../src/gdb/extension.c:303:52: error: expected ‘;’ before ‘(’ token
    303 | using scoped_default_sigint = scoped_signal_handler(SIGINT, SIG_DFL);
        |                                                    ^
        |                                                    ;
  ../../src/gdb/extension.c: In function ‘void ext_lang_initialization()’:
  ../../src/gdb/extension.c:320:4: error: ‘scoped_default_sigint’ was not declared in this scope
    320 |    scoped_default_sigint set_sigint_to_default_handler;
        |    ^~~~~~~~~~~~~~~~~~~~~
  make: *** [Makefile:1963: extension.o] Error 1

which makes sense to me as I thought `using` in this context created an
alias for an existing type-id, and what you have on the right is not a
type.

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

Missing space before '(' here.

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

Not a fan of this change as the sigaction path is now handled
differently from the signal path.  Sure the signal path is probably
better, but I think they should both follow the same pattern.

Of course, this might mean that changing this doesn't fit within this
patch, which I think would be fine.

>  #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
> +{

The class in gdb/extension.c that you removed had the signal number as a
template parameter.  As does the class in scoped_ignore_signal.h.  I
guess my hope would be that these are small classes that would mostly
end up inline, so specialising on the signal number shouldn't result in
significantly more code, and might actually result in less (as the
signal number doesn't need to be preserved in m_sig...  though maybe
that would be optimised out anyway....

Anyway, I guess I don't really know if there's a "right" answer, but it
might be worth mentioning why this choice compared to the similar code
that took the other choice.

Thanks,
Andrew


> +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  */
> -- 
> 2.46.0
  

On 9/26/24 2:24 PM, Andrew Burgess wrote:
> Guinevere Larsen <blarsen@redhat.com> writes:
>
>> 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);
> Does this compile for you?  I see this:
>
>      CXX    extension.o
>    ../../src/gdb/extension.c:303:52: error: expected ‘;’ before ‘(’ token
>      303 | using scoped_default_sigint = scoped_signal_handler(SIGINT, SIG_DFL);
>          |                                                    ^
>          |                                                    ;
>    ../../src/gdb/extension.c: In function ‘void ext_lang_initialization()’:
>    ../../src/gdb/extension.c:320:4: error: ‘scoped_default_sigint’ was not declared in this scope
>      320 |    scoped_default_sigint set_sigint_to_default_handler;
>          |    ^~~~~~~~~~~~~~~~~~~~~
>    make: *** [Makefile:1963: extension.o] Error 1
>
> which makes sense to me as I thought `using` in this context created an
> alias for an existing type-id, and what you have on the right is not a
> type.
You're right, it didn't compile. Moving from v2 to v3 I removed 
templating from the class, and I think I just changes things 
mechanically and forgot to re-test, sorry.
>
>>   
>>   /* 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);
> Missing space before '(' here.
oops, fixed.
>
>>   
>>     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);
> Not a fan of this change as the sigaction path is now handled
> differently from the signal path.  Sure the signal path is probably
> better, but I think they should both follow the same pattern.
>
> Of course, this might mean that changing this doesn't fit within this
> patch, which I think would be fine.
I'm not a fan of the sigaction path, honestly. From my google given 
feedback of version 2, it seems like sigaction is usually better but our 
code doesn't make use of anything that makes it better and just uses it 
as if it was a "signal" call. Personally, I'd put everything to use the 
RAII handler, but I am not experienced with this type of code, so if you 
tell me that we should keep the sigaction path, I'll just revert the 
change on this file.
>
>>   #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
>> +{
> The class in gdb/extension.c that you removed had the signal number as a
> template parameter.  As does the class in scoped_ignore_signal.h.  I
> guess my hope would be that these are small classes that would mostly
> end up inline, so specialising on the signal number shouldn't result in
> significantly more code, and might actually result in less (as the
> signal number doesn't need to be preserved in m_sig...  though maybe
> that would be optimised out anyway....
>
> Anyway, I guess I don't really know if there's a "right" answer, but it
> might be worth mentioning why this choice compared to the similar code
> that took the other choice.

I had both options as templates in v2 and Tromey mentioned he doesn't 
like the function pointer being a handler, and doesn't thing the number 
was necessary either, that's why I went with no templates this time. So 
the reason was "reviewer liked it better that way", I guess. Would you 
like me to add that in a more polite way in the commit message or similar?

>
> Thanks,
> Andrew
>
>
>> +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  */
>> -- 
>> 2.46.0