From patchwork Mon Aug 8 15:06:48 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 14426 Received: (qmail 55895 invoked by alias); 8 Aug 2016 15:06:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55884 invoked by uid 89); 8 Aug 2016 15:06:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=palves, 2023, Fine, UD:aarch64.c X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 08 Aug 2016 15:06:51 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 400297AE96; Mon, 8 Aug 2016 15:06:50 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u78F6mpi002090; Mon, 8 Aug 2016 11:06:49 -0400 Subject: Re: [PATCH 4/5]: Enhancements to "flags": i386 cleanup To: Doug Evans References: <047d7b5dbb865204bd052cf0bc2b@google.com> <2026a39c-0b53-9142-74ce-091bc73832d8@redhat.com> Cc: gdb-patches , cole945@gmail.com From: Pedro Alves Message-ID: <7635a6d6-4059-6b23-952c-a88dbfef3b18@redhat.com> Date: Mon, 8 Aug 2016 16:06:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Hi Doug, I was reverting this today, but the revert stumbles on something, and I think this must be fixed before 7.12 is released. See below. On 07/22/2016 08:16 PM, Doug Evans wrote: > On Wed, Jul 20, 2016 at 11:17 AM, Pedro Alves wrote: >> Hi Doug, >> >> On 02/29/2016 11:09 PM, Doug Evans wrote: >>> Hi. >>> >>> This patch just simplifies things by removing the "end" spec in >>> i386 eflags definitions, and is otherwise a nop. >>> >>> I removed them because they're redundant. >>> >> >> I noticed that this makes older gdbs reject the new target descriptions. >> E.g., gdb 7.11.1 against master gdbserver: >> >> Remote debugging using :9999 >> warning: while parsing target description (at line 24): Field "CF" has neither type nor bit position >> warning: Could not load XML target description; ignoring >> >> Reverting the patch makes old gdb grok the tdesc again (git revert 49b7ae7bb8f2). >> >> Since it was meant as a cleanup, I think we should revert >> it on grounds of avoiding a back compatibility break. WDYT? > > Fine by me. > Testing the revert against gdbserver regresses caught gcore.exp: Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/gcore.exp ... FAIL: gdb.base/gcore.exp: corefile restored general registers FAIL: gdb.base/gcore.exp: corefile restored all registers Turns out that adding an "end" field back now makes gdb consider the flags as bitfields. That is, with: - + etc., we now get: rip 0x4005ea 0x4005ea -eflags 0x202 [ IF ] +eflags 0x202 [ CF=0 PF=0 AF=0 ZF=0 SF=0 TF=0 IF=1 DF=0 OF=0 NT=0 RF=0 VM=0 AC=0 VIF=0 VIP=0 ID=0 ] cs 0x33 51 And indeed, regenerating the features/*.c files gives us: --- c/gdb/features/i386/amd64-avx-linux.c +++ w/gdb/features/i386/amd64-avx-linux.c @@ -20,23 +20,23 @@ initialize_tdesc_amd64_avx_linux (void) feature = tdesc_create_feature (result, "org.gnu.gdb.i386.core"); type = tdesc_create_flags (feature, "i386_eflags", 4); - tdesc_add_flag (type, 0, "CF"); - tdesc_add_flag (type, 1, ""); - tdesc_add_flag (type, 2, "PF"); - tdesc_add_flag (type, 4, "AF"); - tdesc_add_flag (type, 6, "ZF"); - tdesc_add_flag (type, 7, "SF"); - tdesc_add_flag (type, 8, "TF"); - tdesc_add_flag (type, 9, "IF"); - tdesc_add_flag (type, 10, "DF"); - tdesc_add_flag (type, 11, "OF"); - tdesc_add_flag (type, 14, "NT"); - tdesc_add_flag (type, 16, "RF"); - tdesc_add_flag (type, 17, "VM"); - tdesc_add_flag (type, 18, "AC"); - tdesc_add_flag (type, 19, "VIF"); - tdesc_add_flag (type, 20, "VIP"); - tdesc_add_flag (type, 21, "ID"); + tdesc_add_bitfield (type, "CF", 0, 0); + tdesc_add_bitfield (type, "", 1, 1); + tdesc_add_bitfield (type, "PF", 2, 2); + tdesc_add_bitfield (type, "AF", 4, 4); + tdesc_add_bitfield (type, "ZF", 6, 6); + tdesc_add_bitfield (type, "SF", 7, 7); + tdesc_add_bitfield (type, "TF", 8, 8); + tdesc_add_bitfield (type, "IF", 9, 9); + tdesc_add_bitfield (type, "DF", 10, 10); + tdesc_add_bitfield (type, "OF", 11, 11); + tdesc_add_bitfield (type, "NT", 14, 14); + tdesc_add_bitfield (type, "RF", 16, 16); Etc. However this is not what we want; we want these to continue to be treated as flags. (I.e., the regeneration should have come out empty.) Seems like the original change is thus not only a backward compatibility break, but a forward compatibility break as well, unfortunately. I tried to make gdb treat "end" == "start" the same as not specifying "end", with: which kind of looks correct, actually, given the "cpsr_flags" name, and the odd mix of bitfields and flags? However, it also produces this: diff --git c/gdb/features/i386/amd64-avx-mpx-linux.c w/gdb/features/i386/amd64-avx-mpx-linux.c index 4605480..456f262 100644 --- c/gdb/features/i386/amd64-avx-mpx-linux.c +++ w/gdb/features/i386/amd64-avx-mpx-linux.c @@ -191,8 +191,8 @@ initialize_tdesc_amd64_avx_mpx_linux (void) tdesc_set_struct_size (type, 8); tdesc_add_bitfield (type, "base", 12, 63); tdesc_add_bitfield (type, "reserved", 2, 11); - tdesc_add_bitfield (type, "preserved", 1, 1); - tdesc_add_bitfield (type, "enabled", 0, 0); + tdesc_add_flag (type, 1, "preserved"); + tdesc_add_flag (type, 0, "enabled"); type = tdesc_create_union (feature, "cfgu"); field_type = tdesc_named_type (feature, "data_ptr"); which doesn't look so right. Maybe the mpx descriptions are new enough that we could change them, not sure. But I wouldn't know how best to change them to avoid this. Is there something else that could/should be used to distinguish flags vs bitfields other than "end" being present? I put the reversion patch in the users/palves/revert-tdesc-remove-end-spec branch, in case it helps. Thanks, Pedro Alves diff --git c/gdb/xml-tdesc.c w/gdb/xml-tdesc.c index aa58385..34f2d18 100644 --- c/gdb/xml-tdesc.c +++ w/gdb/xml-tdesc.c @@ -414,7 +414,7 @@ tdesc_start_field (struct gdb_xml_parser *parser, _("Bitfield \"%s\" does not fit in struct")); } - if (end == -1) + if (start == end || end == -1) { if (field_type != NULL) tdesc_add_typed_bitfield (t, field_name, start, start, field_type); Regenerating the .c files with that produces changes like these: diff --git i/gdb/features/aarch64.c w/gdb/features/aarch64.c index cec6956..e9eaed8 100644 --- i/gdb/features/aarch64.c +++ w/gdb/features/aarch64.c @@ -19,10 +19,10 @@ initialize_tdesc_aarch64 (void) feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.core"); type = tdesc_create_flags (feature, "cpsr_flags", 4); tdesc_add_flag (type, 0, "SP"); - tdesc_add_bitfield (type, "", 1, 1); + tdesc_add_flag (type, 1, ""); tdesc_add_bitfield (type, "EL", 2, 3); tdesc_add_flag (type, 4, "nRW"); - tdesc_add_bitfield (type, "", 5, 5); + tdesc_add_flag (type, 5, ""); tdesc_add_flag (type, 6, "F"); tdesc_add_flag (type, 7, "I"); tdesc_add_flag (type, 8, "A");