Re: [PATCH] arc: Add support for Linux coredump files

[Simon Marchi <simark@simark.ca>]

On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote:
> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
>
> With the implemenations in this patch, ARC gdb can handle
> coredump related matters.  The binutils counter part of
> this patch has already been pushed [1].
>
> [1] arc: Add support for ARC HS extra registers in core files
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2745674244d6aecddcf636475034bdb9c0a6b4a0

Hi Shahab,

I noted a few comments, the patch is OK to push with these fixed.  I
think this would be relatively safe to push to the gdb-10-branch branch
as well if you want, it's all new features, it doesn't touch existing
features.

> @@ -227,6 +281,142 @@ arc_linux_skip_solib_resolver (struct gdbarch *gdbarch, CORE_ADDR pc)
>      }
>  }
>
> +void
> +arc_linux_supply_gregset (const struct regset *regset,
> +			  struct regcache *regcache,
> +			  int regnum, const void *gregs, size_t size)
> +{
> +  gdb_static_assert (ARC_LAST_REGNUM
> +		     <= ARRAY_SIZE (arc_linux_core_reg_offsets));

Can you explain why this is <= and not < ?  I'm not saying it's wrong,
but it looks unusual.  If ARC_LAST_REGNUM was 2 (meaning there are 3
registers: 0, 1 and 2), then having an offsets array with size 2 would
be bad, would it?  Or maybe I'm just confused.


> +
> +  const bfd_byte *buf = (const bfd_byte *) gregs;
> +
> +  for (int reg = 0; reg <= ARC_LAST_REGNUM; reg++)
> +    {
> +      if (arc_linux_core_reg_offsets[reg] != ARC_OFFSET_NO_REGISTER)
> +	regcache->raw_supply (reg, buf + arc_linux_core_reg_offsets[reg]);
> +    }
> +}
> +
> +void
> +arc_linux_supply_v2_regset (const struct regset *regset,
> +			    struct regcache *regcache, int regnum,
> +			    const void *v2_regs, size_t size)
> +{
> +  const bfd_byte *buf = (const bfd_byte *) v2_regs;
> +
> +  /* user_regs_arcv2 is defined in linux arch/arc/include/uapi/asm/ptrace.h.  */
> +  regcache->raw_supply (ARC_R30_REGNUM, buf);
> +  regcache->raw_supply (ARC_R58_REGNUM, buf + REGOFF (1));
> +  regcache->raw_supply (ARC_R59_REGNUM, buf + REGOFF (2));
> +}
> +
> +/* Populate BUF with register REGNUM from the REGCACHE.  */
> +
> +static void
> +collect_register(const struct regcache *regcache, struct gdbarch *gdbarch,

Space before parenthesis.

> +		 int regnum, gdb_byte *buf)
> +{
> +  /* Skip non-existing registers.  */
> +  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
> +    return;
> +
> +  /* The Address where the execution has stopped is in pseudo-register

Address -> address

> +     STOP_PC.  However, when kernel code is returning from the exception,
> +     it uses the value from ERET register.  Since, TRAP_S (the breakpoint
> +     instruction) commits, the ERET points to the next instruction.  In
> +     other words: ERET != STOP_PC.  To jump back from the kernel code to
> +     the correct address, ERET must be overwritten by GDB's STOP_PC.  Else,
> +     the program will continue at the address after the current instruction.
> +     */
> +  if (regnum == gdbarch_pc_regnum (gdbarch))
> +    {
> +      int eret_offset = REGOFF (6);

That REGOFF (6) looks a bit magical.  Defining a constant would be
clearer.

> +/* Implement the `core_read_description` gdbarch method.  */
> +
> +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_create (abfd,
> +			           gdbarch_bfd_arch_info (gdbarch)->mach);

8 spaces -> tab

Simon