Re: [PATCH 1/2] gdb: support for eBPF

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

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!