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 EF5953858024 for ; Fri, 8 Sep 2023 08:54:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EF5953858024 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=1694163280; 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=3nLJeggQAHJUhn7hcQR3j4useTm0h95aN3+FCqdSxSs=; b=TOxBYa6abda1FrpWIIvUqV3096rRIz+ztRU2N020uk30x9+F3tG6izsdiAyvoxp5dxZsiR ohVT7TZNEYTd+yK4bJ+G8ZEJLciYdq96Sl+BhUwJpnRrk4bdj1xLyJ1usraK/lwiaqGCEe QHPxd2+JdjKROYUm8BKDDBBheSK0Pds= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-459-xd0SaA86N_iMlLgcNfWZYg-1; Fri, 08 Sep 2023 04:54:34 -0400 X-MC-Unique: xd0SaA86N_iMlLgcNfWZYg-1 Received: by mail-ej1-f69.google.com with SMTP id a640c23a62f3a-9a9e12a3093so244085766b.0 for ; Fri, 08 Sep 2023 01:54:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694163273; x=1694768073; 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=3nLJeggQAHJUhn7hcQR3j4useTm0h95aN3+FCqdSxSs=; b=Gbn7fX7NXLqGPW7jfM02OppDtU4QnjZeVTOXG+3k2P1blMcxe4wS9RDjyRbdtQnXV4 t1wm832sdMx264DQT6XCAY80hXGK/EaA3xT1d3aUZckw+6W+nE1+pO0d68jkULaghAie 7J42XgCoCjKgAtf9BxnZJZOPmkvMi/DfjU6VvnWA1G5imnw+pnO2FOajNvybzHgXAmdg S0VinpVpZJJI+9WfwVIzYqu5qwmf1aAPe1a00+pHI1yIbVHx58Rj8ddvN944dCDKD7vE 16niKouOkg/iywKLt8MrDYG/3cvnmUm+DLS03cLMIR3G/EOn+C95tJKjbKr+bIT8DlNz lDEg== X-Gm-Message-State: AOJu0Ywk85/jOWk5oGw3GAlsB0tVgDezIC3FJlNbvps4RmhZT7PEf9gs CcdMWruDwwWxjaEpycgPYqMmzsqJkLgIGLJVTzLLZlmhR8FKmtRJ3BYS7D8H+uZxXloH7Y00WjD ioiBYM++G/LIa7VYgkgHwYg== X-Received: by 2002:a17:906:8475:b0:9a2:474:4aa1 with SMTP id hx21-20020a170906847500b009a204744aa1mr2200622ejc.10.1694163273245; Fri, 08 Sep 2023 01:54:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFAfISzDSocKJLszzr7zwk8x2DMfEjD3Eh9kXYa6VjUyRXqwUy8llkctcB6nLvBHlH5mCP98g== X-Received: by 2002:a17:906:8475:b0:9a2:474:4aa1 with SMTP id hx21-20020a170906847500b009a204744aa1mr2200608ejc.10.1694163272806; Fri, 08 Sep 2023 01:54:32 -0700 (PDT) Received: from localhost ([31.111.84.232]) by smtp.gmail.com with ESMTPSA id s3-20020a170906060300b0099ce188be7fsm737790ejb.3.2023.09.08.01.54.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 08 Sep 2023 01:54:32 -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 09:54:31 +0100 Message-ID: <8734zpm4e0.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=-5.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 Simon, I've been away for a while, but I'm now back and will take a look at this regression. Thanks, Andrew