public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/2] eBPF support
@ 2020-07-03 10:37 Jose E. Marchesi
  2020-07-03 10:37 ` [PATCH 1/2] gdb: support for eBPF Jose E. Marchesi
  0 siblings, 1 reply; 6+ messages in thread
From: Jose E. Marchesi @ 2020-07-03 10:37 UTC (permalink / raw)
  To: gdb-patches

Hi good peoples!

This patch series adds support for the eBPF virtual architecture to
GDB [1].

The first patch contains the basic bits to GDB in order to support the
bpf-unknown-none target.  Breakpointing and instruction
single-stepping works, but the debugging support in eBPF is still very
minimal.  This is mainly due to the many limitations imposed by the
architecture (disjoint stack, maximum stack size, etc).  We are
working to overcome these limitations, by introducing a variant called
xbpf, already supported in GCC with the -mxbpf option, whose purpose
is to ease debugging and to be used in other contexts different than
the Linux kernel, less restrictive.

The second patch adds a basic CGEN-based instruction simulator for
eBPF.  It can run many eBPF programs and works well with GDB.  A
testsuite covering the supported instructions is also included.  We
will be expanding it in order to emulate the several kernel contexts
in which eBPF programs can run, so eBPF developers can use GDB to
debug their programs without having to load them in a running kernel.
Currently the only kernel helper implemented in the simulator is
printk, which is used by the tests.

We of course commit to maintain and evolve this stuff :)

[1] Support for eBPF has been already added to both binutils and GCC.

Jose E. Marchesi (2):
  gdb: support for eBPF
  sim: eBPF simulator

 gdb/ChangeLog                       |    10 +
 gdb/Makefile.in                     |     3 +
 gdb/bpf-tdep.c                      |   331 +
 gdb/bpf-tdep.h                      |    46 +
 gdb/configure.tgt                   |     6 +
 gdb/doc/ChangeLog                   |     6 +
 gdb/doc/gdb.texinfo                 |    21 +
 sim/ChangeLog                       |    36 +
 sim/MAINTAINERS                     |     1 +
 sim/bpf/Makefile.in                 |   205 +
 sim/bpf/aclocal.m4                  |   119 +
 sim/bpf/arch.c                      |    35 +
 sim/bpf/arch.h                      |    50 +
 sim/bpf/bpf-helpers.c               |   175 +
 sim/bpf/bpf-helpers.def             |   194 +
 sim/bpf/bpf-helpers.h               |    31 +
 sim/bpf/bpf-sim.h                   |    31 +
 sim/bpf/bpf.c                       |   327 +
 sim/bpf/config.in                   |   248 +
 sim/bpf/configure                   | 15942 ++++++++++++++++++++++++++
 sim/bpf/configure.ac                |    13 +
 sim/bpf/cpu.c                       |    69 +
 sim/bpf/cpu.h                       |    81 +
 sim/bpf/cpuall.h                    |    65 +
 sim/bpf/decode-be.c                 |  1129 ++
 sim/bpf/decode-be.h                 |    94 +
 sim/bpf/decode-le.c                 |  1129 ++
 sim/bpf/decode-le.h                 |    94 +
 sim/bpf/decode.h                    |    37 +
 sim/bpf/defs-be.h                   |   383 +
 sim/bpf/defs-le.h                   |   383 +
 sim/bpf/eng.h                       |    24 +
 sim/bpf/mloop.in                    |   165 +
 sim/bpf/sem-be.c                    |  3207 ++++++
 sim/bpf/sem-le.c                    |  3207 ++++++
 sim/bpf/sim-if.c                    |   216 +
 sim/bpf/sim-main.h                  |    51 +
 sim/bpf/traps.c                     |    33 +
 sim/configure                       |    22 +-
 sim/configure.tgt                   |     3 +
 sim/testsuite/ChangeLog             |    17 +
 sim/testsuite/configure             |    23 +-
 sim/testsuite/sim/bpf/allinsn.exp   |    26 +
 sim/testsuite/sim/bpf/alu.s         |   109 +
 sim/testsuite/sim/bpf/alu32.s       |    99 +
 sim/testsuite/sim/bpf/endbe.s       |    46 +
 sim/testsuite/sim/bpf/endle.s       |    43 +
 sim/testsuite/sim/bpf/jmp.s         |   120 +
 sim/testsuite/sim/bpf/jmp32.s       |   120 +
 sim/testsuite/sim/bpf/ldabs.s       |    87 +
 sim/testsuite/sim/bpf/mem.s         |    56 +
 sim/testsuite/sim/bpf/mov.s         |    54 +
 sim/testsuite/sim/bpf/testutils.inc |    38 +
 sim/testsuite/sim/bpf/xadd.s        |    44 +
 54 files changed, 29099 insertions(+), 5 deletions(-)
 create mode 100644 gdb/bpf-tdep.c
 create mode 100644 gdb/bpf-tdep.h
 create mode 100644 sim/bpf/Makefile.in
 create mode 100644 sim/bpf/aclocal.m4
 create mode 100644 sim/bpf/arch.c
 create mode 100644 sim/bpf/arch.h
 create mode 100644 sim/bpf/bpf-helpers.c
 create mode 100644 sim/bpf/bpf-helpers.def
 create mode 100644 sim/bpf/bpf-helpers.h
 create mode 100644 sim/bpf/bpf-sim.h
 create mode 100644 sim/bpf/bpf.c
 create mode 100644 sim/bpf/config.in
 create mode 100755 sim/bpf/configure
 create mode 100644 sim/bpf/configure.ac
 create mode 100644 sim/bpf/cpu.c
 create mode 100644 sim/bpf/cpu.h
 create mode 100644 sim/bpf/cpuall.h
 create mode 100644 sim/bpf/decode-be.c
 create mode 100644 sim/bpf/decode-be.h
 create mode 100644 sim/bpf/decode-le.c
 create mode 100644 sim/bpf/decode-le.h
 create mode 100644 sim/bpf/decode.h
 create mode 100644 sim/bpf/defs-be.h
 create mode 100644 sim/bpf/defs-le.h
 create mode 100644 sim/bpf/eng.h
 create mode 100644 sim/bpf/mloop.in
 create mode 100644 sim/bpf/sem-be.c
 create mode 100644 sim/bpf/sem-le.c
 create mode 100644 sim/bpf/sim-if.c
 create mode 100644 sim/bpf/sim-main.h
 create mode 100644 sim/bpf/traps.c
 create mode 100644 sim/testsuite/sim/bpf/allinsn.exp
 create mode 100644 sim/testsuite/sim/bpf/alu.s
 create mode 100644 sim/testsuite/sim/bpf/alu32.s
 create mode 100644 sim/testsuite/sim/bpf/endbe.s
 create mode 100644 sim/testsuite/sim/bpf/endle.s
 create mode 100644 sim/testsuite/sim/bpf/jmp.s
 create mode 100644 sim/testsuite/sim/bpf/jmp32.s
 create mode 100644 sim/testsuite/sim/bpf/ldabs.s
 create mode 100644 sim/testsuite/sim/bpf/mem.s
 create mode 100644 sim/testsuite/sim/bpf/mov.s
 create mode 100644 sim/testsuite/sim/bpf/testutils.inc
 create mode 100644 sim/testsuite/sim/bpf/xadd.s

-- 
2.25.0.2.g232378479e


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] gdb: support for eBPF
  2020-07-03 10:37 [PATCH 0/2] eBPF support Jose E. Marchesi
@ 2020-07-03 10:37 ` Jose E. Marchesi
  2020-07-03 11:39   ` Andrew Burgess
  2020-07-03 11:49   ` Eli Zaretskii
  0 siblings, 2 replies; 6+ messages in thread
From: Jose E. Marchesi @ 2020-07-03 10:37 UTC (permalink / raw)
  To: gdb-patches

This patch adds basic support for the eBPF target: tdep and build
machinery.  The accompanying simulator is introduced in subsequent
patches.

gdb/ChangeLog:

2020-07-03  Weimin Pan <weimin.pan@oracle.com>
	    Jose E. Marchesi  <jose.marchesi@oracle.com>

	* configure.tgt: Add entry for bpf-*-*.
	* Makefile.in (ALL_TARGET_OBS): Add bpf-tdep.o
	(HFILES_NO_SRCDIR): Add bpf-tdep.h.
	(ALLDEPFILES): Add bpf-tdep.c.
	* bpf-tdep.h: New file.
	* bpf-tdep.c: Likewise.

gdb/doc/ChangeLog:

2020-07-03  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* gdb.texinfo (Contributors): Add information for the eBPF
	support.
	(BPF): New section.
---
 gdb/ChangeLog       |  10 ++
 gdb/Makefile.in     |   3 +
 gdb/bpf-tdep.c      | 331 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/bpf-tdep.h      |  46 ++++++
 gdb/configure.tgt   |   6 +
 gdb/doc/ChangeLog   |   6 +
 gdb/doc/gdb.texinfo |  21 +++
 7 files changed, 423 insertions(+)
 create mode 100644 gdb/bpf-tdep.c
 create mode 100644 gdb/bpf-tdep.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 9ae9fe2d1e..bce77b693d 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -717,6 +717,7 @@ ALL_TARGET_OBS = \
 	avr-tdep.o \
 	bfin-linux-tdep.o \
 	bfin-tdep.o \
+	bpf-tdep.o \
 	bsd-uthread.o \
 	cris-linux-tdep.o \
 	cris-tdep.o \
@@ -1221,6 +1222,7 @@ HFILES_NO_SRCDIR = \
 	bcache.h \
 	bfd-target.h \
 	bfin-tdep.h \
+	bpf-tdep.h \
 	block.h \
 	breakpoint.h \
 	bsd-kvm.h \
@@ -2145,6 +2147,7 @@ ALLDEPFILES = \
 	avr-tdep.c \
 	bfin-linux-tdep.c \
 	bfin-tdep.c \
+	bpf-tdep.c \
 	bsd-kvm.c \
 	bsd-uthread.c \
 	csky-linux-tdep.c \
diff --git a/gdb/bpf-tdep.c b/gdb/bpf-tdep.c
new file mode 100644
index 0000000000..ebaf8f8cf3
--- /dev/null
+++ b/gdb/bpf-tdep.c
@@ -0,0 +1,331 @@
+/* Target-dependent code for eBPF.
+
+   Copyright (C) 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/>.  */
+
+#include "defs.h"
+#include "arch-utils.h"
+#include "dis-asm.h"
+#include "frame.h"
+#include "frame-unwind.h"
+#include "trad-frame.h"
+#include "symtab.h"
+#include "value.h"
+#include "gdbcmd.h"
+#include "breakpoint.h"
+#include "inferior.h"
+#include "regcache.h"
+#include "target.h"
+#include "dwarf2/frame.h"
+#include "osabi.h"
+#include "target-descriptions.h"
+#include "bpf-tdep.h"
+#include "remote.h"
+
+\f
+static unsigned int ebpf_debug_flag = 0;
+
+static void ATTRIBUTE_PRINTF (1, 2)
+ebpf_debug (const char *fmt, ...)
+{
+  if (ebpf_debug_flag)
+    {
+       va_list args;
+
+       va_start (args, fmt);
+       printf_unfiltered ("eBPF: ");
+       vprintf_unfiltered (fmt, args);
+       va_end (args);
+    }
+}
+
+\f
+/* eBPF registers */
+
+static const char *ebpf_register_names[] =
+{
+  "r0",   "r1",  "r2",    "r3",   "r4",   "r5",   "r6",   "r7",
+  "r8",   "r9",  "r10",   "pc"
+};
+
+/* Return the name of register REGNUM.  */
+
+static const char *
+ebpf_register_name (struct gdbarch *gdbarch, int reg)
+{
+  if (reg >= 0 && reg < EBPF_NUM_REGS)
+    return ebpf_register_names[reg];
+  return NULL;
+}
+
+/* Return the GDB type of register REGNUM.  */
+
+static struct type *
+ebpf_register_type (struct gdbarch *gdbarch, int reg)
+{
+  if (reg == EBPF_R10_REGNUM)
+    return builtin_type (gdbarch)->builtin_data_ptr;
+  else if (reg == EBPF_PC_REGNUM)
+    return builtin_type (gdbarch)->builtin_func_ptr;
+  return builtin_type (gdbarch)->builtin_int64;
+}
+
+/* Return the GDB register number corresponding to DWARF's REG.  */
+
+static int
+ebpf_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
+{
+  if (reg >= 0 && reg < EBPF_NUM_REGS)
+    return reg;
+  return -1;
+}
+
+/* Implement the "print_insn" gdbarch method.  */
+
+static int
+bpf_gdb_print_insn (bfd_vma memaddr, disassemble_info *info)
+{
+  info->symbols = NULL;
+  return default_print_insn (memaddr, info);
+}
+
+\f
+/* Implement the "unwind_pc" gdbarch method.  */
+
+static CORE_ADDR
+ebpf_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
+{
+  return frame_unwind_register_unsigned (next_frame,
+					 gdbarch_pc_regnum (gdbarch));
+}
+
+/* Return PC of first real instruction of the function starting at
+   START_PC.  */
+
+static CORE_ADDR
+ebpf_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
+{
+  ebpf_debug ("Skipping prologue: start_pc=%s\n",
+	      paddress (gdbarch, start_pc));
+  /* XXX */
+  return start_pc+0;
+}
+
+\f
+/* Frame unwinder.  */
+
+static void
+bpf_frame_this_id (struct frame_info *this_frame,
+		   void **this_prologue_cache,
+		   struct frame_id *this_id)
+{
+  /* XXX */
+}
+
+static enum unwind_stop_reason
+bpf_frame_unwind_stop_reason (struct frame_info *this_frame,
+			      void **this_cache)
+{
+  return UNWIND_OUTERMOST; /* XXX */
+}
+
+static struct value *
+bpf_frame_prev_register (struct frame_info *this_frame,
+			 void **this_prologue_cache, int regnum)
+{
+  return frame_unwind_got_register (this_frame, regnum, regnum); /* XXX */
+}
+
+static const struct frame_unwind bpf_frame_unwind =
+{
+ NORMAL_FRAME,
+ bpf_frame_unwind_stop_reason,
+ bpf_frame_this_id,
+ bpf_frame_prev_register,
+ NULL,
+ default_frame_sniffer
+};
+
+\f
+/* Breakpoints.  */
+
+/* Implement the breakpoint_kind_from_pc gdbarch method.  */
+
+static int
+ebpf_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *start_pc)
+{
+  /* We support just one kind of breakpoint.  */
+  return 8;
+}
+
+/* Implement the sw_breakpoint_from_kind gdbarch method.  */
+
+static const gdb_byte *
+ebpf_sw_breakpoint_from_kind (struct gdbarch *gdbarch, int kind, int *size)
+{
+  static unsigned char ebpf_breakpoint[]
+    = {0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+
+  *size = kind;
+  return ebpf_breakpoint;
+}
+
+\f
+/* Assuming THIS_FRAME is a dummy frame, return its frame ID.  */
+
+static struct frame_id
+ebpf_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
+{
+  CORE_ADDR sp = get_frame_register_unsigned (this_frame,
+					      gdbarch_sp_regnum (gdbarch));
+  return frame_id_build (sp, get_frame_pc (this_frame));
+}
+
+static CORE_ADDR
+ebpf_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
+		      struct regcache *regcache, CORE_ADDR bp_addr,
+		      int nargs, struct value **args, CORE_ADDR sp,
+		      function_call_return_method return_method,
+		      CORE_ADDR struct_addr)
+{
+  ebpf_debug ("Pushing dummy call: sp=%s\n", paddress (gdbarch, sp));
+  /* XXX */
+  return sp;
+}
+
+/* Extract a function return value of TYPE from REGCACHE,
+   and copy it into VALBUF.  */
+
+static void
+ebpf_extract_return_value (struct type *type, struct regcache *regcache,
+				 gdb_byte *valbuf)
+{
+  int len = TYPE_LENGTH (type);
+  gdb_byte vbuf[8];
+
+  gdb_assert (len <= 8);
+  regcache->cooked_read (EBPF_R0_REGNUM, vbuf);
+  memcpy (valbuf, vbuf + 8 - len, len);
+}
+
+/* Store the function return value of type TYPE from VALBUF into REGNAME.  */
+
+static void
+ebpf_store_return_value (struct type *type, struct regcache *regcache,
+			 const gdb_byte *valbuf)
+{
+  int len = TYPE_LENGTH (type);
+  gdb_byte vbuf[8];
+
+  gdb_assert (len <= 8);
+  memset (vbuf, 0, sizeof(vbuf));
+  memcpy (vbuf + 8 - len, valbuf, len);
+  regcache->cooked_write (EBPF_R0_REGNUM, vbuf);
+}
+
+/* Handle function's return value.  */
+
+static enum return_value_convention
+ebpf_return_value (struct gdbarch *gdbarch, struct value *function,
+			 struct type *type, struct regcache *regcache,
+			 gdb_byte *readbuf, const gdb_byte *writebuf)
+{
+  int len = TYPE_LENGTH (type);
+
+  if (len > 8)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
+  if (readbuf)
+    ebpf_extract_return_value (type, regcache, readbuf);
+  if (writebuf)
+    ebpf_store_return_value (type, regcache, writebuf);
+
+  return RETURN_VALUE_REGISTER_CONVENTION;
+}
+
+\f
+/* Initialize the current architecture based on INFO.  If possible, re-use an
+   architecture from ARCHES, which is a list of architectures already created
+   during this debugging session.  */
+
+static struct gdbarch *
+ebpf_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
+{
+  struct gdbarch_tdep *tdep;
+  struct gdbarch *gdbarch;
+
+  /* If there is already a candidate, use it.  */
+  arches = gdbarch_list_lookup_by_info (arches, &info);
+  if (arches != NULL)
+    return arches->gdbarch;
+
+  /* Allocate space for the new architecture.  */
+  tdep = XCNEW (struct gdbarch_tdep);
+  gdbarch = gdbarch_alloc (&info, tdep);
+
+  /* Information about registers, etc.  */
+  set_gdbarch_num_regs (gdbarch, EBPF_NUM_REGS);
+  set_gdbarch_register_name (gdbarch, ebpf_register_name);
+  set_gdbarch_register_type (gdbarch, ebpf_register_type);
+
+  /* Register numbers of various important registers.  */
+  set_gdbarch_sp_regnum (gdbarch, EBPF_R10_REGNUM);
+  set_gdbarch_pc_regnum (gdbarch, EBPF_PC_REGNUM);
+
+  /* Map DWARF2 registers to GDB registers.  */
+  set_gdbarch_dwarf2_reg_to_regnum (gdbarch, ebpf_dwarf2_reg_to_regnum);
+
+  /* Call dummy code.  */
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_dummy_id (gdbarch, ebpf_dummy_id);
+  set_gdbarch_push_dummy_call (gdbarch, ebpf_push_dummy_call);
+
+  /* Returning results.  */
+  set_gdbarch_return_value (gdbarch, ebpf_return_value);
+
+  /* Advance PC across function entry code.  */
+  set_gdbarch_skip_prologue (gdbarch, ebpf_skip_prologue);
+
+  /* Stack grows downward.  */
+  set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
+
+  /* Breakpoint manipulation.  */
+  set_gdbarch_breakpoint_kind_from_pc (gdbarch, ebpf_breakpoint_kind_from_pc);
+  set_gdbarch_sw_breakpoint_from_kind (gdbarch, ebpf_sw_breakpoint_from_kind);
+
+  /* Frame handling.  */
+  set_gdbarch_frame_args_skip (gdbarch, 8);
+  set_gdbarch_unwind_pc (gdbarch, ebpf_unwind_pc);
+
+  /* Disassembly.  */
+  set_gdbarch_print_insn (gdbarch, bpf_gdb_print_insn);
+
+  /* Hook in ABI-specific overrides, if they have been registered.  */
+  gdbarch_init_osabi (info, gdbarch);
+
+  /* Install unwinders.  */
+  frame_unwind_append_unwinder (gdbarch, &bpf_frame_unwind);
+
+  return gdbarch;
+}
+
+void _initialize_ebpf_tdep ();
+void
+_initialize_ebpf_tdep (void)
+{
+  register_gdbarch_init (bfd_arch_bpf, ebpf_gdbarch_init);
+}
diff --git a/gdb/bpf-tdep.h b/gdb/bpf-tdep.h
new file mode 100644
index 0000000000..3cfab9cd43
--- /dev/null
+++ b/gdb/bpf-tdep.h
@@ -0,0 +1,46 @@
+/* Target-dependent code for eBPF, for GDB.
+
+   Copyright (C) 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 BPF_TDEP_H
+#define BPF_TDEP_H
+
+enum ebpf_regnum {
+  EBPF_R0_REGNUM,		/* return value */
+  EBPF_R1_REGNUM,
+  EBPF_R2_REGNUM,
+  EBPF_R3_REGNUM,
+  EBPF_R4_REGNUM,
+  EBPF_R5_REGNUM,
+  EBPF_R6_REGNUM,
+  EBPF_R7_REGNUM,
+  EBPF_R8_REGNUM,
+  EBPF_R9_REGNUM,
+  EBPF_R10_REGNUM,		/* sp */
+  EBPF_PC_REGNUM,
+};
+
+#define EBPF_NUM_REGS	(EBPF_PC_REGNUM + 1)
+
+/* Target-dependent structure in gdbarch.  */
+struct gdbarch_tdep
+{
+  /* XXX */
+};
+
+#endif /* BPF_TDEP_H */
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index d66f01bb9f..8a26bdeb87 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -205,6 +205,12 @@ bfin-*-*)
 	gdb_sim=../sim/bfin/libsim.a
 	;;
 
+bpf-*-*)
+	# Target: eBPF
+	gdb_target_obs="bpf-tdep.o"
+	gdb_sim=../sim/bpf/libsim.a
+	;;
+
 cris*)
 	# Target: CRIS
 	gdb_target_obs="cris-tdep.o cris-linux-tdep.o linux-tdep.o solib-svr4.o"
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index fbe9f850af..b9df9cdf95 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -557,6 +557,10 @@ Alessandro Forin and Per Bothner.  More recent ports have been the work
 of Jeremy Bennett, Franck Jullien, Stefan Wallentowitz and
 Stafford Horne.
 
+Weimin Pan, David Faust and Jose E. Marchesi contributed support for
+the Linux kernel BPF virtual architecture.  This work was sponsored by
+Oracle.
+
 @node Sample Session
 @chapter A Sample @value{GDBN} Session
 
@@ -24379,6 +24383,7 @@ acceptable commands.
 @menu
 * ARC::                         Synopsys ARC
 * ARM::                         ARM
+* BPF::				eBPF
 * M68K::                        Motorola M68K
 * MicroBlaze::			Xilinx MicroBlaze
 * MIPS Embedded::               MIPS Embedded
@@ -24513,6 +24518,22 @@ The default value is @code{all}.
 @end table
 @end table
 
+@node BPF
+@subsection BPF
+
+@table @code
+@item target sim @r{[}@var{simargs}@r{]} @dots{} 
+The @value{GDBN} BPF simulator accepts the following optional arguments.
+
+@table @code
+@item --skb-data-offset=@var{offset}
+Tell the simulator the offset, measured in bytes, of the
+@code{skb_data} field in the kernel @code{struct sk_buff} structure.
+This offset is used by some BPF specific-purpose load/store
+instructions.  Defaults to 0.
+@end table
+@end table
+
 @node M68K
 @subsection M68k
 
-- 
2.25.0.2.g232378479e


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] gdb: support for eBPF
  2020-07-03 10:37 ` [PATCH 1/2] gdb: support for eBPF Jose E. Marchesi
@ 2020-07-03 11:39   ` Andrew Burgess
  2020-07-06 14:50     ` Jose E. Marchesi
  2020-07-03 11:49   ` Eli Zaretskii
  1 sibling, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2020-07-03 11:39 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: gdb-patches

* Jose E. Marchesi via Gdb-patches <gdb-patches@sourceware.org> [2020-07-03 12:37:12 +0200]:

> This patch adds basic support for the eBPF target: tdep and build
> machinery.  The accompanying simulator is introduced in subsequent
> patches.
> 
> gdb/ChangeLog:
> 
> 2020-07-03  Weimin Pan <weimin.pan@oracle.com>
> 	    Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* configure.tgt: Add entry for bpf-*-*.
> 	* Makefile.in (ALL_TARGET_OBS): Add bpf-tdep.o
> 	(HFILES_NO_SRCDIR): Add bpf-tdep.h.
> 	(ALLDEPFILES): Add bpf-tdep.c.
> 	* bpf-tdep.h: New file.
> 	* bpf-tdep.c: Likewise.
> 
> gdb/doc/ChangeLog:
> 
> 2020-07-03  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* gdb.texinfo (Contributors): Add information for the eBPF
> 	support.
> 	(BPF): New section.
> ---
>  gdb/ChangeLog       |  10 ++
>  gdb/Makefile.in     |   3 +
>  gdb/bpf-tdep.c      | 331 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/bpf-tdep.h      |  46 ++++++
>  gdb/configure.tgt   |   6 +
>  gdb/doc/ChangeLog   |   6 +
>  gdb/doc/gdb.texinfo |  21 +++
>  7 files changed, 423 insertions(+)
>  create mode 100644 gdb/bpf-tdep.c
>  create mode 100644 gdb/bpf-tdep.h
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9ae9fe2d1e..bce77b693d 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -717,6 +717,7 @@ ALL_TARGET_OBS = \
>  	avr-tdep.o \
>  	bfin-linux-tdep.o \
>  	bfin-tdep.o \
> +	bpf-tdep.o \
>  	bsd-uthread.o \
>  	cris-linux-tdep.o \
>  	cris-tdep.o \
> @@ -1221,6 +1222,7 @@ HFILES_NO_SRCDIR = \
>  	bcache.h \
>  	bfd-target.h \
>  	bfin-tdep.h \
> +	bpf-tdep.h \
>  	block.h \
>  	breakpoint.h \
>  	bsd-kvm.h \
> @@ -2145,6 +2147,7 @@ ALLDEPFILES = \
>  	avr-tdep.c \
>  	bfin-linux-tdep.c \
>  	bfin-tdep.c \
> +	bpf-tdep.c \
>  	bsd-kvm.c \
>  	bsd-uthread.c \
>  	csky-linux-tdep.c \
> diff --git a/gdb/bpf-tdep.c b/gdb/bpf-tdep.c
> new file mode 100644
> index 0000000000..ebaf8f8cf3
> --- /dev/null
> +++ b/gdb/bpf-tdep.c
> @@ -0,0 +1,331 @@
> +/* Target-dependent code for eBPF.
> +
> +   Copyright (C) 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/>.  */
> +
> +#include "defs.h"
> +#include "arch-utils.h"
> +#include "dis-asm.h"
> +#include "frame.h"
> +#include "frame-unwind.h"
> +#include "trad-frame.h"
> +#include "symtab.h"
> +#include "value.h"
> +#include "gdbcmd.h"
> +#include "breakpoint.h"
> +#include "inferior.h"
> +#include "regcache.h"
> +#include "target.h"
> +#include "dwarf2/frame.h"
> +#include "osabi.h"
> +#include "target-descriptions.h"
> +#include "bpf-tdep.h"
> +#include "remote.h"
> +
> +\f
> +static unsigned int ebpf_debug_flag = 0;
> +
> +static void ATTRIBUTE_PRINTF (1, 2)
> +ebpf_debug (const char *fmt, ...)
> +{
> +  if (ebpf_debug_flag)
> +    {
> +       va_list args;
> +
> +       va_start (args, fmt);
> +       printf_unfiltered ("eBPF: ");
> +       vprintf_unfiltered (fmt, args);
> +       va_end (args);
> +    }
> +}

Every top level item function, variable, struct, etc, should have a
comment on it, you've mostly done this, but there's a few places where
these are missing, I'll not point them all out.

You might want to consider changing this into a real debug flag so
that there's a 'set debug ebpf' option.  For RISC-V I went one further
and added sub-flags like 'set debug riscv unwinders'.  Not a
requirement I think, but maybe worth considering.

> +
> +\f
> +/* eBPF registers */
> +
> +static const char *ebpf_register_names[] =
> +{
> +  "r0",   "r1",  "r2",    "r3",   "r4",   "r5",   "r6",   "r7",
> +  "r8",   "r9",  "r10",   "pc"
> +};
> +
> +/* Return the name of register REGNUM.  */
> +
> +static const char *
> +ebpf_register_name (struct gdbarch *gdbarch, int reg)
> +{
> +  if (reg >= 0 && reg < EBPF_NUM_REGS)
> +    return ebpf_register_names[reg];
> +  return NULL;
> +}
> +
> +/* Return the GDB type of register REGNUM.  */
> +
> +static struct type *
> +ebpf_register_type (struct gdbarch *gdbarch, int reg)
> +{
> +  if (reg == EBPF_R10_REGNUM)
> +    return builtin_type (gdbarch)->builtin_data_ptr;
> +  else if (reg == EBPF_PC_REGNUM)
> +    return builtin_type (gdbarch)->builtin_func_ptr;
> +  return builtin_type (gdbarch)->builtin_int64;
> +}
> +
> +/* Return the GDB register number corresponding to DWARF's REG.  */
> +
> +static int
> +ebpf_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> +{
> +  if (reg >= 0 && reg < EBPF_NUM_REGS)
> +    return reg;
> +  return -1;
> +}
> +
> +/* Implement the "print_insn" gdbarch method.  */
> +
> +static int
> +bpf_gdb_print_insn (bfd_vma memaddr, disassemble_info *info)
> +{
> +  info->symbols = NULL;
> +  return default_print_insn (memaddr, info);
> +}
> +
> +\f
> +/* Implement the "unwind_pc" gdbarch method.  */
> +
> +static CORE_ADDR
> +ebpf_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  return frame_unwind_register_unsigned (next_frame,
> +					 gdbarch_pc_regnum (gdbarch));
> +}

Given the implementation I think you shouldn't need this function,
default_unwind_pc should do just fine for you.

> +
> +/* Return PC of first real instruction of the function starting at
> +   START_PC.  */
> +
> +static CORE_ADDR
> +ebpf_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
> +{
> +  ebpf_debug ("Skipping prologue: start_pc=%s\n",
> +	      paddress (gdbarch, start_pc));
> +  /* XXX */

You've got these 'XXX' comments though out.  I think they mean
different things in different places.  I think these should either be
removed, or converted into real text.  In some places you might want
to consider making use of 'internal_error (....)' so GDB will given
some indication that you've run into an area what needs implementing,
in other cases, just expanding the comment to indicate that the
current code isn't final will be the correct thing to do.

> +  return start_pc+0;

Whitespace around the '+'.

> +}
> +
> +\f
> +/* Frame unwinder.  */
> +
> +static void
> +bpf_frame_this_id (struct frame_info *this_frame,
> +		   void **this_prologue_cache,
> +		   struct frame_id *this_id)
> +{
> +  /* XXX */
> +}
> +
> +static enum unwind_stop_reason
> +bpf_frame_unwind_stop_reason (struct frame_info *this_frame,
> +			      void **this_cache)
> +{
> +  return UNWIND_OUTERMOST; /* XXX */
> +}
> +
> +static struct value *
> +bpf_frame_prev_register (struct frame_info *this_frame,
> +			 void **this_prologue_cache, int regnum)
> +{
> +  return frame_unwind_got_register (this_frame, regnum, regnum); /* XXX */
> +}
> +
> +static const struct frame_unwind bpf_frame_unwind =
> +{
> + NORMAL_FRAME,
> + bpf_frame_unwind_stop_reason,
> + bpf_frame_this_id,
> + bpf_frame_prev_register,
> + NULL,
> + default_frame_sniffer
> +};
> +
> +\f
> +/* Breakpoints.  */
> +
> +/* Implement the breakpoint_kind_from_pc gdbarch method.  */
> +
> +static int
> +ebpf_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *start_pc)
> +{
> +  /* We support just one kind of breakpoint.  */
> +  return 8;
> +}
> +
> +/* Implement the sw_breakpoint_from_kind gdbarch method.  */
> +
> +static const gdb_byte *
> +ebpf_sw_breakpoint_from_kind (struct gdbarch *gdbarch, int kind, int *size)
> +{
> +  static unsigned char ebpf_breakpoint[]
> +    = {0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +
> +  *size = kind;
> +  return ebpf_breakpoint;
> +}
> +
> +\f
> +/* Assuming THIS_FRAME is a dummy frame, return its frame ID.  */
> +
> +static struct frame_id
> +ebpf_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
> +{
> +  CORE_ADDR sp = get_frame_register_unsigned (this_frame,
> +					      gdbarch_sp_regnum (gdbarch));
> +  return frame_id_build (sp, get_frame_pc (this_frame));
> +}
> +
> +static CORE_ADDR
> +ebpf_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> +		      struct regcache *regcache, CORE_ADDR bp_addr,
> +		      int nargs, struct value **args, CORE_ADDR sp,
> +		      function_call_return_method return_method,
> +		      CORE_ADDR struct_addr)
> +{
> +  ebpf_debug ("Pushing dummy call: sp=%s\n", paddress (gdbarch, sp));
> +  /* XXX */
> +  return sp;
> +}
> +
> +/* Extract a function return value of TYPE from REGCACHE,
> +   and copy it into VALBUF.  */
> +
> +static void
> +ebpf_extract_return_value (struct type *type, struct regcache *regcache,
> +				 gdb_byte *valbuf)
> +{
> +  int len = TYPE_LENGTH (type);
> +  gdb_byte vbuf[8];
> +
> +  gdb_assert (len <= 8);
> +  regcache->cooked_read (EBPF_R0_REGNUM, vbuf);
> +  memcpy (valbuf, vbuf + 8 - len, len);
> +}
> +
> +/* Store the function return value of type TYPE from VALBUF into REGNAME.  */
> +
> +static void
> +ebpf_store_return_value (struct type *type, struct regcache *regcache,
> +			 const gdb_byte *valbuf)
> +{
> +  int len = TYPE_LENGTH (type);
> +  gdb_byte vbuf[8];
> +
> +  gdb_assert (len <= 8);
> +  memset (vbuf, 0, sizeof(vbuf));

Whitespace after 'sizeof' here.

> +  memcpy (vbuf + 8 - len, valbuf, len);
> +  regcache->cooked_write (EBPF_R0_REGNUM, vbuf);
> +}
> +
> +/* Handle function's return value.  */
> +
> +static enum return_value_convention
> +ebpf_return_value (struct gdbarch *gdbarch, struct value *function,
> +			 struct type *type, struct regcache *regcache,
> +			 gdb_byte *readbuf, const gdb_byte *writebuf)
> +{
> +  int len = TYPE_LENGTH (type);
> +
> +  if (len > 8)
> +    return RETURN_VALUE_STRUCT_CONVENTION;
> +
> +  if (readbuf)
> +    ebpf_extract_return_value (type, regcache, readbuf);
> +  if (writebuf)
> +    ebpf_store_return_value (type, regcache, writebuf);

GDB style is to compare pointers to NULL or nullptr, not to just use
them as bools.

Thanks,
Andrew


> +
> +  return RETURN_VALUE_REGISTER_CONVENTION;
> +}
> +
> +\f
> +/* Initialize the current architecture based on INFO.  If possible, re-use an
> +   architecture from ARCHES, which is a list of architectures already created
> +   during this debugging session.  */
> +
> +static struct gdbarch *
> +ebpf_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> +  struct gdbarch_tdep *tdep;
> +  struct gdbarch *gdbarch;
> +
> +  /* If there is already a candidate, use it.  */
> +  arches = gdbarch_list_lookup_by_info (arches, &info);
> +  if (arches != NULL)
> +    return arches->gdbarch;
> +
> +  /* Allocate space for the new architecture.  */
> +  tdep = XCNEW (struct gdbarch_tdep);
> +  gdbarch = gdbarch_alloc (&info, tdep);
> +
> +  /* Information about registers, etc.  */
> +  set_gdbarch_num_regs (gdbarch, EBPF_NUM_REGS);
> +  set_gdbarch_register_name (gdbarch, ebpf_register_name);
> +  set_gdbarch_register_type (gdbarch, ebpf_register_type);
> +
> +  /* Register numbers of various important registers.  */
> +  set_gdbarch_sp_regnum (gdbarch, EBPF_R10_REGNUM);
> +  set_gdbarch_pc_regnum (gdbarch, EBPF_PC_REGNUM);
> +
> +  /* Map DWARF2 registers to GDB registers.  */
> +  set_gdbarch_dwarf2_reg_to_regnum (gdbarch, ebpf_dwarf2_reg_to_regnum);
> +
> +  /* Call dummy code.  */
> +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> +  set_gdbarch_dummy_id (gdbarch, ebpf_dummy_id);
> +  set_gdbarch_push_dummy_call (gdbarch, ebpf_push_dummy_call);
> +
> +  /* Returning results.  */
> +  set_gdbarch_return_value (gdbarch, ebpf_return_value);
> +
> +  /* Advance PC across function entry code.  */
> +  set_gdbarch_skip_prologue (gdbarch, ebpf_skip_prologue);
> +
> +  /* Stack grows downward.  */
> +  set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
> +
> +  /* Breakpoint manipulation.  */
> +  set_gdbarch_breakpoint_kind_from_pc (gdbarch, ebpf_breakpoint_kind_from_pc);
> +  set_gdbarch_sw_breakpoint_from_kind (gdbarch, ebpf_sw_breakpoint_from_kind);
> +
> +  /* Frame handling.  */
> +  set_gdbarch_frame_args_skip (gdbarch, 8);
> +  set_gdbarch_unwind_pc (gdbarch, ebpf_unwind_pc);
> +
> +  /* Disassembly.  */
> +  set_gdbarch_print_insn (gdbarch, bpf_gdb_print_insn);
> +
> +  /* Hook in ABI-specific overrides, if they have been registered.  */
> +  gdbarch_init_osabi (info, gdbarch);
> +
> +  /* Install unwinders.  */
> +  frame_unwind_append_unwinder (gdbarch, &bpf_frame_unwind);
> +
> +  return gdbarch;
> +}
> +
> +void _initialize_ebpf_tdep ();
> +void
> +_initialize_ebpf_tdep (void)
> +{
> +  register_gdbarch_init (bfd_arch_bpf, ebpf_gdbarch_init);
> +}
> diff --git a/gdb/bpf-tdep.h b/gdb/bpf-tdep.h
> new file mode 100644
> index 0000000000..3cfab9cd43
> --- /dev/null
> +++ b/gdb/bpf-tdep.h
> @@ -0,0 +1,46 @@
> +/* Target-dependent code for eBPF, for GDB.
> +
> +   Copyright (C) 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 BPF_TDEP_H
> +#define BPF_TDEP_H
> +
> +enum ebpf_regnum {
> +  EBPF_R0_REGNUM,		/* return value */
> +  EBPF_R1_REGNUM,
> +  EBPF_R2_REGNUM,
> +  EBPF_R3_REGNUM,
> +  EBPF_R4_REGNUM,
> +  EBPF_R5_REGNUM,
> +  EBPF_R6_REGNUM,
> +  EBPF_R7_REGNUM,
> +  EBPF_R8_REGNUM,
> +  EBPF_R9_REGNUM,
> +  EBPF_R10_REGNUM,		/* sp */
> +  EBPF_PC_REGNUM,
> +};
> +
> +#define EBPF_NUM_REGS	(EBPF_PC_REGNUM + 1)
> +
> +/* Target-dependent structure in gdbarch.  */
> +struct gdbarch_tdep
> +{
> +  /* XXX */
> +};
> +
> +#endif /* BPF_TDEP_H */
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index d66f01bb9f..8a26bdeb87 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -205,6 +205,12 @@ bfin-*-*)
>  	gdb_sim=../sim/bfin/libsim.a
>  	;;
>  
> +bpf-*-*)
> +	# Target: eBPF
> +	gdb_target_obs="bpf-tdep.o"
> +	gdb_sim=../sim/bpf/libsim.a
> +	;;
> +
>  cris*)
>  	# Target: CRIS
>  	gdb_target_obs="cris-tdep.o cris-linux-tdep.o linux-tdep.o solib-svr4.o"
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index fbe9f850af..b9df9cdf95 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -557,6 +557,10 @@ Alessandro Forin and Per Bothner.  More recent ports have been the work
>  of Jeremy Bennett, Franck Jullien, Stefan Wallentowitz and
>  Stafford Horne.
>  
> +Weimin Pan, David Faust and Jose E. Marchesi contributed support for
> +the Linux kernel BPF virtual architecture.  This work was sponsored by
> +Oracle.
> +
>  @node Sample Session
>  @chapter A Sample @value{GDBN} Session
>  
> @@ -24379,6 +24383,7 @@ acceptable commands.
>  @menu
>  * ARC::                         Synopsys ARC
>  * ARM::                         ARM
> +* BPF::				eBPF
>  * M68K::                        Motorola M68K
>  * MicroBlaze::			Xilinx MicroBlaze
>  * MIPS Embedded::               MIPS Embedded
> @@ -24513,6 +24518,22 @@ The default value is @code{all}.
>  @end table
>  @end table
>  
> +@node BPF
> +@subsection BPF
> +
> +@table @code
> +@item target sim @r{[}@var{simargs}@r{]} @dots{} 
> +The @value{GDBN} BPF simulator accepts the following optional arguments.
> +
> +@table @code
> +@item --skb-data-offset=@var{offset}
> +Tell the simulator the offset, measured in bytes, of the
> +@code{skb_data} field in the kernel @code{struct sk_buff} structure.
> +This offset is used by some BPF specific-purpose load/store
> +instructions.  Defaults to 0.
> +@end table
> +@end table
> +
>  @node M68K
>  @subsection M68k
>  
> -- 
> 2.25.0.2.g232378479e
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] gdb: support for eBPF
  2020-07-03 10:37 ` [PATCH 1/2] gdb: support for eBPF Jose E. Marchesi
  2020-07-03 11:39   ` Andrew Burgess
@ 2020-07-03 11:49   ` Eli Zaretskii
  2020-07-06 14:51     ` Jose E. Marchesi
  1 sibling, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2020-07-03 11:49 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: gdb-patches

> Date: Fri,  3 Jul 2020 12:37:12 +0200
> From: "Jose E. Marchesi via Gdb-patches" <gdb-patches@sourceware.org>
> 
> This patch adds basic support for the eBPF target: tdep and build
> machinery.  The accompanying simulator is introduced in subsequent
> patches.
> 
> gdb/ChangeLog:
> 
> 2020-07-03  Weimin Pan <weimin.pan@oracle.com>
> 	    Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* configure.tgt: Add entry for bpf-*-*.
> 	* Makefile.in (ALL_TARGET_OBS): Add bpf-tdep.o
> 	(HFILES_NO_SRCDIR): Add bpf-tdep.h.
> 	(ALLDEPFILES): Add bpf-tdep.c.
> 	* bpf-tdep.h: New file.
> 	* bpf-tdep.c: Likewise.
> 
> gdb/doc/ChangeLog:
> 
> 2020-07-03  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* gdb.texinfo (Contributors): Add information for the eBPF
> 	support.
> 	(BPF): New section.

Thanks, the documentation part is OK.

Should this be in NEWS?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] gdb: support for eBPF
  2020-07-03 11:39   ` Andrew Burgess
@ 2020-07-06 14:50     ` Jose E. Marchesi
  0 siblings, 0 replies; 6+ messages in thread
From: Jose E. Marchesi @ 2020-07-06 14:50 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches



Hi Andrew.
Thanks for the review.

    > +\f
    > +static unsigned int ebpf_debug_flag = 0;
    > +
    > +static void ATTRIBUTE_PRINTF (1, 2)
    > +ebpf_debug (const char *fmt, ...)
    > +{
    > +  if (ebpf_debug_flag)
    > +    {
    > +       va_list args;
    > +
    > +       va_start (args, fmt);
    > +       printf_unfiltered ("eBPF: ");
    > +       vprintf_unfiltered (fmt, args);
    > +       va_end (args);
    > +    }
    > +}
    
    Every top level item function, variable, struct, etc, should have a
    comment on it, you've mostly done this, but there's a few places where
    these are missing, I'll not point them all out.

Ok, I will add the missing descriptive comments.

    You might want to consider changing this into a real debug flag so
    that there's a 'set debug ebpf' option.  For RISC-V I went one further
    and added sub-flags like 'set debug riscv unwinders'.  Not a
    requirement I think, but maybe worth considering.

Ok.

    > +/* Implement the "unwind_pc" gdbarch method.  */
    > +
    > +static CORE_ADDR
    > +ebpf_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
    > +{
    > +  return frame_unwind_register_unsigned (next_frame,
    > +					 gdbarch_pc_regnum (gdbarch));
    > +}
    
    Given the implementation I think you shouldn't need this function,
    default_unwind_pc should do just fine for you.

Ok, will remove it.

    > +/* Return PC of first real instruction of the function starting at
    > +   START_PC.  */
    > +
    > +static CORE_ADDR
    > +ebpf_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
    > +{
    > +  ebpf_debug ("Skipping prologue: start_pc=%s\n",
    > +	      paddress (gdbarch, start_pc));
    > +  /* XXX */
    
    You've got these 'XXX' comments though out.  I think they mean
    different things in different places.  I think these should either be
    removed, or converted into real text.  In some places you might want
    to consider making use of 'internal_error (....)' so GDB will given
    some indication that you've run into an area what needs implementing,
    in other cases, just expanding the comment to indicate that the
    current code isn't final will be the correct thing to do.

Yeah, these comments are definitely too terse :)
Will fix in a subsequent version of the patches.

Thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] gdb: support for eBPF
  2020-07-03 11:49   ` Eli Zaretskii
@ 2020-07-06 14:51     ` Jose E. Marchesi
  0 siblings, 0 replies; 6+ messages in thread
From: Jose E. Marchesi @ 2020-07-06 14:51 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches


    > 2020-07-03  Jose E. Marchesi  <jose.marchesi@oracle.com>
    > 
    > 	* gdb.texinfo (Contributors): Add information for the eBPF
    > 	support.
    > 	(BPF): New section.
    
    Thanks, the documentation part is OK.
    
    Should this be in NEWS?

I guess so, since it is a new target.
Will add an entry in subsequent version of the series.

Thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-06 14:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-03 10:37 [PATCH 0/2] eBPF support Jose E. Marchesi
2020-07-03 10:37 ` [PATCH 1/2] gdb: support for eBPF Jose E. Marchesi
2020-07-03 11:39   ` Andrew Burgess
2020-07-06 14:50     ` Jose E. Marchesi
2020-07-03 11:49   ` Eli Zaretskii
2020-07-06 14:51     ` Jose E. Marchesi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).