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

[Simon Marchi <simark@simark.ca>]

Hi Shahab,

The patch LGTM with the nits below fixed.

On 2020-09-29 12:15 p.m., Shahab Vahedi wrote:
> +/* arc_linux_core_reg_offsets[i] is the offset in the .reg section of GDB
> +   regnum i.  Array index is an internal GDB register number, as defined in
> +   arc-tdep.h:arc_regnum.
> +
> +   From include/uapi/asm/ptrace.h in the ARC Linux sources.  */

Thanks for this, I _love_ it when people point to references that help
make sense of the code.

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

I might have mislead you in the past by telling you to use curly braces
in this situation before.  I recently learned that it's not necessary.
You can do:

  for (...)
    if (...)
      something ();

It's only needed for nested ifs:

  if (...)
    {
      if (...)
        something ();
    }

to avoid the problem of the dangling else.

> +void
> +arc_linux_collect_gregset (const struct regset *regset,
> +			   const struct regcache *regcache,
> +			   int regnum, void *gregs, size_t size)
> +{
> +  gdb_static_assert (ARC_LAST_REGNUM
> +		     < ARRAY_SIZE (arc_linux_core_reg_offsets));
> +
> +  gdb_byte *buf = (gdb_byte *) gregs;
> +  struct gdbarch *gdbarch = regcache->arch ();
> +
> +  /* regnum == -1 means writing all the registers.  */
> +  if (regnum == -1)
> +    for (int reg = 0; reg <= ARC_LAST_REGNUM; reg++)
> +      collect_register (regcache, gdbarch, reg, buf);
> +  else if (regnum <= ARC_LAST_REGNUM)
> +    collect_register (regcache, gdbarch, regnum, buf);
> +  else
> +    gdb_assert (!"Invalid regnum in arc_linux_collect_gregset.");

You can use gdb_assert_not_reached exactly for this.

Simon