Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin <jhb@FreeBSD.org>]

On 9/21/18 2:27 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2018-09-20 16:01:04 -0700]:
> 
>> On 9/20/18 2:51 PM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]:
>> One other question is if I drop the change to default MISA to 0, we should
>> perhaps fix the comment above riscv_read_misa?  The comment implies that
>> it falls back to zero if it can't read the register and it does that for
>> the !target_has_registers case already.  It's not clear from the comment
>> that targets are required to provide MISA.
> 
> I'm kind-of mixed about the default 0 patch.  The spec says:
> 
>     The misa CSR is an XLEN-bit WARL read-write register reporting the
>     ISA supported by the hart. This register must be readable in any
>     implementation, but a value of zero can be returned to indicate
>     the misa register has not been implemented, requiring that CPU
>     capabilities be determined through a separate non-standard
>     mechanism.
> 
> So, it doesn't seem to crazy to say that in GDB, if we make not one,
> but two attempts to find a MISA value, fail on both, then we could
> assume a default of 0.  After all, the default of 0 just means, go
> figure out an answer for yourself, so we shouldn't be any worse off.
> 
> Still, it would probably be a good thing if targets did just provide a
> 0 value for misa on their own as that would be more inline with the
> spec (I think).
> 
> Having just looked at riscv_read_misa_reg again, it turns out that
> it's completely broken anyway.  Take a look at how the READ_P
> parameter is initialised in the caller and then (not) set in
> function.
> 
> Further, the one and only caller, checks READ_P /or/ for a return
> value of 0, so just defaulting to 0 would be fine.  At a minimum we
> should apply this patch:
> 
> ## START ##
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..52e101e6ab8 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0;
>     default (false).  */
>  
>  static uint32_t
> -riscv_read_misa_reg (bool *read_p)
> +riscv_read_misa_reg ()
>  {
>    uint32_t value = 0;
>  
> @@ -334,13 +334,10 @@ riscv_read_misa_reg (bool *read_p)
>  static bool
>  riscv_has_feature (struct gdbarch *gdbarch, char feature)
>  {
> -  bool have_read_misa = false;
> -  uint32_t misa;
> -
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>  
> -  misa = riscv_read_misa_reg (&have_read_misa);
> -  if (!have_read_misa || misa == 0)
> +  uint32_t misa = riscv_read_misa_reg ();
> +  if (misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>  
>    return (misa & (1 << (feature - 'A'))) != 0;
> 
> ## END ##
> 
> And, better still would be something more like your original patch:
> 
> ## START ##
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..58e4c221a7c 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0;
>     default (false).  */
>  
>  static uint32_t
> -riscv_read_misa_reg (bool *read_p)
> +riscv_read_misa_reg ()
>  {
>    uint32_t value = 0;
>  
> @@ -310,18 +310,23 @@ riscv_read_misa_reg (bool *read_p)
>      {
>        struct frame_info *frame = get_current_frame ();
>  
> -      TRY
> -	{
> -	  value = get_frame_register_unsigned (frame,
> -					       RISCV_CSR_MISA_REGNUM);
> -	}
> -      CATCH (ex, RETURN_MASK_ERROR)
> +      /* Old cores might have MISA located at a different offset.  */
> +      int misa_regs[] =
> +        { RISCV_CSR_MISA_REGNUM, RISCV_CSR_LEGACY_MISA_REGNUM };
> +
> +      for (int i = 0; i < ARRAY_SIZE (misa_regs); ++i)
>  	{
> -	  /* Old cores might have MISA located at a different offset.  */
> -	  value = get_frame_register_unsigned (frame,
> -					       RISCV_CSR_LEGACY_MISA_REGNUM);
> +	  TRY
> +	    {
> +	      value = get_frame_register_unsigned (frame, misa_regs[i]);
> +	      break;
> +	    }
> +	  CATCH (ex, RETURN_MASK_ERROR)
> +	    {
> +	      /* Ignore the error.  */
> +	    }
> +	  END_CATCH
>  	}
> -      END_CATCH
>      }
>  
>    return value;
> @@ -334,13 +339,10 @@ riscv_read_misa_reg (bool *read_p)
>  static bool
>  riscv_has_feature (struct gdbarch *gdbarch, char feature)
>  {
> -  bool have_read_misa = false;
> -  uint32_t misa;
> -
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>  
> -  misa = riscv_read_misa_reg (&have_read_misa);
> -  if (!have_read_misa || misa == 0)
> +  uint32_t misa = riscv_read_misa_reg ();
> +  if (misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>  
>    return (misa & (1 << (feature - 'A'))) != 0;
> 
> ## END ##
> 
> Jim: Given that we agree that targets should definitely provide a
> value for misa, at a minimum just returning the constant 0.  But,
> given that GDB already defaults to 0 in some cases anyway.  And the
> spec is quite clear that 0 is the right default value in the absence
> of anything better, would you be OK with a patch that does return a
> default of 0?

FWIW, I think the second patch looks fine.  I can confirm that the breakpoint
patch is sufficient for my limited testing on FreeBSD that I no longer need
the MISA patch so I've dropped it from my local series (I'll post a V2 in a
bit).

-- 
John Baldwin