Re: [PATCH] [gdb] Fix assert in delete_breakpoint

[Simon Marchi <simark@simark.ca>]

On 11/27/23 05:19, Tom de Vries wrote:
> On 11/22/23 13:44, Tom de Vries wrote:
>> Hmm, looking at this backtrace, it seems that the forced_quit is thrown during update_global_location_list.
>>
>> Removing the QUIT from target_write_with_progress makes the test-case pass, but the impact of that is likely to broad.
>>
>> This patch removes the QUIT from default_memory_remove_breakpoint only, which also fixes the test-case.
>>
>> Is this an acceptible approach?
> 
> This uses the same approach, but implements it in a less intrusive way, by adding a suppress_quit variable.
> 
> Any comments about this approach?
> 
> Thanks,
> - Tom
> 
> diff --git a/gdb/mem-break.c b/gdb/mem-break.c
> index abff4b47081..ce61e055945 100644
> --- a/gdb/mem-break.c
> +++ b/gdb/mem-break.c
> @@ -71,6 +71,9 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
>    return val;
>  }
> 
> +extern bool suppress_quit;
> +#define SCOPED_SUPPRESS_QUIT \
> +  scoped_restore save_suppress_quit = make_scoped_restore (&suppress_quit, true)
> 
>  int
>  default_memory_remove_breakpoint (struct gdbarch *gdbarch,
> @@ -80,6 +83,7 @@ default_memory_remove_breakpoint (struct gdbarch *gdbarch,
> 
>    gdbarch_sw_breakpoint_from_kind (gdbarch, bp_tgt->kind, &bplen);
> 
> +  SCOPED_SUPPRESS_QUIT;
>    return target_write_raw_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
>                    bplen);
>  }
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 7a1841ba21e..b5e6d792383 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -654,11 +654,17 @@ quit (void)
>  #endif
>  }
> 
> +extern bool suppress_quit;
> +bool suppress_quit;
> +
>  /* See defs.h.  */
> 
>  void
>  maybe_quit (void)
>  {
> +  if (suppress_quit)
> +    return;
> +
>    if (!is_main_thread ())
>      return;
> 
> 

I believe that something like this (suppress_quit) would be good to
have.  As you said, removing some QUITs from functions that do memory
reads and writes would be too broad.  In some cases, you want those to
be interruptible, and in other cases you don't.

For instance, you ask GDB to print a lot of memory, it's taking too
long, you want to stop.  There's not risk in stopping that operation in
the middle.  Then let's say you ask GDB to write a big object in memory,
it's taking too long, you want to interrupt it.  It might leave you with
a half-written object, but you know the risks of interrupting your write
operation, that's fine.

On the other hand, I suspect that any memory read / write used to
implement GDB's internal logic should be considered by default unsafe to
interrupt.  Otherwise, that code should be written in a way that ensures
that should exceptions get thrown, the internal state is left in a
coherent... state, either by rolling back changes or otherwise.  That
sounds difficult, if not impossible.  So in those cases, I think the
only safe thing to do is suppress QUITs.

The question is, how to decide where QUITs should be suppressed.  Here's
a random backtrace I just grabbed from a maybe_quit call under an
update_global_location_list call:

    #0  maybe_quit () at /home/smarchi/src/binutils-gdb/gdb/utils.c:662
    #1  0x000056366ecaa8ee in target_read (ops=0x56367c2f4240 <the_amd64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7ffe481b40e0 "HA\033H\376\177", offset=93824992235889,
        len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1972
    #2  0x000056366eca95a0 in target_read_memory (memaddr=0x555555555171, myaddr=0x7ffe481b40e0 "HA\033H\376\177", len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1755
    #3  0x000056366dc0f6be in default_memory_insert_breakpoint (gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/mem-break.c:54
    #4  0x000056366c17574a in gdbarch_memory_insert_breakpoint (gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:2875
    #5  0x000056366dc0fb94 in memory_insert_breakpoint (ops=0x56367c2f4240 <the_amd64_linux_nat_target>, gdbarch=0x61c00001d080, bp_tgt=0x616000073f28)
        at /home/smarchi/src/binutils-gdb/gdb/mem-break.c:92
    #6  0x000056366c0067d5 in memory_breakpoint_target<process_stratum_target>::insert_breakpoint (this=0x56367c2f4240 <the_amd64_linux_nat_target>, gdbarch=0x61c00001d080,
        bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/target.h:2435
    #7  0x000056366ecacf34 in target_insert_breakpoint (gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/target.c:2359
    #8  0x000056366c52ebd5 in code_breakpoint::insert_location (this=0x611000045dc0, bl=0x616000073e80) at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:11977
    #9  0x000056366c477a64 in insert_bp_location (bl=0x616000073e80, tmp_error_stream=0x7ffe481b48f0, disabled_breaks=0x7ffe481b4700, hw_breakpoint_error=0x7ffe481b4710,
        hw_bp_error_explained_already=0x7ffe481b4720) at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:2901
    #10 0x000056366c481f9c in insert_breakpoint_locations () at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:3291
    #11 0x000056366c529ae4 in update_global_location_list (insert_mode=UGLL_MAY_INSERT) at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:11720
    #12 0x000056366c54998d in breakpoint_re_set () at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:13271
    #13 0x000056366deccb27 in objfile_relocate (objfile=0x614000005440, new_offsets=std::__debug::vector of length 38, capacity 38 = {...}) at /home/smarchi/src/binutils-gdb/gdb/objfiles.c:707
    #14 0x000056366e8aaa6b in svr4_relocate_main_executable () at /home/smarchi/src/binutils-gdb/gdb/solib-svr4.c:3004
    #15 0x000056366e8aaefe in svr4_solib_create_inferior_hook (from_tty=0) at /home/smarchi/src/binutils-gdb/gdb/solib-svr4.c:3048
    #16 0x000056366e904f3c in solib_create_inferior_hook (from_tty=0) at /home/smarchi/src/binutils-gdb/gdb/solib.c:1217
    #17 0x000056366d717f57 in post_create_inferior (from_tty=0) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:275
    #18 0x000056366d71a14e in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:486
    #19 0x000056366d71a5b2 in run_command (args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:512
    #20 0x000056366c8d66d5 in do_simple_func (During symbol reading: macro `CTRL' redefined at /usr/include/x86_64-linux-gnu/sys/ttydefaults.h:55; original definition at /usr/include/readline/chardefs.h:51
    args=0x0, from_tty=1, c=0x61200005bcc0) at /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
    #21 0x000056366c8ebe3b in cmd_func (cmd=0x61200005bcc0, args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2735
    #22 0x000056366ee07837 in execute_command (p=0x6020000393d1 "", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/top.c:575
    #23 0x000056366d2e9cdb in command_handler (command=0x6020000393d0 "") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:566
    #24 0x000056366d2eafd1 in command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:802
    #25 0x000056366ef64bdb in tui_command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
    #26 0x000056366d2e7d40 in gdb_rl_callback_handler (rl=0x602000039370 "r") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:259
    #27 0x00007f3ad551246d in rl_callback_read_char () from /lib/x86_64-linux-gnu/libreadline.so.8
    #28 0x000056366d2e7764 in gdb_rl_callback_read_char_wrapper_noexcept () at /home/smarchi/src/binutils-gdb/gdb/event-top.c:195
    #29 0x000056366d2e7990 in gdb_rl_callback_read_char_wrapper (client_data=0x611000001440) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:234
    #30 0x000056366f0b730c in stdin_event_handler (error=0, client_data=0x611000001440) at /home/smarchi/src/binutils-gdb/gdb/ui.c:155
    #31 0x0000563670708345 in handle_file_event (file_ptr=0x60700000dd10, ready_mask=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
    #32 0x0000563670708c58 in gdb_wait_for_event (block=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
    #33 0x00005636707069ce in gdb_do_one_event (mstimeout=-1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
    #34 0x000056366db776a8 in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:407
    #35 0x000056366db77e4b in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:471
    #36 0x000056366db7d3d4 in captured_main (data=0x7ffe481b6780) at /home/smarchi/src/binutils-gdb/gdb/main.c:1324
    #37 0x000056366db7d511 in gdb_main (During symbol reading: const value length mismatch for 'sigall_set', got 8, expected 0
    args=0x7ffe481b6780) at /home/smarchi/src/binutils-gdb/gdb/main.c:1343
    #38 0x000056366bbf0703 in main (argc=5, argv=0x7ffe481b68f8) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:39

If an exception gets thrown at that maybe_quit, I guess it goes
directly through that stack to start_event_loop (ignoring the special
readline handling)?  So, we end up with an half-started inferior?
So, should run_command_1 suppress QUITs during all its execution?

Simon