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

  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:



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
  

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.
  

On 08/08/2016 09:33 PM, Doug Evans wrote:

> 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.

Thanks!  All looks good to me.
  

On 08/09/2016 06:55 PM, Pedro Alves wrote:

> Thanks!  All looks good to me.

A thought has been running through my mind though.

Is the "type" attribute used for anything in <flags> elements?
Does changing the type of a flag bitfield have any user-visible
effect at all?  I.e., does a 1-bit uint32_t bitfield flag
print differently from a bool bitfield flag?

Wondering if we could remove the bool-special case, because
before 8151645076ce927e0ee866c598a19f192e68e103, we used to say,
for <struct>:

                            There are two forms of the @samp{<struct>}
 element; a @samp{<struct>} element must either contain only bitfields
 or contain no bitfields.  If the structure contains only bitfields,
 its total size in bytes must be specified, each bitfield must have an
 explicit start and end, and bitfields are automatically assigned an
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 integer type.  The field's @var{start} should be less than or
 ^^^^^^^^^^^^
 equal to its @var{end}, and zero represents the least significant bit.

And for flags, we used to say:

 @cindex <flags>
 If a register's value is a series of single-bit flags, define it with
 a flags type.  The @samp{<flags>} element has an explicit @var{size}
 and contains one or more @samp{<field>} elements.  Each field has a
 @var{name}, a @var{start}, and an @var{end}.  Only single-bit flags
 are supported.

I think I may be very confused by this all, though.

Thanks,
Pedro Alves
  

On Thu, Aug 11, 2016 at 11:10 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/09/2016 06:55 PM, Pedro Alves wrote:
>
>> Thanks!  All looks good to me.
>
> A thought has been running through my mind though.
>
> Is the "type" attribute used for anything in <flags> elements?
> Does changing the type of a flag bitfield have any user-visible
> effect at all?  I.e., does a 1-bit uint32_t bitfield flag
> print differently from a bool bitfield flag?

There may be some simplification possible, but note that bools do
print differently: if false we don't print the field at all.

[digression: I'm not sure it's possible today, but while I understand
the desire to avoid the extra verbosity if we always printed false
fields, there are times when I *do* want the field printed even if
false]

>
> Wondering if we could remove the bool-special case, because
> before 8151645076ce927e0ee866c598a19f192e68e103, we used to say,
> for <struct>:
>
>                             There are two forms of the @samp{<struct>}
>  element; a @samp{<struct>} element must either contain only bitfields
>  or contain no bitfields.  If the structure contains only bitfields,
>  its total size in bytes must be specified, each bitfield must have an
>  explicit start and end, and bitfields are automatically assigned an
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  integer type.  The field's @var{start} should be less than or
>  ^^^^^^^^^^^^
>  equal to its @var{end}, and zero represents the least significant bit.
>
> And for flags, we used to say:
>
>  @cindex <flags>
>  If a register's value is a series of single-bit flags, define it with
>  a flags type.  The @samp{<flags>} element has an explicit @var{size}
>  and contains one or more @samp{<field>} elements.  Each field has a
>  @var{name}, a @var{start}, and an @var{end}.  Only single-bit flags
>  are supported.
>
> I think I may be very confused by this all, though.
>
> Thanks,
> Pedro Alves
>