Re: [PATCH 4/5]: Enhancements to "flags": i386 cleanup

[Doug Evans <dje at google dot com>]

On Mon, Aug 8, 2016 at 8:06 AM, Pedro Alves <palves@redhat.com> 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 <palves@redhat.com> 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:
>
>  -    <field name="CF" start="0"/>
>  +    <field name="CF" start="0" end="0"/>
>
> etc., we now get:
>
>   rip            0x4005ea        0x4005ea <terminal_func+4>
>  -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.