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

[Shahab Vahedi <shahab.vahedi@gmail.com>]

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