From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id F06393858D28 for ; Tue, 29 Aug 2023 16:37:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F06393858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 37TGb7xr025895 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 29 Aug 2023 12:37:12 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 37TGb7xr025895 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1693327032; bh=S40XDXqdKP968dL/p8ZvfcYPvStLlCReHqARMIQbwG4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=YyF9rviGfi7kIXKMWitSfBwUgozt0UugGPz4PmB+5ImhrUAvk+FMe1xGHQU8ZfL0p 73mdGPfeJwDe95ci8bsh8nRcskBiyCNFMlXoetpOPKoOwLs+jCduUHDDcvtdyDvMRG NiIEEqlwXrZvRtRhj7r2kP2+SEu1mEOX0icN82Cw= Received: from [172.16.0.146] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 47BDA1E092; Tue, 29 Aug 2023 12:37:07 -0400 (EDT) Message-ID: <73807dfe-139f-431f-928c-ba7695cfc1cf@polymtl.ca> Date: Tue, 29 Aug 2023 12:37:06 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] gdb: MI stopped events when unwindonsignal is on To: Andrew Burgess , gdb-patches@sourceware.org Cc: pedro@palves.net References: Content-Language: fr From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 29 Aug 2023 16:37:07 +0000 X-Spam-Status: No, score=-3032.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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