[PATCH V4 1/3] gdb: support for eBPF

[Simon Marchi]

Hi Jose,

I have noted a few mostly stylistic comments.  I don't know much about eBPF, so I can't
really comment on the behavior.

> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9d48445739..23c09ff84f 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -718,6 +718,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 \
> @@ -1222,6 +1223,7 @@ HFILES_NO_SRCDIR = \
>  	bcache.h \
>  	bfd-target.h \
>  	bfin-tdep.h \
> +	bpf-tdep.h \
>  	block.h \

bpf-tdep.h should go after block.h.

> +/* Frame unwinder.
> +
> +   XXX it is not clear how to unwind in eBPF, since the stack is not
> +   guaranteed to be contiguous, and therefore no relative stack
> +   addressing can be done in the callee in order to access the
> +   caller's stack frame.  To explore with xBPF, which will relax this
> +   restriction.  */
> +
> +/* Given THIS_FRAME, return its ID.  */
> +
> +static void
> +bpf_frame_this_id (struct frame_info *this_frame,
> +		   void **this_prologue_cache,
> +		   struct frame_id *this_id)
> +{
> +}

So if I understand correctly, at the moment there's no unwinding at all,
there's always just one frame?  What ID does that frame have?  Would it
still be good to assign something (constant) to *THIS_ID to make sure it's
not random junk?

> +
> +/* Return the reason why we can't unwind past THIS_FRAME.  */
> +
> +static enum unwind_stop_reason
> +bpf_frame_unwind_stop_reason (struct frame_info *this_frame,
> +			      void **this_cache)
> +{
> +  return UNWIND_OUTERMOST;
> +}
> +
> +/* Ask THIS_FRAME to unwind its register.  */
> +
> +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);
> +}
> +
> +/* Frame unwinder machinery for BPF.  */
> +
> +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

Use two spaces for indent here.

> +};
> +
> +

> +/* Breakpoints.  */
> +
> +/* Implement the breakpoint_kind_from_pc gdbarch method.  */
> +
> +static int
> +bpf_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 *
> +bpf_sw_breakpoint_from_kind (struct gdbarch *gdbarch, int kind, int *size)
> +{
> +  static unsigned char bpf_breakpoint[]
> +    = {0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +
> +  *size = kind;
> +  return bpf_breakpoint;

Just to be sure: the kind does not have the be the size of the breakpoint.  Some
architectures do that, but it's not the only way.  You could define that the only
existing breakpoint kind is 0, for example.

Anyway, that can all change later, as long as it's not exposed to GDBserver.

> +}
> +
> +

> +/* Assuming THIS_FRAME is a dummy frame, return its frame ID.  */
> +
> +static struct frame_id
> +bpf_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));
> +}
> +
> +/* Implement the push dummy call gdbarch callback.  */
> +
> +static CORE_ADDR
> +bpf_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)

The wrapped parameter list should be aligned with the first argument.  There
are a few more instances of this in the patch.

> +/* 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 *
> +bpf_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> +  struct gdbarch_tdep *tdep;
> +  struct gdbarch *gdbarch;

Declare these on first use.

> diff --git a/gdb/bpf-tdep.h b/gdb/bpf-tdep.h
> new file mode 100644
> index 0000000000..48a070a24d
> --- /dev/null
> +++ b/gdb/bpf-tdep.h
> @@ -0,0 +1,45 @@
> +/* Target-dependent code for BPF, 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 bpf_regnum {

{ on next line.

> +  BPF_R0_REGNUM,		/* return value */
> +  BPF_R1_REGNUM,
> +  BPF_R2_REGNUM,
> +  BPF_R3_REGNUM,
> +  BPF_R4_REGNUM,
> +  BPF_R5_REGNUM,
> +  BPF_R6_REGNUM,
> +  BPF_R7_REGNUM,
> +  BPF_R8_REGNUM,
> +  BPF_R9_REGNUM,
> +  BPF_R10_REGNUM,		/* sp */
> +  BPF_PC_REGNUM,
> +};
> +
> +#define BPF_NUM_REGS	(BPF_PC_REGNUM + 1)
> +
> +/* Target-dependent structure in gdbarch.  */
> +struct gdbarch_tdep
> +{
> +};
> +

Unless you plan on having other files use these definitions, I'd just put everything
in bpf-tdep.c and not have a bpf-tdep.h.

> +#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

This last line makes this commit not build (with --target=bpf-unknown-none) since the sim
implementation doesn't exist yet.  Please remove it and add it back in the sim commit.

Simon