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,

I pushed the minor testsuite only patch below to fix this issue.

Thanks,
Andrew

---

commit 932a49fff332ba4921dc9e38cf45bf65a301f2c6
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Sep 8 10:45:08 2023 +0100

    gdb/testsuite: fix gdb.mi/mi-condbreak-throw.exp failure
    
    In commit:
    
      commit 3ce8f906be7a55d8c0375e6d360cc53b456d86ae
      Date:   Tue Aug 8 10:45:20 2023 +0100
    
          gdb: MI stopped events when unwindonsignal is on
    
    a new test, gdb.mi/mi-condbreak-throw.exp, was added.  Unfortunately,
    this test would fail when using the native-gdbserver board (and other
    similar boards).
    
    The problem was that one of the expected output patterns included some
    output from the inferior.  When using the native-gdbserver board, this
    output is not printed to GDB's tty, but is instead printed to
    gdbserver's tty, the result is that the expected output no longer
    matches, and the test fails.
    
    Additionally, as the output is actually from the C++ runtime, rather
    than the test's source file, changes to the C++ runtime could cause
    the output to change.
    
    To solve both of these issues, in this commit, I'm removing the
    reference to the inferior's output, and replacing it with '.*', which
    will skip the output if it is present, but is equally happy if the
    output is not present.
    
    After this commit gdb.mi/mi-condbreak-throw.exp now passes on all
    boards, including native-gdbserver.

diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-throw.exp b/gdb/testsuite/gdb.mi/mi-condbreak-throw.exp
index 4151fa18395..7f291244e53 100644
--- a/gdb/testsuite/gdb.mi/mi-condbreak-throw.exp
+++ b/gdb/testsuite/gdb.mi/mi-condbreak-throw.exp
@@ -97,8 +97,7 @@ proc run_test { unwind_on_exception } {
 
 	mi_gdb_test "" \
 	    [multi_line \
-		 "terminate called after throwing an instance of 'int'" \
-		 "~\"\\\\nProgram\"" \
+		 ".*~\"\\\\nProgram\"" \
 		 "~\" received signal SIGABRT, Aborted\\.\\\\n\"${out_ln_re}" \
 		 "\\*stopped,reason=\"signal-received\",signal-name=\"SIGABRT\"\[^\r\n\]+frame=\\{addr=\"$::hex\",\[^\r\n\]+\\}\[^\r\n\]+" \
 		 "&\"Error in testing condition for breakpoint $bpnum:\\\\n\"" \