This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v5 1/8] Commonise tdesc_reg
> 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.