Re: [PATCH v3 1/3] arc: Add ARCv2 XML target along with refactoring

Simon Marchi
2020-07-22T14:54:29Z

On 2020-07-22 10:33 a.m., Shahab Vahedi wrote:
> On Wed, Jul 22, 2020 at 09:49:48AM -0400, Simon Marchi wrote:
>> On 2020-07-22 9:36 a.m., Shahab Vahedi wrote:
>>> On Thu, Jul 16, 2020 at 09:28:50AM -0400, Simon Marchi wrote:
>>>> On 2020-07-15 4:35 p.m., Shahab Vahedi wrote:
>>>> When I search for `arc_gdbarch_features_init` in that patch I don't find anything...
>>>
>>> You're correct. I have it in my local branch. The patch that uses it is going
>>> to be submitted very soon. I hope you don't mind if it remains like this.
>>
>> Let's just make it static in this patch, and make in non-static again in your future
>> patch, it's not a big change.
>
> Will do.
>>
>>>> The code in GDB constructing an arc_gdbarch_features will use the BFD to determine
>>>> these two values, whereas the code in GDBserver will use something else (usually
>>>> looking at the current process' properties).
>>>>
>>>> In fact, the constructor is optional, you could just build a arc_gdbarch_features
>>>> using aggregate initialization and return it from that function:
>>>>
>>>> arc_gdbarch_features features {reg_size, isa};
>>>>
>>>> It doesn't really matter. I just happen to prefer the constructor method, because
>>>> that makes it so you can't "forget" a field and it ensures it can never be in an
>>>> uninitialized state.
>>>
>>> I see now. Actually, I have usecases to not initialize it immediately,
>>> but in a few lines of code.
>>
>> I'd be curious to see. Because instead of declaring it immediately, you can keep
>> the fields separate until you initialize it:
>>
>> int reg_size;
>> enum arc_isa isa;
>>
>> if (something)
>> {
>> reg_size = 8;
>> isa = foo;
>> }
>> else
>> {
>> reg_size = 4;
>> isa = bar;
>> }
>>
>> arc_gdbarch_features features (reg_size, isa);
>>
>> I think this is good, because the day you add a third axis to arc_gdbarch_features,
>> that code will not build and you'll be forced to updated it (you can't forget it).
>>
>> Whereas with:
>>
>> arc_gdbarch_features features;
>>
>> if (something)
>> {
>> features.reg_size = 8;
>> features.isa = foo;
>> }
>> else
>> {
>> features.reg_size = 4;
>> features.isa = bar;
>> }
>>
>> It's possible to forget.
>>
>> But maybe you have a different use case in mind?
>
> These are the 2 usecases I have:
>
> gdb/arc-tdep.c
> --------------
> static bool
> arc_tdesc_init (struct gdbarch_info info, ...)
> {
> const struct target_desc *tdesc_loc = info.target_desc;
>
> if (!tdesc_has_registers (tdesc_loc))
> {
> arc_gdbarch_features features;
> arc_gdbarch_features_init (features, info.abfd,
> info.bfd_arch_info->mach);
> tdesc_loc = arc_lookup_target_description (features);
> }
> ...
> }
>
> gdb/arc-linux-tdep.c
> --------------------
> static const struct target_desc *
> arc_linux_core_read_description (struct gdbarch *gdbarch,
> struct target_ops *target,
> bfd *abfd)
> {
> arc_gdbarch_features features;
> arc_gdbarch_features_init (features, abfd,
> gdbarch_bfd_arch_info (gdbarch)->mach);
> return arc_lookup_target_description (features);
> }

I mentioned earlier that arc_gdbarch_features_init should return a
`arc_gdbarch_features` by value, that would avoid this problem (and
be cleaner, IMO).

> While I totally understand your point, I don't want to unroll the
> logic of "arc_gdbarch_features_init ()" to the same level that
> "features" is declared. Ideally, I should have a constructor
> with the same signature as "arc_gdbarch_features_init ()", but
> that is not possible because compilation of gdbserver will go
> awry when there is mention of ELF data in "gdb/arch/arc.h".
>
> To have a complete overview, in a soon-to-be-submitted patch
> you're going to see that "features" is initialized like:
>
> gdbserver/linux-arc-low.cc
> --------------------------
> static const struct target_desc *
> arc_linux_read_description (void)
> {
> struct target_desc *tdesc;
> arc_gdbarch_features features = {4, ARC_ISA_NONE};
> #ifdef __ARC700__
> features.isa = ARC_ISA_ARCV1;
> #else
> features.isa = ARC_ISA_ARCV2;
> #endif
> tdesc = arc_create_target_description (features);
> ...
> }

You can easily apply the suggestion from my previous message here.

Simon