Re: [PATCH v2 6/6] QUIT processing w/ explicit sync_quit_force_run check

[Pedro Alves <pedro@palves.net>]

On 2021-08-23 12:20 a.m., Kevin Buettner wrote:
> Aside from the extension language updates, I found two other
> cases which required explicit checks of sync_quit_force_run...
> 
> 1) remote_fileio_request in gdb/remote-fileio.c:
> 
> The try block calls do_remote_fileio_request which can (in turn)
> call one of the functions in remote_fio_func_map[].  Taking the
> first one, remote_fileio_func_open(), we have the following call
> path to maybe_quit():
> 
>   "_ZL23remote_fileio_func_openP13remote_targetPc"
>     -> "_Z18target_read_memorymPhl"
>     -> "_Z11target_readP10target_ops13target_objectPKcPhml"
>     -> "_Z10maybe_quitv";

These would be easier to read if you pass them through c++filt.

$ cat /tmp/foo | c++filt 

  "remote_fileio_func_open(remote_target*, char*)"
    -> "target_read_memory(unsigned long, unsigned char*, long)"
    -> "target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long)"
    -> "maybe_quit()";

> 
> Since there is a path to maybe_quit(), we must ensure that the
> catch block is not permitted to swallow a QUIT representing a
> SIGTERM.
> 
> However, for this case, we must take care not to change the way
> that Ctrl-C / SIGINT is handled; we want to send a suitable
> EINTR reply to the remote target should that happen.  That
> being the case, I added an explicit check for sync_quit_force_run;
> if set, it rethrows the exception.

Again makes me wonder whether a separate exception type for sigterm
wouldn't address this whole thing better.

> 
> 2) mi_execute_command in gdb/mi/mi-main.c:
> 
> The try block calls captured_mi_execute_command(); there exists
> a call path to maybe_quit():
> 
>   "_ZL27captured_mi_execute_commandP6ui_outP8mi_parse"
>     -> "_ZL14mi_cmd_executeP8mi_parse"
>     -> "_Z17get_current_framev"
>     -> "_ZL23get_prev_frame_always_1P10frame_info"
>     -> "_ZL30frame_register_unwind_locationP10frame_infoiPiP9lval_typePmS1_"
>     -> "_Z21frame_register_unwindP10frame_infoiPiS1_P9lval_typePmS1_Ph"
>     -> "_Z24value_entirely_availableP5value"
>     -> "_Z16value_fetch_lazyP5value"
>     -> "_ZL23value_fetch_lazy_memoryP5value"
>     -> "_Z17read_value_memoryP5valuelimPhm"
>     -> "_Z10maybe_quitv";
> 
> That being the case, we can't allow the exception handler (catch block)
> to swallow a gdb_exception_quit for SIGTERM.  However, it does seem
> reasonable to output the exception via the mi interface so that some
> suitable message regarding SIGTERM might be printed; therefore, I
> do the check of sync_quit_force_run and the potential throw at the
> end of the catch block.
> 
> (While doing all of this, it's occurred to me that these checks of the
> global could be avoided by introducing a new exception for SIGTERM.
> I haven't explored the implications of doing this though.)

Ah-ha!  Yeah.

If we don't do that, then this patch seems fine.