Re: [PATCH v5 1/8] Commonise tdesc_reg

[Simon Marchi <simark@simark.ca>]

On 2018-04-18 05:03, Alan Hayward wrote:
>> 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.

The important thing is that documentation is accurate, so could you 
adjust the doc of the name field accordingly?

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

Ok.

Thanks,

Simon