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

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> 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?

Hi Simon,

I've been away for a while, but I'm now back and will take a look at
this regression.

Thanks,
Andrew