Re: [PATCH 2/2] gdb: MI stopped events when unwindonsignal is on

[Simon Marchi <simon.marchi@polymtl.ca>]

On 8/8/23 10:45, Andrew Burgess via Gdb-patches wrote:
> This recent commit:
> 
>   commit b1c0ab20809a502b2d2224fecb0dca3ada2e9b22
>   Date:   Wed Jul 12 21:56:50 2023 +0100
> 
>       gdb: avoid double stop after failed breakpoint condition check
> 
> Addressed a problem where two MI stopped events would be reported if a
> breakpoint condition failed due to a signal, this commit was a
> replacement for this commit:
> 
>   commit 2e411b8c68eb2b035b31d5b00d940d4be1a0928b
>   Date:   Fri Oct 14 14:53:15 2022 +0100
> 
>       gdb: don't always print breakpoint location after failed condition check
> 
> which solved the two stop problem, but only for the CLI.  Before both
> of these commits, if a b/p condition failed due to a signal then the
> user would see two stops reported, the first at the location where the
> signal occurred, and the second at the location of the breakpoint.
> 
> By default GDB remains at the location where the signal occurred, so
> the second reported stop can be confusing, this is the problem that
> commit 2e411b8c68eb tried to solve (for the CLI) and b1c0ab20809a
> extended also address the issue for MI too.
> 
> However, while working on another patch I realised that there was a
> problem with GDB after the above commits.  Neither of the above
> commits considered 'set unwindonsignal on'.  With this setting on,
> when an inferior function call fails with a signal GDB will unwind the
> stack back to the location where the inferior function call started.
> In the b/p case we're looking at, the stop should be reported at the
> location of the breakpoint, not at the location where the signal
> occurred, and this isn't what happens.
> 
> This commit fixes this by ensuring that when unwindonsignal is 'on',
> GDB reports a single stop event at the location of the breakpoint,
> this fixes things for both CLI and MI.
> 
> The function call_thread_fsm::should_notify_stop is called when the
> inferior function call completes and GDB is figuring out if the user
> should be notified about this stop event by calling normal_stop from
> fetch_inferior_event in infrun.c.  If normal_stop is called, then this
> notification will be for the location where the inferior call stopped,
> which will be the location at which the signal occurred.
> 
> Prior to this commit, the only time that normal_stop was not called,
> was if the inferior function call completed successfully, this was
> controlled by ::should_notify_stop, which only turns false when the
> inferior function call has completed successfully.
> 
> In this commit I have extended the logic in ::should_notify_stop.  Now
> there are three cases in which ::should_notify_stop will return false,
> and we will not announce the first stop (by calling normal_stop).
> These three reasons are:
> 
>   1. If the inferior function call completes successfully, this is
>   unchanged behaviour,
> 
>   2. If the inferior function call stopped due to a signal and 'set
>   unwindonsignal on' is in effect, and
> 
>   3. If the inferior function call stopped due to an uncaught C++
>   exception, and 'set unwind-on-terminating-exception on' is in
>   effect.
> 
> However, if we don't call normal_stop then we need to call
> async_enable_stdin in call_thread_fsm::should_stop.  Prior to this
> commit this was only done for the case where the inferior function
> call completed successfully.
> 
> In this commit I now call ::should_notify_stop and use this to
> determine if we need to call async_enable_stdin.  With this done we
> now call async_enable_stdin for each of the three cases listed above,
> which means that GDB will exit wait_sync_command_done correctly (see
> run_inferior_call in infcall.c).
> 
> With these two changes the problem is mostly resolved.  However, the
> solution isn't ideal, we've still lost some information.
> 
> Here is how GDB 13.1 behaves, this is before commits b1c0ab20809a and
> 2e411b8c68eb:
> 
>   $ gdb -q /tmp/mi-condbreak-fail \
>         -ex 'set unwindonsignal on' \
>         -ex 'break 30 if (cond_fail())' \
>         -ex 'run'
>   Reading symbols from /tmp/mi-condbreak-fail...
>   Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
>   Starting program: /tmp/mi-condbreak-fail
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   0x0000000000401116 in cond_fail () at /tmp/mi-condbreak-fail.c:24
>   24	  return *p;			/* Crash here.  */
>   Error in testing breakpoint condition:
>   The program being debugged was signaled while in a function called from GDB.
>   GDB has restored the context to what it was before the call.
>   To change this behavior use "set unwindonsignal off".
>   Evaluation of the expression containing the function
>   (cond_fail) will be abandoned.
> 
>   Breakpoint 1, foo () at /tmp/mi-condbreak-fail.c:30
>   30	  global_counter += 1;		/* Set breakpoint here.  */
>   (gdb)
> 
> In this state we see two stop notifications, the first is where the
> signal occurred, while the second is where the breakpoint is located.
> As GDB has unwound the stack (thanks to unwindonsignal) the second
> stop notification reflects where the inferior is actually located.
> 
> Then after commits b1c0ab20809a and 2e411b8c68eb the behaviour changed
> to this:
> 
>   $ gdb -q /tmp/mi-condbreak-fail \
>         -ex 'set unwindonsignal on' \
> 	-ex 'break 30 if (cond_fail())' \
> 	-ex 'run'
>   Reading symbols from /tmp/mi-condbreak-fail...
>   Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
>   Starting program: /tmp/mi-condbreak-fail
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   0x0000000000401116 in cond_fail () at /tmp/mi-condbreak-fail.c:24
>   24	  return *p;			/* Crash here.  */
>   Error in testing condition for breakpoint 1:
>   The program being debugged was signaled while in a function called from GDB.
>   GDB has restored the context to what it was before the call.
>   To change this behavior use "set unwindonsignal off".
>   Evaluation of the expression containing the function
>   (cond_fail) will be abandoned.
>   (gdb) bt 1
>   #0  foo () at /tmp/mi-condbreak-fail.c:30
>   (More stack frames follow...)
>   (gdb)
> 
> This is the broken state.  GDB is reports the SIGSEGV location, but
> not the unwound breakpoint location.  The final 'bt 1' shows that the
> inferior is not located in cond_fail, which is the only location GDB
> reported, so this is clearly wrong.
> 
> After implementing the fixes described above we now get this
> behaviour:
> 
>   $ gdb -q /tmp/mi-condbreak-fail \
>         -ex 'set unwindonsignal on' \
>         -ex 'break 30 if (cond_fail())' \
>         -ex 'run'
>   Reading symbols from /tmp/mi-condbreak-fail...
>   Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
>   Starting program: /tmp/mi-condbreak-fail
>   Error in testing breakpoint condition for breakpoint 1:
>   The program being debugged was signaled while in a function called from GDB.
>   GDB has restored the context to what it was before the call.
>   To change this behavior use "set unwindonsignal off".
>   Evaluation of the expression containing the function
>   (cond_fail) will be abandoned.
> 
>   Breakpoint 1, foo () at /tmp/mi-condbreak-fail.c:30
>   30	  global_counter += 1;		/* Set breakpoint here.  */
>   (gdb)
> 
> This is better.  GDB now reports a single stop at the location of the
> breakpoint, which is where the inferior is actually located.  However,
> by removing the first stop notification we have lost some potentially
> useful information about which signal caused the inferior to stop.
> 
> To address this I've reworked the message that is printed to include
> the signal information.  GDB now reports this:
> 
>   $ gdb -q /tmp/mi-condbreak-fail \
>         -ex 'set unwindonsignal on' \
>         -ex 'break 30 if (cond_fail())' \
>         -ex 'run'
>   Reading symbols from /tmp/mi-condbreak-fail...
>   Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
>   Starting program: /tmp/mi-condbreak-fail
>   Error in testing condition for breakpoint 1:
>   The program being debugged received signal SIGSEGV, Segmentation fault
>   while in a function called from GDB.  GDB has restored the context
>   to what it was before the call.  To change this behavior use
>   "set unwindonsignal off".  Evaluation of the expression containing
>   the function (cond_fail) will be abandoned.
> 
>   Breakpoint 1, foo () at /tmp/mi-condbreak-fail.c:30
>   30	  global_counter += 1;		/* Set breakpoint here.  */
>   (gdb)
> 
> This is better, the user now sees a single stop notification at the
> correct location, and the error message describes which signal caused
> the inferior function call to stop.
> 
> However, we have lost the information about where the signal
> occurred.  I did consider trying to include this information in the
> error message, but, in the end, I opted not too.  I wasn't sure it was
> worth the effort.  If the user has selected to unwind on signal, then
> surely this implies they maybe aren't interested in debugging failed
> inferior calls, so, hopefully, just knowing the signal name will be
> enough.  I figure we can always add this information in later if
> there's a demand for it.

Hi Andrew,

Starting with this commit, I see this failure with the native-gdbserver
board:

FAIL: gdb.mi/mi-condbreak-throw.exp: unwind_on_exception=off: wait for stop (unexpected output)

Do you see it?

Simon