Message ID | 20230112193356.1133696-1-tromey@adacore.com |
---|---|
State | Committed |
Commit | 81b86eced24f905545b58aa6c27478104c364976 |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0CAC03857835 for <patchwork@sourceware.org>; Thu, 12 Jan 2023 19:34:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0CAC03857835 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1673552068; bh=QskoC+KGTHHOsSpyaT0lNClgUo2XjpaoiyEw0pjngUw=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=rNZIwHVLTOqDKyP5GhQyER6g9+Gacj8L3QQYQ62KmAW7e5G/hlsbsgm+Grsj3zhGz 6UCuXdyQLElLy4MeVWyirAlv7cQnm3BkbLdMyJPtc2J45cWTbrA+Y3f0gkZg+BJNiM ipIrKBgtvvStpavW45PYPMbw1Hj+2GL58uPd+J2k= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com [IPv6:2607:f8b0:4864:20::112b]) by sourceware.org (Postfix) with ESMTPS id DB4DF3858C66 for <gdb-patches@sourceware.org>; Thu, 12 Jan 2023 19:34:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DB4DF3858C66 Received: by mail-yw1-x112b.google.com with SMTP id 00721157ae682-4bf16baa865so253972627b3.13 for <gdb-patches@sourceware.org>; Thu, 12 Jan 2023 11:34:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=QskoC+KGTHHOsSpyaT0lNClgUo2XjpaoiyEw0pjngUw=; b=DUP8JsoMA1u3MbFbakZdH8ucEw2Mee+bdfD9vxMDtU1nCgKZFZhHQbiF4NbWfqja15 dU9RtHT+l1Az7c77b/EHjTz61tGMMze1Ii1jW1r07PeTsaFyRF+g4WcI2QHsgiJ+XOmz oYiDxF16r/6oq0K9bKKibYjPg160CBpFt9B1Eb1YQY8GUhPZuBHe64BIODGjnezLtG2Y Gg40n9XgdwvBIIKtEEmn9aaWdf9SnkOhctvMIXOe7FcCTCg3wJFN0qw8x+WPyTUsAoP8 kuOe5dY0WKzxRagCSLxvkAsxVmbOpYStK6sycXSpE7dT1j0Efwddao/pUX0gaOV4odJq pgqQ== X-Gm-Message-State: AFqh2krJcaDCDfKoUIPHdRbLObKZVBXsDnP+0+vBka19pdQRPHCaXNmS wWvU9BEtTToZ4E7obnL9N6XrRkOodZATg5U3 X-Google-Smtp-Source: AMrXdXt/A/EQcLfOw1ZeflvbQrlHK4FX6ulMLsespsQQWd2fnrtjdxMMUoDexkeYvzrZFedxuEab4w== X-Received: by 2002:a81:4e16:0:b0:3cb:1f49:4a59 with SMTP id c22-20020a814e16000000b003cb1f494a59mr26521164ywb.44.1673552044323; Thu, 12 Jan 2023 11:34:04 -0800 (PST) Received: from localhost.localdomain (97-122-76-186.hlrn.qwest.net. [97.122.76.186]) by smtp.gmail.com with ESMTPSA id h10-20020a05620a284a00b006feea093006sm11362691qkp.124.2023.01.12.11.34.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Jan 2023 11:34:03 -0800 (PST) To: gdb-patches@sourceware.org Cc: Tom Tromey <tromey@adacore.com> Subject: [PATCH] Do not record a rejected target description Date: Thu, 12 Jan 2023 12:33:56 -0700 Message-Id: <20230112193356.1133696-1-tromey@adacore.com> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Tom Tromey <tromey@adacore.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Do not record a rejected target description
|
|
Commit Message
Tom Tromey
Jan. 12, 2023, 7:33 p.m. UTC
When connecting to a certain target, gdb issues a warning about the target description: (gdb) target remote localhost:7947 Remote debugging using localhost:7947 warning: Architecture rejected target-supplied description If you then kill the inferior and change the exec-file, this will happen: (gdb) file bar Architecture of file not recognized. After this, debugging doesn't work very well. What happens here is that, despite the warning, target_find_description records the downloaded description in the target_desc_info. Then the "file" command ends up calling set_gdbarch_from_file, which uses that description. It seems to me that, because the architecture rejected the description, it should not be used. That is what this patch implements. --- gdb/target-descriptions.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Comments
>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
Tom> When connecting to a certain target, gdb issues a warning about the
Tom> target description:
Tom> (gdb) target remote localhost:7947
Tom> Remote debugging using localhost:7947
Tom> warning: Architecture rejected target-supplied description
Tom> If you then kill the inferior and change the exec-file, this will
Tom> happen:
Tom> (gdb) file bar
Tom> Architecture of file not recognized.
Tom> After this, debugging doesn't work very well.
Tom> What happens here is that, despite the warning,
Tom> target_find_description records the downloaded description in the
Tom> target_desc_info. Then the "file" command ends up calling
Tom> set_gdbarch_from_file, which uses that description.
Tom> It seems to me that, because the architecture rejected the
Tom> description, it should not be used. That is what this patch
Tom> implements.
We've been using this internally with our misbehaving target and it is
fine, but I'd appreciate someone else's review.
thanks,
Tom
Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: > When connecting to a certain target, gdb issues a warning about the > target description: > > (gdb) target remote localhost:7947 > Remote debugging using localhost:7947 > warning: Architecture rejected target-supplied description > > If you then kill the inferior and change the exec-file, this will > happen: > > (gdb) file bar > Architecture of file not recognized. > > After this, debugging doesn't work very well. > > What happens here is that, despite the warning, > target_find_description records the downloaded description in the > target_desc_info. Then the "file" command ends up calling > set_gdbarch_from_file, which uses that description. > > It seems to me that, because the architecture rejected the > description, it should not be used. That is what this patch > implements. > --- > gdb/target-descriptions.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index 1a451c79b82..38d0b3f8c7d 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -565,7 +565,10 @@ target_find_description (void) > > info.target_desc = tdesc_info->tdesc; > if (!gdbarch_update_p (info)) > - warning (_("Architecture rejected target-supplied description")); > + { > + warning (_("Architecture rejected target-supplied description")); > + tdesc_info->tdesc = nullptr; > + } This seems fine, but it is possible that an exception could take us out of this function too, for example, a misbehaving remote target can cause target_read_description_xml to throw an exception. I guess that any of the issues you were originally seeing would also trigger if we throw an exception. I wonder if a better solution would be to use a RAII object to ensure that tdesc_info->tdesc is always reset to nullptr on scope exit, then at the end of target_find_description, after this line: tdesc_info->fetched = true; we would call a method on the RAII object to indicate that it should NOT clear the tdesc field. Thanks, Andrew
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: >> info.target_desc = tdesc_info->tdesc; >> if (!gdbarch_update_p (info)) >> - warning (_("Architecture rejected target-supplied description")); >> + { >> + warning (_("Architecture rejected target-supplied description")); >> + tdesc_info->tdesc = nullptr; >> + } Andrew> This seems fine, but it is possible that an exception could take us out Andrew> of this function too, for example, a misbehaving remote target can cause Andrew> target_read_description_xml to throw an exception. I guess that any of Andrew> the issues you were originally seeing would also trigger if we throw an Andrew> exception. I don't think this can really happen, because that call is part of the assignment to the field in question: if (tdesc_info->tdesc == nullptr) tdesc_info->tdesc = target_read_description (current_inferior ()->top_target ()); So, if it does throw, the tdesc_info won't be updated. Tom
Tom Tromey <tromey@adacore.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > >>> info.target_desc = tdesc_info->tdesc; >>> if (!gdbarch_update_p (info)) >>> - warning (_("Architecture rejected target-supplied description")); >>> + { >>> + warning (_("Architecture rejected target-supplied description")); >>> + tdesc_info->tdesc = nullptr; >>> + } > > Andrew> This seems fine, but it is possible that an exception could take us out > Andrew> of this function too, for example, a misbehaving remote target can cause > Andrew> target_read_description_xml to throw an exception. I guess that any of > Andrew> the issues you were originally seeing would also trigger if we throw an > Andrew> exception. > > I don't think this can really happen, because that call is part of the > assignment to the field in question: > > if (tdesc_info->tdesc == nullptr) > tdesc_info->tdesc = target_read_description > (current_inferior ()->top_target ()); > > So, if it does throw, the tdesc_info won't be updated. You are, of course, correct. I guess given we know nothing after the assignment can throw the what you have is fine. Hopefully we'll not add an error() call later. Sorry for the noise. Andrew
Andrew> You are, of course, correct. I guess given we know nothing after the Andrew> assignment can throw the what you have is fine. Hopefully we'll not add Andrew> an error() call later. Andrew> Sorry for the noise. No, thank you for taking a look at this. I appreciate it. I'm going to check it in now. Tom
On 2/15/23 10:59, Tom Tromey via Gdb-patches wrote: > Andrew> You are, of course, correct. I guess given we know nothing after the > Andrew> assignment can throw the what you have is fine. Hopefully we'll not add > Andrew> an error() call later. > > Andrew> Sorry for the noise. > > No, thank you for taking a look at this. I appreciate it. I'm going to > check it in now. I see: maint print c-tdesc^M There is no target description to print.^M (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield Simon
Simon> I see: Simon> maint print c-tdesc^M Simon> There is no target description to print.^M Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield Sorry about that. I'll take a look. Tom
Simon> I see: Simon> maint print c-tdesc^M Simon> There is no target description to print.^M Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield Here's a patch that avoids this problem. Let me know what you think. Tom commit a12897d22ccc1d927960fa411b99723d76620af0 Author: Tom Tromey <tromey@adacore.com> Date: Thu Feb 16 12:17:50 2023 -0700 Fix regression in maint_print_struct.exp An earlier patch of mine caused a regression in maint_print_struct.exp. This patch fixes it by modifying the test so that it does not rely on the erroneous target description being available. diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp index 6f411895501..5b7c1489d3e 100644 --- a/gdb/testsuite/gdb.xml/maint_print_struct.exp +++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp @@ -21,12 +21,7 @@ require allow_xml_test gdb_start -# Required registers are not present so it is expected a warning. -# -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " -warning:.*" "setting a new tdesc having only a structure" - -gdb_test "maint print c-tdesc" " +gdb_test "maint print c-tdesc $srcdir/$subdir/maint_print_struct.xml" " .*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r .*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r .*" "printing tdesc with a structure and a bitfield"
On 2/16/23 14:20, Tom Tromey wrote: > Simon> I see: > > Simon> maint print c-tdesc^M > Simon> There is no target description to print.^M > Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield > > Here's a patch that avoids this problem. > Let me know what you think. > > Tom > > commit a12897d22ccc1d927960fa411b99723d76620af0 > Author: Tom Tromey <tromey@adacore.com> > Date: Thu Feb 16 12:17:50 2023 -0700 > > Fix regression in maint_print_struct.exp > > An earlier patch of mine caused a regression in > maint_print_struct.exp. This patch fixes it by modifying the test so > that it does not rely on the erroneous target description being > available. I don't have time to dig in the implementation to review, but I confirmed it fixes the problem on my side, so: Tested-By: Simon Marchi <simon.marchi@efficios.com> Simon
Tom Tromey <tromey@adacore.com> writes: > Simon> I see: > > Simon> maint print c-tdesc^M > Simon> There is no target description to print.^M > Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield > > Here's a patch that avoids this problem. > Let me know what you think. > > Tom > > commit a12897d22ccc1d927960fa411b99723d76620af0 > Author: Tom Tromey <tromey@adacore.com> > Date: Thu Feb 16 12:17:50 2023 -0700 > > Fix regression in maint_print_struct.exp > > An earlier patch of mine caused a regression in > maint_print_struct.exp. This patch fixes it by modifying the test so > that it does not rely on the erroneous target description being > available. > > diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp > index 6f411895501..5b7c1489d3e 100644 > --- a/gdb/testsuite/gdb.xml/maint_print_struct.exp > +++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp > @@ -21,12 +21,7 @@ require allow_xml_test > > gdb_start > > -# Required registers are not present so it is expected a warning. > -# > -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " > -warning:.*" "setting a new tdesc having only a structure" Haven't you just deleted a test for a warning here? > - > -gdb_test "maint print c-tdesc" " This test (without the filename) actually tests for the change in your original patch - if we just update the expected output... > +gdb_test "maint print c-tdesc $srcdir/$subdir/maint_print_struct.xml" " This should probably be as well as retaining the above tests. I'd propose the patch below. thanks, Andrew > .*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r > .*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r > .*" "printing tdesc with a structure and a bitfield" --- commit 416465d0331d3347bebf4c72c544d7d3ecb926e7 Author: Andrew Burgess <aburgess@redhat.com> Date: Fri Feb 17 10:15:27 2023 +0000 gdb: fix regression in gdb.xml/maint_print_struct.exp A regression in gdb.xml/maint_print_struct.exp was introduced with commit: commit 81b86eced24f905545b58aa6c27478104c364976 Date: Fri Jan 6 09:30:40 2023 -0700 Do not record a rejected target description The test relied on an invalid target description being stored within the tdesc_info of the current inferior, the above commit stopped this behaviour. Update the test to check that the invalid architecture is NOT stored, and then check printing the target description directly from the file. diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp index 6f411895501..fbb16aeb8f5 100644 --- a/gdb/testsuite/gdb.xml/maint_print_struct.exp +++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp @@ -21,12 +21,17 @@ require allow_xml_test gdb_start +set xml_file "$srcdir/$subdir/maint_print_struct.xml" + # Required registers are not present so it is expected a warning. # -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " +gdb_test "set tdesc filename $xml_file" " warning:.*" "setting a new tdesc having only a structure" -gdb_test "maint print c-tdesc" " +gdb_test "maint print c-tdesc" \ + "There is no target description to print\\." + +gdb_test "maint print c-tdesc $xml_file" " .*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r .*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r .*" "printing tdesc with a structure and a bitfield"
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: >> -# Required registers are not present so it is expected a warning. >> -# >> -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " >> -warning:.*" "setting a new tdesc having only a structure" Andrew> Haven't you just deleted a test for a warning here? I thought it was just incidental. I looked at the history of the file and the original commit message only discusses handling the "struct" case. Andrew> I'd propose the patch below. Looks good to me, thanks. Approved-By: Tom Tromey <tromey@adacore.com> Tom
Tom Tromey <tromey@adacore.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > >>> -# Required registers are not present so it is expected a warning. >>> -# >>> -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " >>> -warning:.*" "setting a new tdesc having only a structure" > > Andrew> Haven't you just deleted a test for a warning here? > > I thought it was just incidental. I looked at the history of the file > and the original commit message only discusses handling the "struct" > case. > > Andrew> I'd propose the patch below. > > Looks good to me, thanks. > Approved-By: Tom Tromey <tromey@adacore.com> Pushed. Thanks, Andrew
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 1a451c79b82..38d0b3f8c7d 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -565,7 +565,10 @@ target_find_description (void) info.target_desc = tdesc_info->tdesc; if (!gdbarch_update_p (info)) - warning (_("Architecture rejected target-supplied description")); + { + warning (_("Architecture rejected target-supplied description")); + tdesc_info->tdesc = nullptr; + } else { struct tdesc_arch_data *data;