Re: [PATCH] RISC-V: Don't decrement pc after break.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Jim Wilson <jimw@sifive.com> [2018-07-16 17:12:41 -0700]:

> This is the gdb patch for the RISC-V Linux kernel patch I just submitted.
>     https://patchwork.kernel.org/patch/10524925/
> This removes the code that decrements the pc after a break, now that we have
> a patch to stop linux from pointlessly adding to the pc after a break.  It
> would be nice if this goes in now, to avoid unnecessary divergence between
> gdb and the linux kernel.  Palmer, the RISC-V linux kernel maintainer, has
> already agreed to accept that patch.
> 
> This will also be needed by the FreeBSD gdb port which may be started soon.
> 
> This also fixes a bug in the code.  The fact that we have two different
> mechanisms to decide breakpoint size, used_compressed_breakpoints and
> has_compressed_isa, means that it is possible for gdb to emit a 4 byte
> breakpoint and then subtract 2 from the pc, and vice versa.  Removing the
> unnecessary pc decrement fixes that problem.
> 
> OK?
> 
> Jim
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_has_feature): Delete comment that refers to
> 	set_gdbarch_decr_pc_after_break.  Call riscv_read_misa_reg always.
> 	(riscv_gdbarch_init): Delete local has_compressed_isa.  Delete now
> 	unecessary braces after EF_RISCV_RVC test.  Delete call to
> 	set_gdbarch_decr_pc_after_break.

This looks good to me.

Thanks,
Andrew


> ---
>  gdb/riscv-tdep.c | 25 ++-----------------------
>  1 file changed, 2 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 72dab0f897..f5d1af822c 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -335,23 +335,7 @@ riscv_has_feature (struct gdbarch *gdbarch, char feature)
>  
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>  
> -  /* It would be nice to always check with the real target where possible,
> -     however, for compressed instructions this is a bad idea.
> -
> -     The call to `set_gdbarch_decr_pc_after_break' is made just once per
> -     GDBARCH and we decide at that point if we should decrement by 2 or 4
> -     bytes based on whether the BFD has compressed instruction support or
> -     not.
> -
> -     If the BFD was not compiled with compressed instruction support, but we
> -     are running on a target with compressed instructions then we might
> -     place a 4-byte breakpoint, then decrement the $pc by 2 bytes leading to
> -     confusion.
> -
> -     It's safer if we just make decisions about compressed instruction
> -     support based on the BFD.  */
> -  if (feature != 'C')
> -    misa = riscv_read_misa_reg (&have_read_misa);
> +  misa = riscv_read_misa_reg (&have_read_misa);
>    if (!have_read_misa || misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>  
> @@ -2440,7 +2424,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    struct gdbarch *gdbarch;
>    struct gdbarch_tdep *tdep;
>    struct gdbarch_tdep tmp_tdep;
> -  bool has_compressed_isa = false;
>    int i;
>  
>    /* Ideally, we'd like to get as much information from the target for
> @@ -2472,10 +2455,7 @@ riscv_gdbarch_init (struct gdbarch_info info,
>  			_("unknown ELF header class %d"), eclass);
>  
>        if (e_flags & EF_RISCV_RVC)
> -	{
> -	  has_compressed_isa = true;
> -	  tmp_tdep.core_features |= (1 << ('C' - 'A'));
> -	}
> +	tmp_tdep.core_features |= (1 << ('C' - 'A'));
>  
>        if (e_flags & EF_RISCV_FLOAT_ABI_DOUBLE)
>  	{
> @@ -2545,7 +2525,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    set_gdbarch_register_reggroup_p (gdbarch, riscv_register_reggroup_p);
>  
>    /* Functions to analyze frames.  */
> -  set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>    set_gdbarch_skip_prologue (gdbarch, riscv_skip_prologue);
>    set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
>    set_gdbarch_frame_align (gdbarch, riscv_frame_align);
> -- 
> 2.17.1
>