[PATCH 2/4] Fall back to a default value of 0 for the MISA register.
Andrew Burgess
andrew.burgess@embecosm.com
Fri Sep 21 09:27:00 GMT 2018
* 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
>
>
More information about the Gdb-patches
mailing list