Re: [PATCH] gdb:csky using csky_gdbarch_tdep in csky_gdbarch_init

[Simon Marchi <simark@simark.ca>]

On 2022-03-22 07:52, Jiangshuai Li wrote:
> add three points:
> 1. add vars to csky_gdbarch_tdep
> 2. save mach, fpu_abi, vdsp_verion and fpu_hardfp from info.abfd
> 3. find a pre-declared gdbarch via point 2
>
> fpu_abi and vdsp_verion will be used by gdbarch_return_value
> and gdbarch_push_dummy_call later.
>
> gdb/
> 	*csky-tdep.h: add vars to csky_gdbarch_tdep
> 	*csky-tdep.c: save msgs from info.abfd and use them
> 	to find a pre-declared gdbarch

Don't need a ChangeLog entry for GDB.

The subject of the patch:

  csky using csky_gdbarch_tdep in csky_gdbarch_init

is not clear.  What behavior change do you want to introduce here?

> ---
>  gdb/csky-tdep.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++--
>  gdb/csky-tdep.h | 17 +++++++++-
>  2 files changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/csky-tdep.c b/gdb/csky-tdep.c
> index cba0065fa53..44c2b56fba4 100644
> --- a/gdb/csky-tdep.c
> +++ b/gdb/csky-tdep.c
> @@ -2157,16 +2157,97 @@ static struct gdbarch *
>  csky_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
>    struct gdbarch *gdbarch;
> +  /* Analysis info.abfd.  */

Analysis -> Analyze ?

> +  unsigned int mach;
> +  int mach_find = 0;
> +  unsigned long abfd_e_flags;
> +  unsigned int fpu_abi = 0;
> +  unsigned int vdsp_version = 0;
> +  unsigned int fpu_hardfp = 0;
> +  static bfd_arch_info_type bfd_csky_arch_t;

What's the purpose of this variable being static?

> +
> +  /* When the type of bfd file is srec(or any files are not elf),
> +     the E_FLAGS will be not credible.  */
> +  if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)

info.abfd != nullptr

> +    {
> +      /*  Get e_flags form abfd.  */
> +      abfd_e_flags = elf_elfheader (info.abfd)->e_flags;
> +      if (IS_CSKY_V2 (abfd_e_flags))
> +        {
> +          switch (abfd_e_flags & CSKY_ARCH_MASK)

Please correct the indentation to fit the coding style (tabs + spaces).

> +            {
> +            case CSKY_ARCH_801:
> +            case CSKY_ARCH_802:
> +            case CSKY_ARCH_803:
> +            case CSKY_ARCH_807:
> +            case CSKY_ARCH_810:
> +            case CSKY_ARCH_860:
> +              mach = abfd_e_flags & CSKY_ARCH_MASK;
> +              mach_find = 1;

It's not clear why you need to do this here.  Selecting the right
machine type based on flags is normally the kind of things done in BFD.
So by the time it reaches GDB, the correct bfd_arch_info should have
already been selected by BFD.  For example, if the ELF file is a
CSKY_ARCH_803, then info.abfd->arch_info should already point to
bfd_mach_ck803.

> +              break;
> +            default:
> +              mach = CSKY_ARCH_810;
> +              break;
> +            }
> +        }
> +      else
> +        {
> +          mach = CSKY_ARCH_810;
> +        }
> +
> +      /* Get FPU, VDSP build options.  */
> +      fpu_abi = bfd_elf_get_obj_attr_int (info.abfd,
> +                                          OBJ_ATTR_PROC,
> +                                          Tag_CSKY_FPU_ABI);
> +      vdsp_version = bfd_elf_get_obj_attr_int (info.abfd,
> +                                               OBJ_ATTR_PROC,
> +                                               Tag_CSKY_VDSP_VERSION);
> +      fpu_hardfp = bfd_elf_get_obj_attr_int (info.abfd,
> +                                             OBJ_ATTR_PROC,
> +                                             Tag_CSKY_FPU_HARDFP);
> +    }
> +  else
> +    {
> +      mach = CSKY_ARCH_810;
> +    }
> +
> +  /* Set info.bfd_arch_info.  */
> +  memcpy (&bfd_csky_arch_t, info.bfd_arch_info, sizeof (bfd_csky_arch_t));
> +  if (mach_find)
> +    bfd_csky_arch_t.mach = abfd_e_flags;
> +  else
> +    bfd_csky_arch_t.mach = mach | CSKY_ABI_V2;
> +  info.bfd_arch_info = &bfd_csky_arch_t;
>
>    /* Find a candidate among the list of pre-declared architectures.  */
> -  arches = gdbarch_list_lookup_by_info (arches, &info);
> -  if (arches != NULL)
> -    return arches->gdbarch;
> +  for (arches = gdbarch_list_lookup_by_info (arches, &info);
> +       arches != NULL;
> +       arches = gdbarch_list_lookup_by_info (arches->next, &info))
> +    {
> +      csky_gdbarch_tdep *tdep
> +        = (csky_gdbarch_tdep *) gdbarch_tdep (arches->gdbarch);
> +      if (mach != tdep->mach)
> +        continue;

As explained above, this filtering on mach should not be necessary: two
ELF files with different machine values (in their e_flags) should lead
to two abfds with different arch infos.  And gdbarch_list_lookup_by_info
will automatically filters out those with different arch infos.

It's not clear what problem you are trying to solve, maybe if try to
explain that first it will be easier to understand what you are trying
to achieve.

Simon