[PATCH] arc: Add support for Linux coredump files

[PATCH] arc: Add support for Linux coredump files

[Shahab Vahedi]

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

gdb/ChangeLog:

Anton Kolesov  <anton.kolesov@synopsys.com>

	* arc-linux-tdep.h: New file.
	* arc-linux-tdep.c (arc_linux_core_reg_offsets,
	arc_linux_supply_gregset, arc_linux_supply_v2_regset,
	arc_linux_collect_gregset, arc_linux_collect_v2_regset,
	arc_linux_gregset, arc_linux_v2_regset,
	arc_linux_iterate_over_regset_sections,
	arc_linux_core_read_description): Implement.
	(arc_linux_init_osabi): Set iterate_over_regset_sections.
	* arc-tdep.c (ARC_OFFSET_NO_REGISTER): Declare.
---
 gdb/arc-linux-tdep.c | 193 +++++++++++++++++++++++++++++++++++++++++++
 gdb/arc-linux-tdep.h |  52 ++++++++++++
 gdb/arc-tdep.c       |   5 +-
 gdb/arc-tdep.h       |   8 ++
 4 files changed, 255 insertions(+), 3 deletions(-)
 create mode 100644 gdb/arc-linux-tdep.h

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 36f32459bbe..8dcd9785458 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -27,7 +27,61 @@
 
 /* ARC header files.  */
 #include "opcodes/arc-dis.h"
+#include "arc-linux-tdep.h"
 #include "arc-tdep.h"
+#include "arch/arc.h"
+
+#define REGOFF(offset) (offset * ARC_REGISTER_SIZE)
+
+/* 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.  */
+
+static const int arc_linux_core_reg_offsets[] = {
+  /* R0 - R12.  */
+  REGOFF (22), REGOFF (21), REGOFF (20), REGOFF (19),
+  REGOFF (18), REGOFF (17), REGOFF (16), REGOFF (15),
+  REGOFF (14), REGOFF (13), REGOFF (12), REGOFF (11),
+  REGOFF (10),
+
+  /* R13 - R25.  */
+  REGOFF (37), REGOFF (36), REGOFF (35), REGOFF (34),
+  REGOFF (33), REGOFF (32), REGOFF (31), REGOFF (30),
+  REGOFF (29), REGOFF (28), REGOFF (27), REGOFF (26),
+  REGOFF (25),
+
+  REGOFF (9),			/* R26 (GP) */
+  REGOFF (8),			/* FP */
+  REGOFF (23),			/* SP */
+  ARC_OFFSET_NO_REGISTER,	/* ILINK */
+  ARC_OFFSET_NO_REGISTER,	/* R30 */
+  REGOFF (7),			/* BLINK */
+
+  /* R32 - R59.  */
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER,
+
+  REGOFF (4),			/* LP_COUNT */
+  ARC_OFFSET_NO_REGISTER,	/* RESERVED */
+  ARC_OFFSET_NO_REGISTER,	/* LIMM */
+  ARC_OFFSET_NO_REGISTER,	/* PCL */
+
+  REGOFF (39),			/* PC  */
+  REGOFF (5),			/* STATUS32 */
+  REGOFF (2),			/* LP_START */
+  REGOFF (3),			/* LP_END */
+  REGOFF (1),			/* BTA */
+};
 
 /* Implement the "cannot_fetch_register" gdbarch method.  */
 
@@ -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));
+
+  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,
+		 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
+     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);
+      regcache->raw_collect (regnum, buf + eret_offset);
+    }
+  else
+    regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+}
+
+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.");
+}
+
+void
+arc_linux_collect_v2_regset (const struct regset *regset,
+			     const struct regcache *regcache, int regnum,
+			     void *v2_regs, size_t size)
+{
+  bfd_byte *buf = (bfd_byte *) v2_regs;
+
+  regcache->raw_collect (ARC_R30_REGNUM, buf);
+  regcache->raw_collect (ARC_R58_REGNUM, buf + REGOFF (1));
+  regcache->raw_collect (ARC_R59_REGNUM, buf + REGOFF (2));
+}
+
+/* Linux regset definitions.  */
+
+static const struct regset arc_linux_gregset = {
+  arc_linux_core_reg_offsets,
+  arc_linux_supply_gregset,
+  arc_linux_collect_gregset,
+};
+
+static const struct regset arc_linux_v2_regset = {
+  NULL,
+  arc_linux_supply_v2_regset,
+  arc_linux_collect_v2_regset,
+};
+
+/* Implement the `iterate_over_regset_sections` gdbarch method.  */
+
+static void
+arc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
+					iterate_over_regset_sections_cb *cb,
+					void *cb_data,
+					const struct regcache *regcache)
+{
+  /* There are 40 registers in Linux user_regs_struct, although some of
+     them are now just a mere paddings, kept to maintain binary
+     compatibility with older tools.  */
+  const int sizeof_gregset = 40 * ARC_REGISTER_SIZE;
+
+  cb (".reg", sizeof_gregset, sizeof_gregset, &arc_linux_gregset, NULL,
+      cb_data);
+  cb (".reg-arc-v2", ARC_LINUX_SIZEOF_V2_REGSET, ARC_LINUX_SIZEOF_V2_REGSET,
+      &arc_linux_v2_regset, NULL, cb_data);
+}
+
+/* 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);
+  return arc_lookup_target_description (features);
+}
+
 /* Initialization specific to Linux environment.  */
 
 static void
@@ -260,6 +450,9 @@ arc_linux_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_software_single_step (gdbarch, arc_linux_software_single_step);
   set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);
   set_gdbarch_skip_solib_resolver (gdbarch, arc_linux_skip_solib_resolver);
+  set_gdbarch_iterate_over_regset_sections
+    (gdbarch, arc_linux_iterate_over_regset_sections);
+  set_gdbarch_core_read_description (gdbarch, arc_linux_core_read_description);
 
   /* GNU/Linux uses SVR4-style shared libraries, with 32-bit ints, longs
      and pointers (ILP32).  */
diff --git a/gdb/arc-linux-tdep.h b/gdb/arc-linux-tdep.h
new file mode 100644
index 00000000000..c26cacd7029
--- /dev/null
+++ b/gdb/arc-linux-tdep.h
@@ -0,0 +1,52 @@
+/* Target dependent code for GNU/Linux ARC.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef ARC_LINUX_TDEP_H
+#define ARC_LINUX_TDEP_H
+
+#include "gdbarch.h"
+#include "regset.h"
+
+#define ARC_LINUX_SIZEOF_V2_REGSET (3 * ARC_REGISTER_SIZE)
+
+/* Reads registers from the NT_PRSTATUS data array into the regcache.  */
+
+void arc_linux_supply_gregset (const struct regset *regset,
+			       struct regcache *regcache, int regnum,
+			       const void *gregs, size_t size);
+
+/* Reads regsiters from the NT_ARC_V2 data array into the regcache.  */
+
+void arc_linux_supply_v2_regset (const struct regset *regset,
+				 struct regcache *regcache, int regnum,
+				 const void *v2_regs, size_t size);
+
+/* Writes registers from the regcache into the NT_PRSTATUS data array.  */
+
+void arc_linux_collect_gregset (const struct regset *regset,
+				const struct regcache *regcache,
+				int regnum, void *gregs, size_t size);
+
+/* Writes registers from the regcache into the NT_ARC_V2 data array.  */
+
+void arc_linux_collect_v2_regset (const struct regset *regset,
+				  const struct regcache *regcache,
+				  int regnum, void *v2_regs, size_t size);
+
+#endif /* ARC_LINUX_TDEP_H */
diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 6f544bff78d..91e106a0167 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -1883,10 +1883,9 @@ mach_type_to_arc_isa (const unsigned long mach)
     }
 }
 
-/* Common construction code for ARC_GDBARCH_FEATURES struct.  If there
-   is no ABFD, then a FEATURE with default values is returned.  */
+/* See arc-tdep.h.  */
 
-static arc_gdbarch_features
+arc_gdbarch_features
 arc_gdbarch_features_create (const bfd *abfd, const unsigned long mach)
 {
   /* Use 4 as a fallback value.  */
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 6331d29f402..d8a0c42909d 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -105,6 +105,9 @@ enum arc_regnum
 /* STATUS32 register: current instruction is a delay slot.  */
 #define ARC_STATUS32_DE_MASK (1 << 6)
 
+/* Special value for register offset arrays.  */
+#define ARC_OFFSET_NO_REGISTER (-1)
+
 #define arc_print(fmt, args...) fprintf_unfiltered (gdb_stdlog, fmt, ##args)
 
 extern int arc_debug;
@@ -182,4 +185,9 @@ CORE_ADDR arc_insn_get_branch_target (const struct arc_instruction &insn);
 
 CORE_ADDR arc_insn_get_linear_next_pc (const struct arc_instruction &insn);
 
+/* Create an arc_gdbarch_features instance from the provided data.  */
+
+arc_gdbarch_features arc_gdbarch_features_create (const bfd *abfd,
+						  const unsigned long mach);
+
 #endif /* ARC_TDEP_H */
-- 
2.28.0

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

[Shahab Vahedi]

Just a small reminder for reviwing the patch [1].


Cheers,
Shahab

[1] arc: Add support for Linux coredump files
https://sourceware.org/pipermail/gdb-patches/2020-August/171515.html

[PING^2][PATCH] arc: Add support for Linux coredump files

[Shahab Vahedi]

A gentle reminder for reviewing "arc: Add support for Linux
coredump files" [1].

[1]
https://sourceware.org/pipermail/gdb-patches/2020-August/171515.html 

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

[Simon Marchi]

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

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

[Aktemur, Tankut Baris]

> On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote:
> +bool
> +arc_target::low_breakpoint_at (CORE_ADDR where)
> +{
> +  uint16_t insn;
> +  uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);
> +
> +  the_target->read_memory (where, (gdb_byte *) &insn, TRAP_S_1_SIZE);

Because 'the_target' is the same as 'this', you may want to simply call
'read_memory' without explicitly stating the receiver object.  This way,
accessing the global variable could be avoided.

> +  return (insn == breakpoint);
> +}

Regards
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Shahab Vahedi]

Hi Baris,

On Thu, Sep 17, 2020 at 11:55:42AM +0000, Aktemur, Tankut Baris wrote:
> > On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote:
> > +bool
> > +arc_target::low_breakpoint_at (CORE_ADDR where)
> > +{
> > +  uint16_t insn;
> > +  uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);
> > +
> > +  the_target->read_memory (where, (gdb_byte *) &insn, TRAP_S_1_SIZE);
> 
> Because 'the_target' is the same as 'this', you may want to simply call
> 'read_memory' without explicitly stating the receiver object.  This way,
> accessing the global variable could be avoided.

Thank you very much for your input, but since it is not very obvious to
me where/when the "the_target" is initialized, I'd rather keep it this
way. Specially seeing that other targets (ARM and RISCV) doing the same.


Cheers,
Shahab

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

[Aktemur, Tankut Baris]

Hi Shahab,

On Monday, September 28, 2020 3:47 PM, Shahab Vahedi wrote:
> Hi Baris,
> 
> On Thu, Sep 17, 2020 at 11:55:42AM +0000, Aktemur, Tankut Baris wrote:
> > > On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote:
> > > +bool
> > > +arc_target::low_breakpoint_at (CORE_ADDR where)
> > > +{
> > > +  uint16_t insn;
> > > +  uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);
> > > +
> > > +  the_target->read_memory (where, (gdb_byte *) &insn, TRAP_S_1_SIZE);
> >
> > Because 'the_target' is the same as 'this', you may want to simply call
> > 'read_memory' without explicitly stating the receiver object.  This way,
> > accessing the global variable could be avoided.
> 
> Thank you very much for your input, but since it is not very obvious to
> me where/when the "the_target" is initialized, I'd rather keep it this
> way. Specially seeing that other targets (ARM and RISCV) doing the same.
> 
> 
> Cheers,
> Shahab

Just for the record, there are similar uses in linux-aarch32-low.cc,
linux-arm-low.cc, linux-low.cc, linux-sparc-low.cc, and win32-low.cc (I don't
see it in linux-riscv-low.cc), and in all of these uses the context is a function
(not a method) where the 'this' pointer is not available.

Regards
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Simon Marchi]

On 2020-09-28 10:08 a.m., Aktemur, Tankut Baris wrote:
> Hi Shahab,
>
> On Monday, September 28, 2020 3:47 PM, Shahab Vahedi wrote:
>> Hi Baris,
>>
>> On Thu, Sep 17, 2020 at 11:55:42AM +0000, Aktemur, Tankut Baris wrote:
>>>> On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote:
>>>> +bool
>>>> +arc_target::low_breakpoint_at (CORE_ADDR where)
>>>> +{
>>>> +  uint16_t insn;
>>>> +  uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);
>>>> +
>>>> +  the_target->read_memory (where, (gdb_byte *) &insn, TRAP_S_1_SIZE);
>>>
>>> Because 'the_target' is the same as 'this', you may want to simply call
>>> 'read_memory' without explicitly stating the receiver object.  This way,
>>> accessing the global variable could be avoided.
>>
>> Thank you very much for your input, but since it is not very obvious to
>> me where/when the "the_target" is initialized, I'd rather keep it this
>> way. Specially seeing that other targets (ARM and RISCV) doing the same.
>>
>>
>> Cheers,
>> Shahab
>
> Just for the record, there are similar uses in linux-aarch32-low.cc,
> linux-arm-low.cc, linux-low.cc, linux-sparc-low.cc, and win32-low.cc (I don't
> see it in linux-riscv-low.cc), and in all of these uses the context is a function
> (not a method) where the 'this' pointer is not available.
>
> Regards
> -Baris

FWIW, I'm leaning towards what Baris says, better use "this" than the
global variable.  And I even prefer to make it explicit,
"this->read_memory (...", to make it clear that we call a method and not
a free function.  But I don't want to start an endless discussion about
coding style :).

Simon

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

[Shahab Vahedi]

Hi Baris, Simon,

On Mon, Sep 28, 2020 at 03:10:23PM -0400, Simon Marchi wrote:
> On 2020-09-28 10:08 a.m., Aktemur, Tankut Baris wrote:
> >
> > Just for the record, there are similar uses in linux-aarch32-low.cc,
> > linux-arm-low.cc, linux-low.cc, linux-sparc-low.cc, and win32-low.cc (I don't
> > see it in linux-riscv-low.cc), and in all of these uses the context is a function
> > (not a method) where the 'this' pointer is not available.

I've looked into those files in master branch [1] which at the time
was at commit 9aed480c3a7. The relevant results are:

--------
==> linux-aarch32-low.relevant_grep_results <==
      the_target->read_memory (where, (unsigned char *) &insn, 2);
	  the_target->read_memory (where + 2, (unsigned char *) &insn, 2);
      the_target->read_memory (where, (unsigned char *) &insn, 4);
      if (target_read_memory (*pcptr, buf, 2) == 0)
	  target_read_memory (*pcptr, (gdb_byte *) &inst1, 2);

==> linux-arm-low.relevant_grep_results <==
  target_read_memory (memaddr, (unsigned char *) &res, len);
  the_target->read_memory (sp, (unsigned char *) &sp_data, 4);
  the_target->read_memory (sp + pc_offset, (unsigned char *) &next_pc, 4);
  the_target->read_memory (sp + pc_offset + 4, (unsigned char *) &cpsr, 4);
      target_read_memory (pc, (unsigned char *) &this_instr, 4);
      if (read_memory (pc - 4, (unsigned char *) &insn, 4))

==> linux-low.relevant_grep_results <==
  return the_target->read_memory (memaddr, myaddr, len);

==> linux-riscv-low.relevant_grep_results <==
  if (target_read_memory (*pcptr, buf.bytes, sizeof (buf.insn)) == 0
  if (target_read_memory (pc, buf.bytes, sizeof (buf.insn)) == 0
	      && target_read_memory (pc + sizeof (buf.insn), buf.bytes,

==> linux-sparc-low.relevant_grep_results <==
      the_target->read_memory (addr, tmp_reg_buf, sizeof (tmp_reg_buf));
  read_memory (where, (unsigned char *) insn, sizeof (insn));

==> win32-low.relevant_grep_results <==
--------

First, I am confused, because apparently there are 3 ways to
skin a target's memory [2]:

the_target->read_memory()             8 occurrences
target_read_memory()                  7 occurrences
read_memory()                         2 occurrences

"the_target->read_memory()" and "read_memory()" should be the
same in terms of code execution (not code writing). Then,
there is "target_read_memory()" which I think is the default
implementation [3].

Maybe it is my style, but I dislike hunting the implementation
of a pure virtual method somewhere along the inheritance line
(lineage? :p). Since ARC is not overriding the "read_memory()",
it is more meaningful to use the "target_read_memory()". I will
be changing "the_target->read_memory()" to that then.

[1] Master branch at commit 9aed480c3a7
https://sourceware.org/git/?p=binutils-gdb.git;hb=HEAD;a=tree

[2]
To be clear, what I mean is "There are 3 flavors". I just couldn't
leave this joke alone.

[3]
"process_stratum_target" class (the base) provides
"read_memory()" as pure virtual method. Therefore, it must be defined
overridden somewhere and I _think_ this is the default implementation.

> FWIW, I'm leaning towards what Baris says, better use "this" than the
> global variable.  And I even prefer to make it explicit,
> "this->read_memory (...", to make it clear that we call a method and not
> a free function.  But I don't want to start an endless discussion about
> coding style :).

Please see my answer above.

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

[Shahab Vahedi]

On Tue, Sep 29, 2020 at 10:24:59AM +0200, Shahab Vahedi wrote:
> "the_target->read_memory()" and "read_memory()" should be the
> same in terms of code execution (not code writing). Then,
> there is "target_read_memory()" which I think is the default
> implementation [3].
> 
> [3]
> "process_stratum_target" class (the base) provides
> "read_memory()" as pure virtual method. Therefore, it must be defined
> overridden somewhere and I _think_ this is the default implementation.

To be correct, they are NOT the same:

the_target->read_memory --> linux-low.cc: linux_process_target::read_memory
target_read_memory      --> target.cc:    read_inferior_memory

The second is a wrapper around the first one with some sanity checks.
This supports my previous decision to using it.


Cheers,
Shahab

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

[Simon Marchi]

On 2020-09-29 4:24 a.m., Shahab Vahedi wrote:
> Hi Baris, Simon,
>
> On Mon, Sep 28, 2020 at 03:10:23PM -0400, Simon Marchi wrote:
>> On 2020-09-28 10:08 a.m., Aktemur, Tankut Baris wrote:
>>>
>>> Just for the record, there are similar uses in linux-aarch32-low.cc,
>>> linux-arm-low.cc, linux-low.cc, linux-sparc-low.cc, and win32-low.cc (I don't
>>> see it in linux-riscv-low.cc), and in all of these uses the context is a function
>>> (not a method) where the 'this' pointer is not available.
>
> I've looked into those files in master branch [1] which at the time
> was at commit 9aed480c3a7. The relevant results are:
>
> --------
> ==> linux-aarch32-low.relevant_grep_results <==
>       the_target->read_memory (where, (unsigned char *) &insn, 2);
> 	  the_target->read_memory (where + 2, (unsigned char *) &insn, 2);
>       the_target->read_memory (where, (unsigned char *) &insn, 4);
>       if (target_read_memory (*pcptr, buf, 2) == 0)
> 	  target_read_memory (*pcptr, (gdb_byte *) &inst1, 2);
>
> ==> linux-arm-low.relevant_grep_results <==
>   target_read_memory (memaddr, (unsigned char *) &res, len);
>   the_target->read_memory (sp, (unsigned char *) &sp_data, 4);
>   the_target->read_memory (sp + pc_offset, (unsigned char *) &next_pc, 4);
>   the_target->read_memory (sp + pc_offset + 4, (unsigned char *) &cpsr, 4);
>       target_read_memory (pc, (unsigned char *) &this_instr, 4);
>       if (read_memory (pc - 4, (unsigned char *) &insn, 4))
>
> ==> linux-low.relevant_grep_results <==
>   return the_target->read_memory (memaddr, myaddr, len);
>
> ==> linux-riscv-low.relevant_grep_results <==
>   if (target_read_memory (*pcptr, buf.bytes, sizeof (buf.insn)) == 0
>   if (target_read_memory (pc, buf.bytes, sizeof (buf.insn)) == 0
> 	      && target_read_memory (pc + sizeof (buf.insn), buf.bytes,
>
> ==> linux-sparc-low.relevant_grep_results <==
>       the_target->read_memory (addr, tmp_reg_buf, sizeof (tmp_reg_buf));
>   read_memory (where, (unsigned char *) insn, sizeof (insn));
>
> ==> win32-low.relevant_grep_results <==
> --------
>
> First, I am confused, because apparently there are 3 ways to
> skin a target's memory [2]:
>
> the_target->read_memory()             8 occurrences
> target_read_memory()                  7 occurrences
> read_memory()                         2 occurrences
>
> "the_target->read_memory()" and "read_memory()" should be the
> same in terms of code execution (not code writing). Then,
> there is "target_read_memory()" which I think is the default
> implementation [3].

target_read_memory isn't the default implementation of
process_stratum_target::read_memory.

target_read_memory calls read_inferior_memory, which calls the target's
read_memory method, and then puts back in place the instructions
shadowed by breakpoints and fast tracepoint jumps inserted by GDBserver
in place.  This is so that the memory returned to the caller (and
possibly to GDB) contains the contents it expects to see.

It also exists because it implements the "target" interface in
gdb/target/target.h interface, which was meant to provide a common
interface between GDB and GDBserver.  Presumably, this allows calling it
from common code.  Although, I'm not sure whether having this common
interface was ever really useful...

`read_memory (...)` is the same as `this->read_memory (...)`, it calls
process_stratum_target::read_memory on the current target object.  The
concrete implementation of this method contains calls to the platform's
API to fetch the requested bytes from the process.  Like I said before I
personally prefer the second version just because it makes it clear
you're not calling a free function (especially that we a `read_memory`
free function in GDB, so it may be confusing).

`the_target->read_memory (...)` is the also the same as
`this->read_memory (...)` when we are already in the method of a target.
When we are already in such a context, I don't really see the point of
using `the_target`, as it just adds more unnecessary references to a
global variable.  It could make the reader wonder "oh, are we calling
read_memory on some other object?" before they realize that the_target
is the same as this.  So, it's not a big deal for me, but I just find it
expresses the intention a bit better.

So the right thing to call between target_read_memory /
read_inferior_memory and process_stratum_target::read_memory depends if
you need the breakpoint shadowing replacement feature or not.  I presume
that in many contexts, it won't make a difference.

>
> Maybe it is my style, but I dislike hunting the implementation
> of a pure virtual method somewhere along the inheritance line
> (lineage? :p). Since ARC is not overriding the "read_memory()",
> it is more meaningful to use the "target_read_memory()". I will
> be changing "the_target->read_memory()" to that then.

I don't understand what you mean.  target_read_memory ends up calling
the_target->read_memory (and it does the breakpoint shadowing stuff on
top).  So those calls are _not_ equivalent, and they both end up calling
the concrete implementation of the pure virtual method.  So I don't
understand what you win here.

> [3]
> "process_stratum_target" class (the base) provides
> "read_memory()" as pure virtual method. Therefore, it must be defined
> overridden somewhere and I _think_ this is the default implementation.

No it isn't.  There are three implementations of
process_stratum_target::read_memory that I and my IDE know of:

    $ ag '::read_memory'
    linux-low.cc
    5522:linux_process_target::read_memory (CORE_ADDR memaddr,

    win32-low.cc
    1675:win32_process_target::read_memory (CORE_ADDR memaddr, unsigned char *myaddr,

    netbsd-low.cc
    554:netbsd_process_target::read_memory (CORE_ADDR memaddr, unsigned char *myaddr,

As I said previously, the implementation of
process_stratum_target::read_memory does calls to the native platform's
API to fetch the bytes from the process' memory.  These three platforms
(linux, win32 and netbsd) all have different ways of doing it, hence the
three implementations.

The linux one offers an implementation that is re-used by all the final
linux targets (aarch64_target, x86_target, etc) because it is the same
on all Linux systems, regardless of the architecture.

Simon

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

[Shahab Vahedi]

Hi Simon,

First, thank you a lot for such explanatory answer.

On Tue, Sep 29, 2020 at 10:22:18AM -0400, Simon Marchi wrote:
> target_read_memory calls read_inferior_memory, which calls the target's
> read_memory method, and then puts back in place the instructions
> shadowed by breakpoints and fast tracepoint jumps inserted by GDBserver
> in place.  This is so that the memory returned to the caller (and
> possibly to GDB) contains the contents it expects to see.

I repeat the code snippet here:
----
bool
arc_target::low_breakpoint_at (CORE_ADDR where)
{
  uint16_t insn;
  uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);

  target_read_memory (where, (gdb_byte *) &insn, TRAP_S_1_SIZE);
  return (insn == breakpoint);
}
----

Because this memory access is indeed to check for a "breakpoint",
then I should NOT be using breakpoint-undoing edition.  Therefore,
I will use "this->read_memory (...)".


Cheers,
Shahab

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

[Shahab Vahedi]

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].

v2 [2]:
- arc_linux_collect_gregset: Use "reg <= ARC_LAST_REGNUM" instead of
  "reg < ARC_LAST_REGNUM" for the condition check of the for-loop.
- arc-linux-tdep.c: Use "ARC_LAST_REGNUM < ARRAY_SIZE (...)" instead of
  "ARC_LAST_REGNUM < ARRAY_SIZE (...)" for the "asserts".
- Use "buf + arc_linux_core_reg_offsets[ARC_ERET_REGNUM]" instead of
  "buf + REG_OFF (6)".
- Some indentation fixes.

[1] arc: Add support for ARC HS extra registers in core files
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2745674244d6aecddcf636475034bdb9c0a6b4a0

[2] Remarks
https://sourceware.org/pipermail/gdb-patches/2020-September/171912.html

gdb/ChangeLog:

	* arc-linux-tdep.h: New file.
	* arc-linux-tdep.c (arc_linux_core_reg_offsets,
	arc_linux_supply_gregset, arc_linux_supply_v2_regset,
	arc_linux_collect_gregset, arc_linux_collect_v2_regset,
	arc_linux_gregset, arc_linux_v2_regset,
	arc_linux_iterate_over_regset_sections,
	arc_linux_core_read_description): Implement.
	(arc_linux_init_osabi): Set iterate_over_regset_sections.
	* arc-tdep.h (ARC_OFFSET_NO_REGISTER): Declare.
        (arc_gdbarch_features_create): Add.
	* arc-tdep.c (arc_gdbarch_features_create): Not static anymore.
---
 gdb/arc-linux-tdep.c | 193 +++++++++++++++++++++++++++++++++++++++++++
 gdb/arc-linux-tdep.h |  52 ++++++++++++
 gdb/arc-tdep.c       |   5 +-
 gdb/arc-tdep.h       |  12 ++-
 4 files changed, 258 insertions(+), 4 deletions(-)
 create mode 100644 gdb/arc-linux-tdep.h

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 36f32459bbe..9590397273d 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -27,7 +27,65 @@
 
 /* ARC header files.  */
 #include "opcodes/arc-dis.h"
+#include "arc-linux-tdep.h"
 #include "arc-tdep.h"
+#include "arch/arc.h"
+
+#define REGOFF(offset) (offset * ARC_REGISTER_SIZE)
+
+/* 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.  */
+
+/* The layout of this struct is tightly bound to "arc_regnum" enum
+   in arc-tdep.h.  Any change of order in there, must be reflected
+   here as well.  */
+static const int arc_linux_core_reg_offsets[] = {
+  /* R0 - R12.  */
+  REGOFF (22), REGOFF (21), REGOFF (20), REGOFF (19),
+  REGOFF (18), REGOFF (17), REGOFF (16), REGOFF (15),
+  REGOFF (14), REGOFF (13), REGOFF (12), REGOFF (11),
+  REGOFF (10),
+
+  /* R13 - R25.  */
+  REGOFF (37), REGOFF (36), REGOFF (35), REGOFF (34),
+  REGOFF (33), REGOFF (32), REGOFF (31), REGOFF (30),
+  REGOFF (29), REGOFF (28), REGOFF (27), REGOFF (26),
+  REGOFF (25),
+
+  REGOFF (9),			/* R26 (GP) */
+  REGOFF (8),			/* FP */
+  REGOFF (23),			/* SP */
+  ARC_OFFSET_NO_REGISTER,	/* ILINK */
+  ARC_OFFSET_NO_REGISTER,	/* R30 */
+  REGOFF (7),			/* BLINK */
+
+  /* R32 - R59.  */
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER,
+
+  REGOFF (4),			/* LP_COUNT */
+  ARC_OFFSET_NO_REGISTER,	/* RESERVED */
+  ARC_OFFSET_NO_REGISTER,	/* LIMM */
+  ARC_OFFSET_NO_REGISTER,	/* PCL */
+
+  REGOFF (39),			/* PC  */
+  REGOFF (5),			/* STATUS32 */
+  REGOFF (2),			/* LP_START */
+  REGOFF (3),			/* LP_END */
+  REGOFF (1),			/* BTA */
+  REGOFF (6)			/* ERET */
+};
 
 /* Implement the "cannot_fetch_register" gdbarch method.  */
 
@@ -227,6 +285,138 @@ 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));
+
+  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,
+		  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
+     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))
+    regnum = ARC_ERET_REGNUM;
+  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+}
+
+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.");
+}
+
+void
+arc_linux_collect_v2_regset (const struct regset *regset,
+			     const struct regcache *regcache, int regnum,
+			     void *v2_regs, size_t size)
+{
+  bfd_byte *buf = (bfd_byte *) v2_regs;
+
+  regcache->raw_collect (ARC_R30_REGNUM, buf);
+  regcache->raw_collect (ARC_R58_REGNUM, buf + REGOFF (1));
+  regcache->raw_collect (ARC_R59_REGNUM, buf + REGOFF (2));
+}
+
+/* Linux regset definitions.  */
+
+static const struct regset arc_linux_gregset = {
+  arc_linux_core_reg_offsets,
+  arc_linux_supply_gregset,
+  arc_linux_collect_gregset,
+};
+
+static const struct regset arc_linux_v2_regset = {
+  NULL,
+  arc_linux_supply_v2_regset,
+  arc_linux_collect_v2_regset,
+};
+
+/* Implement the `iterate_over_regset_sections` gdbarch method.  */
+
+static void
+arc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
+					iterate_over_regset_sections_cb *cb,
+					void *cb_data,
+					const struct regcache *regcache)
+{
+  /* There are 40 registers in Linux user_regs_struct, although some of
+     them are now just a mere paddings, kept to maintain binary
+     compatibility with older tools.  */
+  const int sizeof_gregset = 40 * ARC_REGISTER_SIZE;
+
+  cb (".reg", sizeof_gregset, sizeof_gregset, &arc_linux_gregset, NULL,
+      cb_data);
+  cb (".reg-arc-v2", ARC_LINUX_SIZEOF_V2_REGSET, ARC_LINUX_SIZEOF_V2_REGSET,
+      &arc_linux_v2_regset, NULL, cb_data);
+}
+
+/* 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);
+  return arc_lookup_target_description (features);
+}
+
 /* Initialization specific to Linux environment.  */
 
 static void
@@ -260,6 +450,9 @@ arc_linux_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_software_single_step (gdbarch, arc_linux_software_single_step);
   set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);
   set_gdbarch_skip_solib_resolver (gdbarch, arc_linux_skip_solib_resolver);
+  set_gdbarch_iterate_over_regset_sections
+    (gdbarch, arc_linux_iterate_over_regset_sections);
+  set_gdbarch_core_read_description (gdbarch, arc_linux_core_read_description);
 
   /* GNU/Linux uses SVR4-style shared libraries, with 32-bit ints, longs
      and pointers (ILP32).  */
diff --git a/gdb/arc-linux-tdep.h b/gdb/arc-linux-tdep.h
new file mode 100644
index 00000000000..c26cacd7029
--- /dev/null
+++ b/gdb/arc-linux-tdep.h
@@ -0,0 +1,52 @@
+/* Target dependent code for GNU/Linux ARC.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef ARC_LINUX_TDEP_H
+#define ARC_LINUX_TDEP_H
+
+#include "gdbarch.h"
+#include "regset.h"
+
+#define ARC_LINUX_SIZEOF_V2_REGSET (3 * ARC_REGISTER_SIZE)
+
+/* Reads registers from the NT_PRSTATUS data array into the regcache.  */
+
+void arc_linux_supply_gregset (const struct regset *regset,
+			       struct regcache *regcache, int regnum,
+			       const void *gregs, size_t size);
+
+/* Reads regsiters from the NT_ARC_V2 data array into the regcache.  */
+
+void arc_linux_supply_v2_regset (const struct regset *regset,
+				 struct regcache *regcache, int regnum,
+				 const void *v2_regs, size_t size);
+
+/* Writes registers from the regcache into the NT_PRSTATUS data array.  */
+
+void arc_linux_collect_gregset (const struct regset *regset,
+				const struct regcache *regcache,
+				int regnum, void *gregs, size_t size);
+
+/* Writes registers from the regcache into the NT_ARC_V2 data array.  */
+
+void arc_linux_collect_v2_regset (const struct regset *regset,
+				  const struct regcache *regcache,
+				  int regnum, void *v2_regs, size_t size);
+
+#endif /* ARC_LINUX_TDEP_H */
diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 6f544bff78d..91e106a0167 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -1883,10 +1883,9 @@ mach_type_to_arc_isa (const unsigned long mach)
     }
 }
 
-/* Common construction code for ARC_GDBARCH_FEATURES struct.  If there
-   is no ABFD, then a FEATURE with default values is returned.  */
+/* See arc-tdep.h.  */
 
-static arc_gdbarch_features
+arc_gdbarch_features
 arc_gdbarch_features_create (const bfd *abfd, const unsigned long mach)
 {
   /* Use 4 as a fallback value.  */
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 6331d29f402..917737d822d 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -85,7 +85,9 @@ enum arc_regnum
     ARC_LP_END_REGNUM,
     /* Branch target address.  */
     ARC_BTA_REGNUM,
-    ARC_LAST_AUX_REGNUM = ARC_BTA_REGNUM,
+    /* Exception return address.  */
+    ARC_ERET_REGNUM,
+    ARC_LAST_AUX_REGNUM = ARC_ERET_REGNUM,
     ARC_LAST_REGNUM = ARC_LAST_AUX_REGNUM,
 
     /* Additional ABI constants.  */
@@ -105,6 +107,9 @@ enum arc_regnum
 /* STATUS32 register: current instruction is a delay slot.  */
 #define ARC_STATUS32_DE_MASK (1 << 6)
 
+/* Special value for register offset arrays.  */
+#define ARC_OFFSET_NO_REGISTER (-1)
+
 #define arc_print(fmt, args...) fprintf_unfiltered (gdb_stdlog, fmt, ##args)
 
 extern int arc_debug;
@@ -182,4 +187,9 @@ CORE_ADDR arc_insn_get_branch_target (const struct arc_instruction &insn);
 
 CORE_ADDR arc_insn_get_linear_next_pc (const struct arc_instruction &insn);
 
+/* Create an arc_gdbarch_features instance from the provided data.  */
+
+arc_gdbarch_features arc_gdbarch_features_create (const bfd *abfd,
+						  const unsigned long mach);
+
 #endif /* ARC_TDEP_H */
-- 
2.28.0

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

[Shahab Vahedi]

Hi Simon,

On Wed, Sep 16, 2020 at 04:21:43PM -0400, Simon Marchi wrote:
> On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote:
> > +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.

You're absolutely right! Not only this question made me fix the assert
here, but also to revisit it everywhere. In the end, it resulted in
finding a "for loop" that was not looping the whole data set. All have
been mentioned/fixed in the next patch v2 [1].


[1] [PATCH v2] arc: Add support for Linux coredump files
https://sourceware.org/pipermail/gdb-patches/2020-September/172199.html


Cheers,
Shahab

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

[Simon Marchi]

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

[PUSHED master] arc: Add support for Linux coredump files

[Shahab Vahedi]

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].

v2 [2]:
- arc_linux_collect_gregset: Use "reg <= ARC_LAST_REGNUM" instead of
  "reg < ARC_LAST_REGNUM" for the condition check of the for-loop.
- arc-linux-tdep.c: Use "ARC_LAST_REGNUM < ARRAY_SIZE (...)" instead of
  "ARC_LAST_REGNUM <= ARRAY_SIZE (...)" for the "asserts".
- Use "buf + arc_linux_core_reg_offsets[ARC_ERET_REGNUM]" instead of
  "buf + REG_OFF (6)".
- Fix a few typos/indentation.

v3 [3]:
- Use gdb_assert_not_reached(text) instead of gdb_assert (!text).
- Remove unnecessary braces in the for loop.

[1] arc: Add support for ARC HS extra registers in core files
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2745674244d6aecddcf636475034bdb9c0a6b4a0

[2] First remarks
https://sourceware.org/pipermail/gdb-patches/2020-September/171912.html

[3] Second remarks
https://sourceware.org/pipermail/gdb-patches/2020-October/172302.html

gdb/ChangeLog:

	* arc-linux-tdep.h: New file.
	* arc-linux-tdep.c (arc_linux_core_reg_offsets,
	arc_linux_supply_gregset, arc_linux_supply_v2_regset,
	arc_linux_collect_gregset, arc_linux_collect_v2_regset,
	arc_linux_gregset, arc_linux_v2_regset,
	arc_linux_iterate_over_regset_sections,
	arc_linux_core_read_description): Implement.
	(arc_linux_init_osabi): Set iterate_over_regset_sections.
	* arc-tdep.h (ARC_OFFSET_NO_REGISTER): Declare.
        (arc_gdbarch_features_create): Add.
	* arc-tdep.c (arc_gdbarch_features_create): Not static anymore.
---
 gdb/arc-linux-tdep.c | 191 +++++++++++++++++++++++++++++++++++++++++++
 gdb/arc-linux-tdep.h |  52 ++++++++++++
 gdb/arc-tdep.c       |   5 +-
 gdb/arc-tdep.h       |  12 ++-
 4 files changed, 256 insertions(+), 4 deletions(-)
 create mode 100644 gdb/arc-linux-tdep.h

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 36f32459bbe..9ff5f1214a1 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -27,7 +27,65 @@
 
 /* ARC header files.  */
 #include "opcodes/arc-dis.h"
+#include "arc-linux-tdep.h"
 #include "arc-tdep.h"
+#include "arch/arc.h"
+
+#define REGOFF(offset) (offset * ARC_REGISTER_SIZE)
+
+/* 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.  */
+
+/* The layout of this struct is tightly bound to "arc_regnum" enum
+   in arc-tdep.h.  Any change of order in there, must be reflected
+   here as well.  */
+static const int arc_linux_core_reg_offsets[] = {
+  /* R0 - R12.  */
+  REGOFF (22), REGOFF (21), REGOFF (20), REGOFF (19),
+  REGOFF (18), REGOFF (17), REGOFF (16), REGOFF (15),
+  REGOFF (14), REGOFF (13), REGOFF (12), REGOFF (11),
+  REGOFF (10),
+
+  /* R13 - R25.  */
+  REGOFF (37), REGOFF (36), REGOFF (35), REGOFF (34),
+  REGOFF (33), REGOFF (32), REGOFF (31), REGOFF (30),
+  REGOFF (29), REGOFF (28), REGOFF (27), REGOFF (26),
+  REGOFF (25),
+
+  REGOFF (9),			/* R26 (GP) */
+  REGOFF (8),			/* FP */
+  REGOFF (23),			/* SP */
+  ARC_OFFSET_NO_REGISTER,	/* ILINK */
+  ARC_OFFSET_NO_REGISTER,	/* R30 */
+  REGOFF (7),			/* BLINK */
+
+  /* R32 - R59.  */
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER,
+
+  REGOFF (4),			/* LP_COUNT */
+  ARC_OFFSET_NO_REGISTER,	/* RESERVED */
+  ARC_OFFSET_NO_REGISTER,	/* LIMM */
+  ARC_OFFSET_NO_REGISTER,	/* PCL */
+
+  REGOFF (39),			/* PC  */
+  REGOFF (5),			/* STATUS32 */
+  REGOFF (2),			/* LP_START */
+  REGOFF (3),			/* LP_END */
+  REGOFF (1),			/* BTA */
+  REGOFF (6)			/* ERET */
+};
 
 /* Implement the "cannot_fetch_register" gdbarch method.  */
 
@@ -227,6 +285,136 @@ 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));
+
+  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,
+		  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
+     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))
+    regnum = ARC_ERET_REGNUM;
+  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+}
+
+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_not_reached ("Invalid regnum in arc_linux_collect_gregset.");
+}
+
+void
+arc_linux_collect_v2_regset (const struct regset *regset,
+			     const struct regcache *regcache, int regnum,
+			     void *v2_regs, size_t size)
+{
+  bfd_byte *buf = (bfd_byte *) v2_regs;
+
+  regcache->raw_collect (ARC_R30_REGNUM, buf);
+  regcache->raw_collect (ARC_R58_REGNUM, buf + REGOFF (1));
+  regcache->raw_collect (ARC_R59_REGNUM, buf + REGOFF (2));
+}
+
+/* Linux regset definitions.  */
+
+static const struct regset arc_linux_gregset = {
+  arc_linux_core_reg_offsets,
+  arc_linux_supply_gregset,
+  arc_linux_collect_gregset,
+};
+
+static const struct regset arc_linux_v2_regset = {
+  NULL,
+  arc_linux_supply_v2_regset,
+  arc_linux_collect_v2_regset,
+};
+
+/* Implement the `iterate_over_regset_sections` gdbarch method.  */
+
+static void
+arc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
+					iterate_over_regset_sections_cb *cb,
+					void *cb_data,
+					const struct regcache *regcache)
+{
+  /* There are 40 registers in Linux user_regs_struct, although some of
+     them are now just a mere paddings, kept to maintain binary
+     compatibility with older tools.  */
+  const int sizeof_gregset = 40 * ARC_REGISTER_SIZE;
+
+  cb (".reg", sizeof_gregset, sizeof_gregset, &arc_linux_gregset, NULL,
+      cb_data);
+  cb (".reg-arc-v2", ARC_LINUX_SIZEOF_V2_REGSET, ARC_LINUX_SIZEOF_V2_REGSET,
+      &arc_linux_v2_regset, NULL, cb_data);
+}
+
+/* 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_arch_features features
+    = arc_arch_features_create (abfd,
+				gdbarch_bfd_arch_info (gdbarch)->mach);
+  return arc_lookup_target_description (features);
+}
+
 /* Initialization specific to Linux environment.  */
 
 static void
@@ -260,6 +448,9 @@ arc_linux_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_software_single_step (gdbarch, arc_linux_software_single_step);
   set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);
   set_gdbarch_skip_solib_resolver (gdbarch, arc_linux_skip_solib_resolver);
+  set_gdbarch_iterate_over_regset_sections
+    (gdbarch, arc_linux_iterate_over_regset_sections);
+  set_gdbarch_core_read_description (gdbarch, arc_linux_core_read_description);
 
   /* GNU/Linux uses SVR4-style shared libraries, with 32-bit ints, longs
      and pointers (ILP32).  */
diff --git a/gdb/arc-linux-tdep.h b/gdb/arc-linux-tdep.h
new file mode 100644
index 00000000000..c26cacd7029
--- /dev/null
+++ b/gdb/arc-linux-tdep.h
@@ -0,0 +1,52 @@
+/* Target dependent code for GNU/Linux ARC.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef ARC_LINUX_TDEP_H
+#define ARC_LINUX_TDEP_H
+
+#include "gdbarch.h"
+#include "regset.h"
+
+#define ARC_LINUX_SIZEOF_V2_REGSET (3 * ARC_REGISTER_SIZE)
+
+/* Reads registers from the NT_PRSTATUS data array into the regcache.  */
+
+void arc_linux_supply_gregset (const struct regset *regset,
+			       struct regcache *regcache, int regnum,
+			       const void *gregs, size_t size);
+
+/* Reads regsiters from the NT_ARC_V2 data array into the regcache.  */
+
+void arc_linux_supply_v2_regset (const struct regset *regset,
+				 struct regcache *regcache, int regnum,
+				 const void *v2_regs, size_t size);
+
+/* Writes registers from the regcache into the NT_PRSTATUS data array.  */
+
+void arc_linux_collect_gregset (const struct regset *regset,
+				const struct regcache *regcache,
+				int regnum, void *gregs, size_t size);
+
+/* Writes registers from the regcache into the NT_ARC_V2 data array.  */
+
+void arc_linux_collect_v2_regset (const struct regset *regset,
+				  const struct regcache *regcache,
+				  int regnum, void *v2_regs, size_t size);
+
+#endif /* ARC_LINUX_TDEP_H */
diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 60c76af3352..c29a16eccf6 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -1883,10 +1883,9 @@ mach_type_to_arc_isa (const unsigned long mach)
     }
 }
 
-/* Common construction code for ARC_ARCH_FEATURES struct.  If there
-   is no ABFD, then a FEATURE with default values is returned.  */
+/* See arc-tdep.h.  */
 
-static arc_arch_features
+arc_arch_features
 arc_arch_features_create (const bfd *abfd, const unsigned long mach)
 {
   /* Use 4 as a fallback value.  */
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 6331d29f402..09fcbeab9c7 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -85,7 +85,9 @@ enum arc_regnum
     ARC_LP_END_REGNUM,
     /* Branch target address.  */
     ARC_BTA_REGNUM,
-    ARC_LAST_AUX_REGNUM = ARC_BTA_REGNUM,
+    /* Exception return address.  */
+    ARC_ERET_REGNUM,
+    ARC_LAST_AUX_REGNUM = ARC_ERET_REGNUM,
     ARC_LAST_REGNUM = ARC_LAST_AUX_REGNUM,
 
     /* Additional ABI constants.  */
@@ -105,6 +107,9 @@ enum arc_regnum
 /* STATUS32 register: current instruction is a delay slot.  */
 #define ARC_STATUS32_DE_MASK (1 << 6)
 
+/* Special value for register offset arrays.  */
+#define ARC_OFFSET_NO_REGISTER (-1)
+
 #define arc_print(fmt, args...) fprintf_unfiltered (gdb_stdlog, fmt, ##args)
 
 extern int arc_debug;
@@ -182,4 +187,9 @@ CORE_ADDR arc_insn_get_branch_target (const struct arc_instruction &insn);
 
 CORE_ADDR arc_insn_get_linear_next_pc (const struct arc_instruction &insn);
 
+/* Create an arc_arch_features instance from the provided data.  */
+
+arc_arch_features arc_arch_features_create (const bfd *abfd,
+					    const unsigned long mach);
+
 #endif /* ARC_TDEP_H */
-- 
2.28.0

[PUSHED gdb-10-branch] arc: Add support for Linux coredump files

[Shahab Vahedi]

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].

v2 [2]:
- arc_linux_collect_gregset: Use "reg <= ARC_LAST_REGNUM" instead of
  "reg < ARC_LAST_REGNUM" for the condition check of the for-loop.
- arc-linux-tdep.c: Use "ARC_LAST_REGNUM < ARRAY_SIZE (...)" instead of
  "ARC_LAST_REGNUM <= ARRAY_SIZE (...)" for the "asserts".
- Use "buf + arc_linux_core_reg_offsets[ARC_ERET_REGNUM]" instead of
  "buf + REG_OFF (6)".
- Fix a few typos/indentation.

v3 [3]:
- Use gdb_assert_not_reached(text) instead of gdb_assert (!text).
- Remove unnecessary braces in the for loop.

[1] arc: Add support for ARC HS extra registers in core files
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2745674244d6aecddcf636475034bdb9c0a6b4a0

[2] First remarks
https://sourceware.org/pipermail/gdb-patches/2020-September/171912.html

[3] Second remarks
https://sourceware.org/pipermail/gdb-patches/2020-October/172302.html

gdb/ChangeLog:

	* arc-linux-tdep.h: New file.
	* arc-linux-tdep.c (arc_linux_core_reg_offsets,
	arc_linux_supply_gregset, arc_linux_supply_v2_regset,
	arc_linux_collect_gregset, arc_linux_collect_v2_regset,
	arc_linux_gregset, arc_linux_v2_regset,
	arc_linux_iterate_over_regset_sections,
	arc_linux_core_read_description): Implement.
	(arc_linux_init_osabi): Set iterate_over_regset_sections.
	* arc-tdep.h (ARC_OFFSET_NO_REGISTER): Declare.
        (arc_gdbarch_features_create): Add.
	* arc-tdep.c (arc_gdbarch_features_create): Not static anymore.
---
 gdb/arc-linux-tdep.c | 191 +++++++++++++++++++++++++++++++++++++++++++
 gdb/arc-linux-tdep.h |  52 ++++++++++++
 gdb/arc-tdep.c       |   5 +-
 gdb/arc-tdep.h       |  12 ++-
 4 files changed, 256 insertions(+), 4 deletions(-)
 create mode 100644 gdb/arc-linux-tdep.h

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 36f32459bbe..9ff5f1214a1 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -27,7 +27,65 @@
 
 /* ARC header files.  */
 #include "opcodes/arc-dis.h"
+#include "arc-linux-tdep.h"
 #include "arc-tdep.h"
+#include "arch/arc.h"
+
+#define REGOFF(offset) (offset * ARC_REGISTER_SIZE)
+
+/* 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.  */
+
+/* The layout of this struct is tightly bound to "arc_regnum" enum
+   in arc-tdep.h.  Any change of order in there, must be reflected
+   here as well.  */
+static const int arc_linux_core_reg_offsets[] = {
+  /* R0 - R12.  */
+  REGOFF (22), REGOFF (21), REGOFF (20), REGOFF (19),
+  REGOFF (18), REGOFF (17), REGOFF (16), REGOFF (15),
+  REGOFF (14), REGOFF (13), REGOFF (12), REGOFF (11),
+  REGOFF (10),
+
+  /* R13 - R25.  */
+  REGOFF (37), REGOFF (36), REGOFF (35), REGOFF (34),
+  REGOFF (33), REGOFF (32), REGOFF (31), REGOFF (30),
+  REGOFF (29), REGOFF (28), REGOFF (27), REGOFF (26),
+  REGOFF (25),
+
+  REGOFF (9),			/* R26 (GP) */
+  REGOFF (8),			/* FP */
+  REGOFF (23),			/* SP */
+  ARC_OFFSET_NO_REGISTER,	/* ILINK */
+  ARC_OFFSET_NO_REGISTER,	/* R30 */
+  REGOFF (7),			/* BLINK */
+
+  /* R32 - R59.  */
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER, ARC_OFFSET_NO_REGISTER,
+  ARC_OFFSET_NO_REGISTER,
+
+  REGOFF (4),			/* LP_COUNT */
+  ARC_OFFSET_NO_REGISTER,	/* RESERVED */
+  ARC_OFFSET_NO_REGISTER,	/* LIMM */
+  ARC_OFFSET_NO_REGISTER,	/* PCL */
+
+  REGOFF (39),			/* PC  */
+  REGOFF (5),			/* STATUS32 */
+  REGOFF (2),			/* LP_START */
+  REGOFF (3),			/* LP_END */
+  REGOFF (1),			/* BTA */
+  REGOFF (6)			/* ERET */
+};
 
 /* Implement the "cannot_fetch_register" gdbarch method.  */
 
@@ -227,6 +285,136 @@ 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));
+
+  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,
+		  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
+     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))
+    regnum = ARC_ERET_REGNUM;
+  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+}
+
+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_not_reached ("Invalid regnum in arc_linux_collect_gregset.");
+}
+
+void
+arc_linux_collect_v2_regset (const struct regset *regset,
+			     const struct regcache *regcache, int regnum,
+			     void *v2_regs, size_t size)
+{
+  bfd_byte *buf = (bfd_byte *) v2_regs;
+
+  regcache->raw_collect (ARC_R30_REGNUM, buf);
+  regcache->raw_collect (ARC_R58_REGNUM, buf + REGOFF (1));
+  regcache->raw_collect (ARC_R59_REGNUM, buf + REGOFF (2));
+}
+
+/* Linux regset definitions.  */
+
+static const struct regset arc_linux_gregset = {
+  arc_linux_core_reg_offsets,
+  arc_linux_supply_gregset,
+  arc_linux_collect_gregset,
+};
+
+static const struct regset arc_linux_v2_regset = {
+  NULL,
+  arc_linux_supply_v2_regset,
+  arc_linux_collect_v2_regset,
+};
+
+/* Implement the `iterate_over_regset_sections` gdbarch method.  */
+
+static void
+arc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
+					iterate_over_regset_sections_cb *cb,
+					void *cb_data,
+					const struct regcache *regcache)
+{
+  /* There are 40 registers in Linux user_regs_struct, although some of
+     them are now just a mere paddings, kept to maintain binary
+     compatibility with older tools.  */
+  const int sizeof_gregset = 40 * ARC_REGISTER_SIZE;
+
+  cb (".reg", sizeof_gregset, sizeof_gregset, &arc_linux_gregset, NULL,
+      cb_data);
+  cb (".reg-arc-v2", ARC_LINUX_SIZEOF_V2_REGSET, ARC_LINUX_SIZEOF_V2_REGSET,
+      &arc_linux_v2_regset, NULL, cb_data);
+}
+
+/* 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_arch_features features
+    = arc_arch_features_create (abfd,
+				gdbarch_bfd_arch_info (gdbarch)->mach);
+  return arc_lookup_target_description (features);
+}
+
 /* Initialization specific to Linux environment.  */
 
 static void
@@ -260,6 +448,9 @@ arc_linux_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_software_single_step (gdbarch, arc_linux_software_single_step);
   set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);
   set_gdbarch_skip_solib_resolver (gdbarch, arc_linux_skip_solib_resolver);
+  set_gdbarch_iterate_over_regset_sections
+    (gdbarch, arc_linux_iterate_over_regset_sections);
+  set_gdbarch_core_read_description (gdbarch, arc_linux_core_read_description);
 
   /* GNU/Linux uses SVR4-style shared libraries, with 32-bit ints, longs
      and pointers (ILP32).  */
diff --git a/gdb/arc-linux-tdep.h b/gdb/arc-linux-tdep.h
new file mode 100644
index 00000000000..c26cacd7029
--- /dev/null
+++ b/gdb/arc-linux-tdep.h
@@ -0,0 +1,52 @@
+/* Target dependent code for GNU/Linux ARC.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef ARC_LINUX_TDEP_H
+#define ARC_LINUX_TDEP_H
+
+#include "gdbarch.h"
+#include "regset.h"
+
+#define ARC_LINUX_SIZEOF_V2_REGSET (3 * ARC_REGISTER_SIZE)
+
+/* Reads registers from the NT_PRSTATUS data array into the regcache.  */
+
+void arc_linux_supply_gregset (const struct regset *regset,
+			       struct regcache *regcache, int regnum,
+			       const void *gregs, size_t size);
+
+/* Reads regsiters from the NT_ARC_V2 data array into the regcache.  */
+
+void arc_linux_supply_v2_regset (const struct regset *regset,
+				 struct regcache *regcache, int regnum,
+				 const void *v2_regs, size_t size);
+
+/* Writes registers from the regcache into the NT_PRSTATUS data array.  */
+
+void arc_linux_collect_gregset (const struct regset *regset,
+				const struct regcache *regcache,
+				int regnum, void *gregs, size_t size);
+
+/* Writes registers from the regcache into the NT_ARC_V2 data array.  */
+
+void arc_linux_collect_v2_regset (const struct regset *regset,
+				  const struct regcache *regcache,
+				  int regnum, void *v2_regs, size_t size);
+
+#endif /* ARC_LINUX_TDEP_H */
diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 60c76af3352..c29a16eccf6 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -1883,10 +1883,9 @@ mach_type_to_arc_isa (const unsigned long mach)
     }
 }
 
-/* Common construction code for ARC_ARCH_FEATURES struct.  If there
-   is no ABFD, then a FEATURE with default values is returned.  */
+/* See arc-tdep.h.  */
 
-static arc_arch_features
+arc_arch_features
 arc_arch_features_create (const bfd *abfd, const unsigned long mach)
 {
   /* Use 4 as a fallback value.  */
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 6331d29f402..09fcbeab9c7 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -85,7 +85,9 @@ enum arc_regnum
     ARC_LP_END_REGNUM,
     /* Branch target address.  */
     ARC_BTA_REGNUM,
-    ARC_LAST_AUX_REGNUM = ARC_BTA_REGNUM,
+    /* Exception return address.  */
+    ARC_ERET_REGNUM,
+    ARC_LAST_AUX_REGNUM = ARC_ERET_REGNUM,
     ARC_LAST_REGNUM = ARC_LAST_AUX_REGNUM,
 
     /* Additional ABI constants.  */
@@ -105,6 +107,9 @@ enum arc_regnum
 /* STATUS32 register: current instruction is a delay slot.  */
 #define ARC_STATUS32_DE_MASK (1 << 6)
 
+/* Special value for register offset arrays.  */
+#define ARC_OFFSET_NO_REGISTER (-1)
+
 #define arc_print(fmt, args...) fprintf_unfiltered (gdb_stdlog, fmt, ##args)
 
 extern int arc_debug;
@@ -182,4 +187,9 @@ CORE_ADDR arc_insn_get_branch_target (const struct arc_instruction &insn);
 
 CORE_ADDR arc_insn_get_linear_next_pc (const struct arc_instruction &insn);
 
+/* Create an arc_arch_features instance from the provided data.  */
+
+arc_arch_features arc_arch_features_create (const bfd *abfd,
+					    const unsigned long mach);
+
 #endif /* ARC_TDEP_H */
-- 
2.28.0