[PATCH] [gdb] Handle ^C during disassembly

[Tom de Vries]

On 8/8/24 23:58, Tom de Vries wrote:
> On 8/8/24 19:21, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> In PR gdb/32025, a fatal error was reported when sending a SIGINT to 
>>> gdb while
>>> disassembling.
>>>
>>> I managed to reproduce this on aarch64-linux in a Leap 15.5 container 
>>> using
>>> this trigger patch:
>>> ...
>>>   gdb_disassembler_memory_reader::dis_asm_read_memory
>>>     (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
>>>      struct disassemble_info *info) noexcept
>>>   {
>>> +  set_quit_flag ();
>>>     return target_read_code (memaddr, myaddr, len);
>>>   }
>>> ...
>>> and a simple gdb command line calling the disassemble command:
>>> ...
>>> $ gdb -q -batch a.out -ex "disassemble main"
>>> ...
>>>
>>> The following scenario leads to the fatal error:
>>> - the disassemble command is executed,
>>> - set_quit_flag is called in
>>>    gdb_disassembler_memory_reader::dis_asm_read_memory, pretending 
>>> that a
>>>    user pressed ^C,
>>> - target_read_code calls QUIT, which throws a
>>>    gdb_exception_quit,
>>> - the exception propagation mechanism reaches c code in libopcodes 
>>> and a fatal
>>>    error triggers because the c code is not compiled with -fexception.
>>>

I also managed to reproduce this on x86_64-linux, which proves that the 
bit about -fexception is incorrect.

The error triggers because 
gdb_disassembler_memory_reader::dis_asm_read_memory has noexcept.

Knowing now that this reproduces on more than one architecture, I'd like 
to backport this patch to gdb-15-branch.

WDYT?

Thanks
- Tom


>>> Fix this by:
>>> - wrapping the body of 
>>> gdb_disassembler_memory_reader::dis_asm_read_memory in
>>>    catch_exceptions (which consequently needs moving to a header 
>>> file), and
>>> - reraising the caught exception in default_print_insn using QUIT.
>>>
>>> Tested on aarch64-linux.
>>
>> Thanks for fixing this.
>>
>> I haven't looked at the possible test yet.  I think having tests would
>> be nice, but also those could potentially come later.  Anyway, I'll
>> leave this:
>>
>> Approved-By: Andrew Burgess <aburgess@redhat.com>
>>
>> You can merge now if you want, or work on the tests first.
>>
> 
> Hi Andrew,
> 
> thanks for the reviews, I've chosen to merge both patches.
> 
> Thanks,
> - Tom
> 
>>
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32025
>>> ---
>>>   gdb/arch-utils.c |  7 ++++++-
>>>   gdb/disasm.c     |  7 ++++++-
>>>   gdb/event-top.h  | 25 +++++++++++++++++++++++++
>>>   gdb/gdb_bfd.c    | 23 -----------------------
>>>   4 files changed, 37 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
>>> index fb5634d5fa0..785940da70c 100644
>>> --- a/gdb/arch-utils.c
>>> +++ b/gdb/arch-utils.c
>>> @@ -37,6 +37,7 @@
>>>   #include "auxv.h"
>>>   #include "observable.h"
>>>   #include "solib-target.h"
>>> +#include "event-top.h"
>>>   #include "gdbsupport/version.h"
>>> @@ -1040,7 +1041,11 @@ default_print_insn (bfd_vma memaddr, 
>>> disassemble_info *info)
>>>                    info->mach, current_program_space->exec_bfd ());
>>>     gdb_assert (disassemble_fn != NULL);
>>> -  return (*disassemble_fn) (memaddr, info);
>>> +  int res = (*disassemble_fn) (memaddr, info);
>>> +
>>> +  QUIT;
>>> +
>>> +  return res;
>>>   }
>>>   /* See arch-utils.h.  */
>>> diff --git a/gdb/disasm.c b/gdb/disasm.c
>>> index 16736e54997..7209cfc227b 100644
>>> --- a/gdb/disasm.c
>>> +++ b/gdb/disasm.c
>>> @@ -197,7 +197,12 @@ gdb_disassembler_memory_reader::dis_asm_read_memory
>>>     (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
>>>      struct disassemble_info *info) noexcept
>>>   {
>>> -  return target_read_code (memaddr, myaddr, len);
>>> +  auto res = catch_exceptions<int, -1> ([&]
>>> +    {
>>> +      return target_read_code (memaddr, myaddr, len);
>>> +    });
>>> +
>>> +  return res;
>>>   }
>>>   /* Wrapper of memory_error.  */
>>> diff --git a/gdb/event-top.h b/gdb/event-top.h
>>> index 846d1e48289..d5905527c2a 100644
>>> --- a/gdb/event-top.h
>>> +++ b/gdb/event-top.h
>>> @@ -24,6 +24,8 @@
>>>   #include <signal.h>
>>> +#include "extension.h"
>>> +
>>>   struct cmd_list_element;
>>>   /* The current quit handler (and its type).  This is called from the
>>> @@ -81,6 +83,29 @@ extern void quit_serial_event_set ();
>>>   extern void quit_serial_event_clear ();
>>> +/* Wrap f (args) and handle exceptions by:
>>> +   - returning val, and
>>> +   - calling set_quit_flag or set_force_quit_flag, if needed.  */
>>> +
>>> +template <typename R, R val, typename F, typename... Args>
>>> +static R
>>> +catch_exceptions (F &&f, Args&&... args)
>>> +{
>>> +   try
>>> +     {
>>> +       return f (std::forward<Args> (args)...);
>>> +     }
>>> +   catch (const gdb_exception &ex)
>>> +     {
>>> +       if (ex.reason == RETURN_QUIT)
>>> +     set_quit_flag ();
>>> +       else if (ex.reason == RETURN_FORCED_QUIT)
>>> +     set_force_quit_flag ();
>>> +     }
>>> +
>>> +   return val;
>>> +}
>>> +
>>>   extern void display_gdb_prompt (const char *new_prompt);
>>>   extern void gdb_setup_readline (int);
>>>   extern void gdb_disable_readline (void);
>>> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
>>> index 7e272c719c3..0854d571ecf 100644
>>> --- a/gdb/gdb_bfd.c
>>> +++ b/gdb/gdb_bfd.c
>>> @@ -930,29 +930,6 @@ gdb_bfd_openw (const char *filename, const char 
>>> *target)
>>>     return gdb_bfd_ref_ptr::new_reference (result);
>>>   }
>>> -/* Wrap f (args) and handle exceptions by:
>>> -   - returning val, and
>>> -   - calling set_quit_flag or set_force_quit_flag, if needed.  */
>>> -
>>> -template <typename R, R val, typename F, typename... Args>
>>> -static R
>>> -catch_exceptions (F &&f, Args&&... args)
>>> -{
>>> -   try
>>> -     {
>>> -       return f (std::forward<Args> (args)...);
>>> -     }
>>> -   catch (const gdb_exception &ex)
>>> -     {
>>> -       if (ex.reason == RETURN_QUIT)
>>> -     set_quit_flag ();
>>> -       else if (ex.reason == RETURN_FORCED_QUIT)
>>> -     set_force_quit_flag ();
>>> -     }
>>> -
>>> -   return val;
>>> -}
>>> -
>>>   /* See gdb_bfd.h.  */
>>>   gdb_bfd_ref_ptr
>>>
>>> base-commit: 4e7b18de44be59e8a85b62aa0d89d0c4a54d0dbe
>>> -- 
>>> 2.35.3
>>
>