From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id D841E3858D1E for ; Fri, 8 Sep 2023 10:03:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D841E3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1694167388; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=3aI/sfsNuQRW4YLgsKqBYBcXUp7BKMUCh2ZCCsuwqQI=; b=f3U9qQlcD0ZWMnME1Vb9Kk22gtSZydFFaPDV1687vyXVl2Kff5FL74tVb3JLnCBuqXLThu ZK1psedLOwOiXT7MTcz7Wpf3a7TqUj+zQeO3VYZbNGGOqrxoU1ZLL6i83keKAC9pbEMT4v iSxAInHtv8P2/71RvZw7559wjwA4cRw= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-629-mJkPAZ45NW23jbtY3Bl8MA-1; Fri, 08 Sep 2023 06:03:05 -0400 X-MC-Unique: mJkPAZ45NW23jbtY3Bl8MA-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-401ea9bf934so14576915e9.2 for ; Fri, 08 Sep 2023 03:03:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694167383; x=1694772183; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3aI/sfsNuQRW4YLgsKqBYBcXUp7BKMUCh2ZCCsuwqQI=; b=ADDzCXPxW4XYKUPrWRivn0LliT7oFimHfZaoteaO1QksX5pBjZlyyOFIVrsKhmDYGo 1xWd5M6mwExx6NSLA/3iMtugkfeVFQS9vOROzhwzsY0ZLPT/ofp+cZKbgHCsC74DTRxl fZnNLIZwr1iuUxDYRrSyoDc1tbe9EaACv8y7LuqTmJkKmBrZsOp/OQ3vlfTr4+CLpitB KpLUD8BtMaWfCUBdGzQXPcKKg6wAM4bZYHd03e/EVW0045yYHxKd9+VmIP4Kq4BO9JA/ WuhrfVjtSGGPD1EAG2CYAp1MUT73DvfhPRf+nhdmJdux0R5Ut9eEaSTj8Xj5h36BAt9w EcAg== X-Gm-Message-State: AOJu0YyCBgnT/9bd8S0p64nMNqQz4iw07ONS/8LR6I0K7NN/j9g5oaSG 6JxlZlpLKSLxT8NHpy3+Uqpdmap+XEj4rvXxMOuP+FqbtOsrdszjPDXvP0Pm2qMuIN1YmWdNDdz xbxDt/kfB1To8/iTe+YIunQ== X-Received: by 2002:a7b:c855:0:b0:3fb:a100:2581 with SMTP id c21-20020a7bc855000000b003fba1002581mr2002168wml.14.1694167383588; Fri, 08 Sep 2023 03:03:03 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGMVEULEnxKkk1vsdlMQpyG6IwkAltNy4N9BcobekYObB42QMbdyGkO4XxpNbSSNPKPY0a8cQ== X-Received: by 2002:a7b:c855:0:b0:3fb:a100:2581 with SMTP id c21-20020a7bc855000000b003fba1002581mr2002138wml.14.1694167383050; Fri, 08 Sep 2023 03:03:03 -0700 (PDT) Received: from localhost ([31.111.84.232]) by smtp.gmail.com with ESMTPSA id e11-20020a05600c108b00b0040210a27e29sm1555439wmd.32.2023.09.08.03.03.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 08 Sep 2023 03:03:02 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: pedro@palves.net Subject: Re: [PATCH 2/2] gdb: MI stopped events when unwindonsignal is on In-Reply-To: <73807dfe-139f-431f-928c-ba7695cfc1cf@polymtl.ca> References: <73807dfe-139f-431f-928c-ba7695cfc1cf@polymtl.ca> Date: Fri, 08 Sep 2023 11:03:01 +0100 Message-ID: <87zg1xkmne.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: Simon Marchi 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 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\"" \