Re: [PATCH v5 1/8] Commonise tdesc_reg

[Alan Hayward <Alan.Hayward@arm.com>]

> On 18 Apr 2018, at 02:57, Simon Marchi <simark@simark.ca> wrote:
> 
> On 2018-04-10 10:33 AM, Alan Hayward wrote:
>> This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.
>> 
>> gdbserver tdesc_create_reg is changed to create a tdesc_reg instead of
>> a reg_defs entry. The vector of tdesc_reg is held inside tdesc_feature.
>> 
>> However, other modules in gdbserver directly access the reg_defs structure.
>> To work around this, init_target_desc fills in reg_defs by parsing the
>> tdesc_reg vector.
>> The long term goal is to remove reg_defs, replacing with accessor funcs.
>> 
>> I wanted to make tdesc_create_reg common, but I cannot do that until
>> the next patch.
>> 
>> I also had to commonise tdesc_element_visitor and tdesc_element.
>> 
>> This patch only differs from the V4 version in init_target_desc() and
>> the changing of constructors for regdef.
> 
> Hi Alan,
> 
> Just two small comment, but the patch LGTM with those answered or addressed.
> 
>> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
>> index 85139d948c..8eb88eedce 100644
>> --- a/gdb/gdbserver/tdesc.h
>> +++ b/gdb/gdbserver/tdesc.h
>> @@ -25,7 +25,10 @@
>> #include <vector>
>> 
>> struct tdesc_feature
>> -{};
>> +{
>> +  /* The registers associated with this feature.  */
>> +  std::vector<tdesc_reg_up> registers;
>> +};
>> 
>> /* A target description.  Inherit from tdesc_feature so that target_desc
>>    can be used as tdesc_feature.  */
>> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
>> index 4775e863e9..7c80d45d48 100644
>> --- a/gdb/regformats/regdef.h
>> +++ b/gdb/regformats/regdef.h
>> @@ -21,15 +21,15 @@
>> 
>> struct reg
>> {
>> -  reg ()
>> +  reg (int _offset)
>>     : name (""),
>> -      offset (0),
>> +      offset (_offset),
>>       size (0)
>>   {}
> 
> If this constructor is only used for padding entries, shouldn't name be
> NULL, as the documentation for the field states?
> 

That creates two issues:

The reg:operator== segfaults in the strcmp due to the null.
Easily fixable, but a little ugly.

With that patched, the gdbserver unit tests will fail.
That is because the creation of target descriptions in the  -generated.c files
in the build dir create the gaps using:
tdesc_create_reg (feature, “", 0, 0, NULL, 0, NULL);

Would fixing that cause more issues? Not sure.

Having “” rather than 0 does make sense when you format these to xml files.
We are explicitly setting to a blank string. Whereas null is more for uninitialised.

I was going to suggest fixing in a follow on patch, but the more I think about it,
The more I’m thinking “” is correct.

> Also, did you notice something failing if the padding entries don't have
> the offset field to the "current" offset at the time they are created?
> If we could leave them at 0, I think it would keep things simpler.  I
> stopped for a few seconds, wondering why init_target_desc did:
> 
>  tdesc->reg_defs.resize (regnum, reg (offset));
> 
> and not just:
> 
>  tdesc->reg_defs.resize (regnum);

Again, this creates unit test failures due to the same tdesc_create_reg above.
Here offset is a little more explicit, and setting to 0 says it’s at the start of the
description, which isn’t right.

Thanks again for the reviews.

Alan.