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 83B133858D37 for ; Mon, 3 Apr 2023 11:10:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 83B133858D37 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=1680520213; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=tNVvgDRatfHQpb9ikxpr7SKvq76fZM72orf4sMZ3HYI=; b=TObVSUaay5ZdUmsa4wWD57cEQHwQnOOv0vzlgWsUMWFoQoqfLG2dserMlLmzKOuxGHgiRN NPMe7LTBNvhJpevbuZjvVUjd69u9bGNsTPQpCuGhpn+IV4I2JD3/UOQucUiO3rkv9ipqgm Y7hkyFfWW3Qn+b/BZBbIiaK2vycZWhE= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-625-B32cYQzxNvC38ImEtjXzsA-1; Mon, 03 Apr 2023 07:10:12 -0400 X-MC-Unique: B32cYQzxNvC38ImEtjXzsA-1 Received: by mail-qk1-f199.google.com with SMTP id ou5-20020a05620a620500b007423e532628so12843427qkn.5 for ; Mon, 03 Apr 2023 04:10:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680520211; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tNVvgDRatfHQpb9ikxpr7SKvq76fZM72orf4sMZ3HYI=; b=Zu0HezzyMae0VCIXJgkWvvLd3xGVtKenvcX6xz+CudavqA/QSmfkMr9sy3yw5DqOUs SPKU6U1FODvg47EW5NMY1XFq4kNhbh8OU9rD4E4VQS5pefVd8sl9cfC5vdMCFHycUjuf qN0oVGQnBcyR3Uj8qzCvQi+TJ8orSLDzCPQgcCAiLCdnwRylCYXvvTtnASlcjdrqfGSg rvExN5Yrk9/m5uJ2FAiJqrd76/3bWMcrIyYaGB0rdWkSB7mNES0tf3BvnpxoAnxm5Xk1 cBk6tkcgNRbtHXWN3MYPToV0d+m271ave7TVZKwYqkHBmYtw5t8kUkQmjbAFi5TTxMUB fdDw== X-Gm-Message-State: AAQBX9c+jdP3R/Dvx3uga3fl8kgSNNtBl54BhZptLid+6fNgb7GGHFm3 f36Tq5V8uv6ZD7LoJlWUtUEhwI/NVyENp1DZqhYLONfOWBirklTeLjxBE7wxwHJmjKlI2NDC6o6 cCv8s10zsVVZ6677XS5v/Z4QKavrQDyZIJW3YBTzSmylP5QehM9g35ZXQoxiiOVrtg+sDOcTLxv GmpO+kJw== X-Received: by 2002:ad4:5ca7:0:b0:5cc:e059:efa3 with SMTP id q7-20020ad45ca7000000b005cce059efa3mr58965958qvh.23.1680520211231; Mon, 03 Apr 2023 04:10:11 -0700 (PDT) X-Google-Smtp-Source: AKy350a0dYA2e542gHI765QifXS6DfeOf6TS7WlB8rMMS1tPSWVyyXZGTBHkJIC2ZIoqDQ7X/gMtHA== X-Received: by 2002:ad4:5ca7:0:b0:5cc:e059:efa3 with SMTP id q7-20020ad45ca7000000b005cce059efa3mr58965914qvh.23.1680520210793; Mon, 03 Apr 2023 04:10:10 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id j2-20020a0cc342000000b005dd8b93459esm2584032qvi.54.2023.04.03.04.10.10 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Apr 2023 04:10:10 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/testsuite: gdb.server/server-kill.exp 'info frame' before kill_server In-Reply-To: <8c339273cc142b66429ee0a48b0ae974250ecf3b.1678891287.git.aburgess@redhat.com> References: <8c339273cc142b66429ee0a48b0ae974250ecf3b.1678891287.git.aburgess@redhat.com> Date: Mon, 03 Apr 2023 12:10:08 +0100 Message-ID: <87zg7pcjcf.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.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_LOTSOFHASH,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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: Andrew Burgess writes: > This commit follows on from the following two commits: > > commit 80dc83fd0e70f4d522a534bc601df5e05b81d564 > Date: Fri Jun 11 11:30:47 2021 +0100 > > gdb/remote: handle target dying just before a stepi > > And: > > commit 079f190d4cfc6aa9c934b00a9134bc0fcc172d53 > Date: Thu Mar 9 10:45:03 2023 +0100 > > [gdb/testsuite] Fix gdb.server/server-kill.exp for remote target > > The first of these issues commits fixes an issue in GDB and tried to > extend the gdb.server/server-kill.exp test to cover the GDB fix. > > Unfortunately, the changes to gdb.server/server-kill.exp were not > correct, and were causing problems when trying to run with the > remote-gdbserver-on-localhost board file. > > The second commit reverts some of the gdb.server/server-kill.exp > changes introduced in the first commit so that the test will now work > correctly with the remote-gdbserver-on-localhost board file. > > The second commit is just about GDB's testing infrastructure -- it's > not about the original fix to GDB from the first commit. > > While reviewing the second commit I wanted to check that the problem > fixed in the first commit is still being tested by the > gdb.server/server-kill.exp script, so I reverted the change to > breakpoint.c that is the core of the first commit and ran the test > script ..... and saw no failures. > > The first commit is about GDB discovering that gdbserver has dies > while trying to insert a breakpoint. As soon as GDB spots that > gdbserver is gone we mourn the remote inferior, which ends up deleting > all the breakpoints associated with the remote inferiors. We then > throw an exception which is caught in the insert breakpoints code, and > we try to display an error that includes the breakpoint number > .... but the breakpoint has already been deleted ... and so GDB > crashes. > > After digging a little, what I found is that today, when the test does > 'stepi' the first thing we end up doing is calculating the frame-id as > part of the stepi logic, it is during this frame-id calculation that > we mourn the remote inferior, delete the breakpoints, and throw an > exception. The exception is caught by the top level interpreter loop, > and so we never try to print the breakpoint number which is what > caused the original crash. > > If I add an 'info frame' command to the test script, prior to killing > gdbserver, then now when we 'stepi' GDB already has the frame-id > calculated, and the first thing we do is try to insert the > breakpoints, this will trigger the original bug. > > In order to reproduce this experiment you'll need to change a function > in breakpoint.c, like this: > > static void > rethrow_on_target_close_error (const gdb_exception &e) > { > return; > } > > Then run gdb.server/server-kill.exp with and without this patch. You > should find that without this patch there are zero test failures, > while with this patch there will be one failure like this: > > (gdb) PASS: gdb.server/server-kill.exp: test_stepi: info frame > Executing on target: kill -9 4513 (timeout = 300) > builtin_spawn -ignore SIGHUP kill -9 4513 > stepi > ../../src/gdb/breakpoint.c:2863: internal-error: insert_bp_location: Assertion `bl->owner != nullptr' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > ----- Backtrace ----- > ... > --- > gdb/testsuite/gdb.server/server-kill.exp | 6 ++++++ > 1 file changed, 6 insertions(+) I fixed a few grammatical errors in the commit message and pushed this patch. The updated version is below. Let me know if this causes anyone any problems. Thanks, Andrew --- commit 71f18376db954e95a44a9281d05699a228070f77 Author: Andrew Burgess Date: Wed Mar 15 14:18:37 2023 +0000 gdb/testsuite: gdb.server/server-kill.exp 'info frame' before kill_server This commit follows on from the following two commits: commit 80dc83fd0e70f4d522a534bc601df5e05b81d564 Date: Fri Jun 11 11:30:47 2021 +0100 gdb/remote: handle target dying just before a stepi And: commit 079f190d4cfc6aa9c934b00a9134bc0fcc172d53 Date: Thu Mar 9 10:45:03 2023 +0100 [gdb/testsuite] Fix gdb.server/server-kill.exp for remote target The first of these commits fixed an issue in GDB and tried to extend the gdb.server/server-kill.exp test to cover the GDB fix. Unfortunately, the changes to gdb.server/server-kill.exp were not correct, and were causing problems when trying to run with the remote-gdbserver-on-localhost board file. The second commit reverts some of the gdb.server/server-kill.exp changes introduced in the first commit so that the test will now work correctly with the remote-gdbserver-on-localhost board file. The second commit is just about GDB's testing infrastructure -- it's not about the original fix to GDB from the first commit, the actual GDB change was fine. While reviewing the second commit I wanted to check that the problem fixed in the first commit is still being tested by the gdb.server/server-kill.exp script, so I reverted the change to breakpoint.c that is the core of the first commit and ran the test script ..... and saw no failures. The first commit is about GDB discovering that gdbserver has died while trying to insert a breakpoint. As soon as GDB spots that gdbserver is gone we mourn the remote inferior, which ends up deleting all the breakpoints associated with the remote inferiors. We then throw an exception which is caught in the insert breakpoints code, and we try to display an error that includes the breakpoint number .... but the breakpoint has already been deleted ... and so GDB crashes. After digging a little, what I found is that today, when the test does 'stepi' the first thing we end up doing is calculating the frame-id as part of the stepi logic, it is during this frame-id calculation that we mourn the remote inferior, delete the breakpoints, and throw an exception. The exception is caught by the top level interpreter loop, and so we never try to print the breakpoint number which is what caused the original crash. If I add an 'info frame' command to the test script, prior to killing gdbserver, then now when we 'stepi' GDB already has the frame-id calculated, and the first thing we do is try to insert the breakpoints, this will trigger the original bug. In order to reproduce this experiment you'll need to change a function in breakpoint.c, like this: static void rethrow_on_target_close_error (const gdb_exception &e) { return; } Then run gdb.server/server-kill.exp with and without this patch. You should find that without this patch there are zero test failures, while with this patch there will be one failure like this: (gdb) PASS: gdb.server/server-kill.exp: test_stepi: info frame Executing on target: kill -9 4513 (timeout = 300) builtin_spawn -ignore SIGHUP kill -9 4513 stepi ../../src/gdb/breakpoint.c:2863: internal-error: insert_bp_location: Assertion `bl->owner != nullptr' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. ----- Backtrace ----- ... diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp index 0e9df6e1a5f..4b40913fff6 100644 --- a/gdb/testsuite/gdb.server/server-kill.exp +++ b/gdb/testsuite/gdb.server/server-kill.exp @@ -137,6 +137,12 @@ proc_with_prefix test_stepi {} { return } + # Ensure GDB has computed the frame-id for the current frame + # before we kill the gdbserver. With the frame-id cached when we + # stepi below the first packets we try to send to gdbserver will + # be from within the breakpoint insertion process. + gdb_test "info frame" "Stack level 0, .*" + kill_server gdb_test "stepi" "(Target disconnected|Remote connection closed|Remote communication error).*"