From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128087 invoked by alias); 8 Aug 2016 20:34:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 128070 invoked by uid 89); 8 Aug 2016 20:34:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Etc X-HELO: mail-ua0-f174.google.com Received: from mail-ua0-f174.google.com (HELO mail-ua0-f174.google.com) (209.85.217.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 08 Aug 2016 20:34:33 +0000 Received: by mail-ua0-f174.google.com with SMTP id i31so126389836uai.2 for ; Mon, 08 Aug 2016 13:34:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=dCqQuNj1FvQPjv+EsgSqpQunelurCdX8SYnJtl6GHI0=; b=B8nt5fZjAyZ30TgMia/O3AJwrKACCz7XYSOjPoFkaFFckwhOrGaNpIB6TXxQuo6nd/ jepSsWl9xv+A/COTciQ9KWKKjYGZzEk67y24mS4cEZCveOpN1TdbsGRClHKfMrcGJYbk Gl6IeakPPULHBNvmp1uO9B4AN7hjHHbFVqWs6rT+ae5wQOiUHoBe/oIEec1/SoHAfZPc mep3jYPlKsDTZxGALf+kFjj2Is0INwrCVs/UzTIUzFPbohA3rAcxNuSlk4R8BVnO5Sl2 hxoDAkUrzyzmUGJw9K1yjjzd9T3xRAWYcs4ad1/Ohn9DVDhQPfGvYSelaGmBVuNbcdrE vzIA== X-Gm-Message-State: AEkoouuP3olcGMuxLCx3+fz0OBp+U5OX8IByE4xfOzXazr8qbCEoP/OHB2/qC4+/ZWBi7kNuzk1DLC8P0NREekZJ X-Received: by 10.159.55.199 with SMTP id q65mr48504899uaq.125.1470688470586; Mon, 08 Aug 2016 13:34:30 -0700 (PDT) MIME-Version: 1.0 Received: by 10.31.141.8 with HTTP; Mon, 8 Aug 2016 13:33:49 -0700 (PDT) In-Reply-To: <7635a6d6-4059-6b23-952c-a88dbfef3b18@redhat.com> References: <047d7b5dbb865204bd052cf0bc2b@google.com> <2026a39c-0b53-9142-74ce-091bc73832d8@redhat.com> <7635a6d6-4059-6b23-952c-a88dbfef3b18@redhat.com> From: Doug Evans Date: Mon, 08 Aug 2016 20:34:00 -0000 Message-ID: Subject: Re: [PATCH 4/5]: Enhancements to "flags": i386 cleanup To: Pedro Alves Cc: gdb-patches , Wei-cheng Wang Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg00107.txt.bz2 On Mon, Aug 8, 2016 at 8:06 AM, Pedro Alves wrote: > 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: > > 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"); > > > 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 > Hi. Sorry for the trouble. I think(!) these two patches fix things. Basically, I made "end" required again, and made single bit fields default to bool, and then tweaked a couple of xml files to minimize changes. https://sourceware.org/ml/gdb-patches/2016-08/msg00105.html https://sourceware.org/ml/gdb-patches/2016-08/msg00106.html It occurs to me I need to update the docs too. Additional patch to follow.