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

[Pedro Alves <palves@redhat.com>]

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