[PATCH 0/2] RISC-V Prologue Scan And Test Improvement

[Andrew Burgess]

Two patches that are unrelated other than being RISC-V related.

--

Andrew Burgess (2):
gdb/riscv: Stop prologue scan if instruction fetch/decode fails
gdb/riscv: Update test to support targets without FP hardware

gdb/ChangeLog | 6 ++++++
gdb/riscv-tdep.c | 27 ++++++++++++++++++++++++---
gdb/testsuite/ChangeLog | 5 +++++
gdb/testsuite/gdb.arch/riscv-reg-aliases.exp | 24 ++++++++++++++++++------
4 files changed, 53 insertions(+), 9 deletions(-)

[Andrew Burgess]

If an error is thrown from the instruction fetch/decode during a
prologue scan then we should stop the prologue scan at that point
rather than propagating the error.

Propagating the error out of the prologue scan was causing unwanted
behaviour when connecting to a remote target. When connecting to a
remote target GDB will read the $pc value from the target and try to
establish a frame-id, this can involve a prologue scan.

If the target has not yet had a program loaded into it, and the $pc
value is pointing an unreadable memory, then the prologue scan would
throw an error, this would then cause GDB to abandon its attempt to
connect to the target. It was in fact impossible to connect to the
target at all.

With this patch in place GDB simply stops the prologue scan at the
point of the error, and GDB can now successfully connect.

I did consider placing the error catch within riscv_insn::decode
however, in the end I felt that catching and ignoring errors should be
done on a case by case basis, the other users of riscv_insn::decode
are currently all related to finding the next pc as part of software
single step. If the user asks for a step and the contents of $pc
can't be read then if feels like terminating that command with an
error is the right thing to do.

gdb/ChangeLog:

* riscv-tdep.c (riscv_insn::decode): Update header comment.
(riscv_scan_prologue): Catch errors thrown from
riscv_insn::decode and stop prologue scan.
---
gdb/ChangeLog | 6 ++++++
gdb/riscv-tdep.c | 27 ++++++++++++++++++++++++---
2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index db372e21632..aae93ea2fac 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1256,7 +1256,9 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
return extract_unsigned_integer (buf, instlen, byte_order);
}

-/* Fetch from target memory an instruction at PC and decode it. */
+/* Fetch from target memory an instruction at PC and decode it. This can
+ throw an error if the memory access fails, callers are responsible for
+ handling this error if that is appropriate. */

void
riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
@@ -1427,10 +1429,29 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
for (next_pc = cur_pc = start_pc; cur_pc < end_pc; cur_pc = next_pc)
{
struct riscv_insn insn;
+ bool decode_valid = false;

/* Decode the current instruction, and decide where the next
- instruction lives based on the size of this instruction. */
- insn.decode (gdbarch, cur_pc);
+ instruction lives based on the size of this instruction. If the
+ decode (which includes fetching from memory) fails then we stop
+ the prologue scan at this point. */
+ TRY
+ {
+ insn.decode (gdbarch, cur_pc);
+ decode_valid = true;
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ /* Ignore errors. */
+ }
+ END_CATCH
+
+ if (!decode_valid)
+ {
+ end_prologue_addr = cur_pc;
+ break;
+ }
+
gdb_assert (insn.length () > 0);
next_pc = cur_pc + insn.length ();

[Jim Wilson]

On Mon, Nov 5, 2018 at 3:10 PM Andrew Burgess

Post by Andrew Burgess
If the target has not yet had a program loaded into it, and the $pc
value is pointing an unreadable memory, then the prologue scan would
throw an error, this would then cause GDB to abandon its attempt to
connect to the target. It was in fact impossible to connect to the
target at all.

In my case, with openocd/spike, the pc value is actually correct and
there is a valid instruction there. The problem rather happens in
riscv_frame_cache which calls get_frame_func, and this returns 0
because there is no program loaded yet. This then causes a scan for
the prologue to start at address zero, which is wrong, and leads to
the null deref error that kills the connection. I have a simpler fix
based on code I found in mips-tdep.c, which just returns from
riscv_frame_cache if start_addr is zero, and also in
riscv_frame_this_id we don't set this_id if the frame_base is zero.
With your fix, riscv_scan_prologue will be run, the frame cache will
be filled with incorrect values, and we will try to compute a frame id
based on bad info. That doesn't look like the right solution to me.
My patch is a slightly cleaned up version of the workarounds I sent to
you last week, which I am testing now.

Jim

PS Did you see my code_elim testcase fix? Simon Marchi suggested that
you should review it.

[Andrew Burgess]

Thanks for the feedback.

Post by Jim Wilson
On Mon, Nov 5, 2018 at 3:10 PM Andrew Burgess

Post by Andrew Burgess
If the target has not yet had a program loaded into it, and the $pc
value is pointing an unreadable memory, then the prologue scan would
throw an error, this would then cause GDB to abandon its attempt to
connect to the target. It was in fact impossible to connect to the
target at all.

In my case, with openocd/spike, the pc value is actually correct and
there is a valid instruction there. The problem rather happens in
riscv_frame_cache which calls get_frame_func, and this returns 0
because there is no program loaded yet.

You are right that my original explanation isn't quite right. But I
think the real problem is not whether a program is loaded or not, it's
that GDB can't figure out the function start. This of course is done
as I understand it by examining the program at GDB's end. I'm not
sure why I even mention this to be honest, it's largely irrelevant...

Post by Jim Wilson
This then causes a scan for
the prologue to start at address zero, which is wrong, and leads to
the null deref error that kills the connection.

Interesting, does it actually kill the connection for you? I too am
testing against openocd/spike, and what I see is that GDB
disconnects, but the target is still running, and I can try to connect
again, and again, and again.....

We probably are seeing the same thing, I just want to make sure we're
not trying to fix different problems :)

Post by Jim Wilson
I have a simpler fix
based on code I found in mips-tdep.c, which just returns from
riscv_frame_cache if start_addr is zero, and also in
riscv_frame_this_id we don't set this_id if the frame_base is zero.

We really shouldn't do that. I've worked on too many embeded targets
where 0 is a valid address, and every time I hit a "zero is special"
case in GDB I die a little inside.

There certainly is plenty of such code in GDB, there shouldn't be, and
when I run into them, I do try to remove them.

Post by Jim Wilson
With your fix, riscv_scan_prologue will be run, the frame cache will
be filled with incorrect values, and we will try to compute a frame id
based on bad info.

Yeah, OK. I don't think I see this as being as big a problem as you
do, the targets in an undefined state, we see undefined things. I can
live with that. I'm pretty sure that even with you special case zero
fix, you still see undefined state, its just that some of the value
are undefined to zero.... That said, I do agree a little that leaving
the frame cache partially initialised probably isn't that great.

Post by Jim Wilson
That doesn't look like the right solution to me.
My patch is a slightly cleaned up version of the workarounds I sent to
you last week, which I am testing now.

Sorry, I missed that patch, otherwise I would have replied.

The revised patch below achieves the result you would like (not
setting the frame id) but does so without special casing address
zero. How do you feel about this?

Thanks,
Andrew

---

gdb/riscv: Handle errors while setting the frame id

When we connect to a remote target one of the first things GDB does is
establish a frame id. If an error is thrown while building this frame
id then GDB will disconnect from the target.

This can mean that, if the user is attempting to connect to a target
that doesn't yet have a program loaded, or the program the user is
going to load onto the target doesn't match what is already loaded, or
the target is just in some undefined state, then the very first
request for a frame id can fail (for example, by trying to load from
an invalid memory address), and GDB will disconnect. It is then
impossible for the user to connect to the target and load a new
program at all.

An example of such a session might look like this:

Reading symbols from ./gdb/testsuite/outputs/gdb.arch/riscv-reg-aliases/riscv-reg-aliases...
(gdb) target remote :37191
Remote debugging using :37191
0x0000000000000100 in ?? ()
Cannot access memory at address 0x0
(gdb) load
You can't do that when your target is `exec'
(gdb) info frame
/path/to/gdb/gdb/thread.c:93: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

The solution is to handle errors in riscv_frame_this_id, and leave the
this_id variable with its default value, which is the predefined
'outermost' frame.

With this fix in place, connecting to the same target now looks like
this:

(gdb) target remote :37191
Remote debugging using :37191
0x0000000000000100 in ?? ()
(gdb) info frame
Stack level 0, frame at 0x0:
pc = 0x100; saved pc = <not saved>
Outermost frame: outermost
Arglist at unknown address.
Locals at unknown address, Previous frame's sp in sp

gdb/ChangeLog:

* riscv-tdep.c (riscv_insn::decode): Update header comment.
(riscv_frame_this_id): Catch errors thrown while building the
frame cache, leave the frame id as the default, which is the outer
frame id.
---
gdb/ChangeLog | 7 +++++++
gdb/riscv-tdep.c | 17 ++++++++++++++---
2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index db372e21632..7a92fc7fae5 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1256,7 +1256,9 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
return extract_unsigned_integer (buf, instlen, byte_order);
}

-/* Fetch from target memory an instruction at PC and decode it. */
+/* Fetch from target memory an instruction at PC and decode it. This can
+ throw an error if the memory access fails, callers are responsible for
+ handling this error if that is appropriate. */

void
riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
@@ -2752,8 +2754,17 @@ riscv_frame_this_id (struct frame_info *this_frame,
{
struct riscv_unwind_cache *cache;

- cache = riscv_frame_cache (this_frame, prologue_cache);
- *this_id = cache->this_id;
+ TRY
+ {
+ cache = riscv_frame_cache (this_frame, prologue_cache);
+ *this_id = cache->this_id;
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ /* Ignore errors, this leaves the frame id as the predefined outer
+ frame id which terminates the backtrace at this point. */
+ }
+ END_CATCH
}

/* Implement the prev_register callback for RiscV frame unwinder. */

[Jim Wilson]

On Tue, Nov 6, 2018 at 3:18 AM Andrew Burgess

Post by Andrew Burgess
Interesting, does it actually kill the connection for you? I too am
testing against openocd/spike, and what I see is that GDB
disconnects, but the target is still running, and I can try to connect
again, and again, and again.....

The target remote command fails. I'm not actually sure about the
underlying connection.

Post by Andrew Burgess

Post by Jim Wilson
I have a simpler fix
based on code I found in mips-tdep.c, which just returns from
riscv_frame_cache if start_addr is zero, and also in
riscv_frame_this_id we don't set this_id if the frame_base is zero.

We really shouldn't do that. I've worked on too many embeded targets
where 0 is a valid address, and every time I hit a "zero is special"
case in GDB I die a little inside.

Yes, I'm not entirely happy with that either, it just seemed
acceptable for now. Your solution in the decoder just looked funny to
me, because it isn't a bug in the decoder if it is given a bogus
address to decode. We should avoid giving it a bogus address in the
first place.

get_frame_func is calling get_pc_function_start which returns 0 if it
can't find the correct value. Perhaps there should be a better way
for this function to indicate an error, and then we can avoid passing
a bogus 0 address into the decoder. Maybe also
get_frame_func_if_available should avoid setting prev_func.p to 1 when
get_pc_function_start fails. This would generate an exception from
get_frame_func which we could catch.

Post by Andrew Burgess
Yeah, OK. I don't think I see this as being as big a problem as you
do, the targets in an undefined state, we see undefined things. I can
live with that. I'm pretty sure that even with you special case zero
fix, you still see undefined state, its just that some of the value
are undefined to zero.... That said, I do agree a little that leaving
the frame cache partially initialised probably isn't that great.

I don't know if this is a problem or not. It just seemed unwise to
have some possibly wrong info in there.

Post by Andrew Burgess
The revised patch below achieves the result you would like (not
setting the frame id) but does so without special casing address
zero. How do you feel about this?
* riscv-tdep.c (riscv_insn::decode): Update header comment.
(riscv_frame_this_id): Catch errors thrown while building the
frame cache, leave the frame id as the default, which is the outer
frame id.

Yes, I like this better, because the fix is closer to where the real
problem is, in the riscv frame cache code.

Jim

[Andrew Burgess]

Update gdb.arch/riscv-reg-aliases.exp test to support targets without
floating point registers.

gdb/testsuite/ChangeLog:

* gdb.arch/riscv-reg-aliases.exp: Handle targets without floating
point hardware.
---
gdb/testsuite/ChangeLog | 5 +++++
gdb/testsuite/gdb.arch/riscv-reg-aliases.exp | 24 ++++++++++++++++++------
2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/riscv-reg-aliases.exp b/gdb/testsuite/gdb.arch/riscv-reg-aliases.exp
index b4a2c982897..746ba7dda5d 100644
--- a/gdb/testsuite/gdb.arch/riscv-reg-aliases.exp
+++ b/gdb/testsuite/gdb.arch/riscv-reg-aliases.exp
@@ -151,17 +151,23 @@ check_zero_register_value "after write to \$x0"
# we should access the register using 'REG_NAME.float'. In the following we
# figure out if the field name is needed or not by looking at how GDB prints
# on register.
+set skip_freg_tests 0
set freg_extension "INVALID"
set message "check format of float registers"
-gdb_test_multiple "p \$ft0" $message {
- -re " = {float = \[^\r\n\]+}\r\n$gdb_prompt $" {
- set freg_extension ".float"
+gdb_test_multiple "info registers \$ft0" $message {
+ -re "Invalid register `ft0'\r\n$gdb_prompt $" {
+ set skip_freg_tests 1
+ set freg_extension "NONE"
pass $message
}
- -re " = \[^{}\r\n\]+\r\n$gdb_prompt $" {
+ -re "ft0 \+\[0-9\]\+.*\r\n$gdb_prompt $" {
set freg_extension ""
pass $message
}
+ -re "ft0 \+\{float = .*\r\n$gdb_prompt $" {
+ set freg_extension ".float"
+ pass $message
+ }
}
gdb_assert ![string eq "${freg_extension}" "INVALID"] \
"check that floating point format has been understood"
@@ -169,7 +175,10 @@ gdb_assert ![string eq "${freg_extension}" "INVALID"] \
# Now check that we can write zero, and read zero back to all of the integer
# and floating point registers.
check_setting_registers_to_zero ${xreg_names} ""
-check_setting_registers_to_zero ${freg_names} ${freg_extension}
+
+if { ! $skip_freg_tests } {
+ check_setting_registers_to_zero ${freg_names} ${freg_extension}
+}

# Set each register in turn to a new value, and confirm that the new value can
# be read back from the primary name, and from all of the alias names. The
@@ -177,4 +186,7 @@ check_setting_registers_to_zero ${freg_names} ${freg_extension}
# significantly different so that the float tests don't reuse value from the
# integer tests.
check_setting_registers_to_value ${xreg_names} "" 100
-check_setting_registers_to_value ${freg_names} ${freg_extension} 500
+
+if { ! $skip_freg_tests } {
+ check_setting_registers_to_value ${freg_names} ${freg_extension} 500
+}