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

[Simon Marchi <simark@simark.ca>]

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