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

Shahab Vahedi
2020-07-22T14:33:44Z

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);
}

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);
...
}

Cheers,
Shahab