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

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

* 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]:
> >> @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
> >>  {
> >>    if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
> >>      {
> >> -      if (riscv_has_feature (gdbarch, 'C'))
> >> +      enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> > 
> > byte_order is unused.
> 
> Will fix.
> 
> >> +      gdb_byte buf[1];
> >> +      int status;
> >> +
> >> +      /* Read the opcode byte to determine the instruction length.  */
> >> +      status = target_read_memory (*pcptr, buf, 1);
> > 
> > This should use target_read_code.  I know that we already have some
> > (incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix
> > those later.
> 
> Ok.
> 
> > However, this causes a testsuite regression for gdb.gdb/unittest.exp.
> > You can easily reproduce the issue with:
> > 
> >     (gdb) maintenance selftest 
> > 
> > We probably need to add a riscv specific case into
> > disasm-selftest.c:print_one_insn_test, lots of other targets already
> > do.
> 
> Ok.  I'll reproduce that and figure out the fix and include it in a V2
> patch.
> 
> 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?

Thanks,
Andrew

> 
> -- 
> John Baldwin
> 
>