diff mbox

[1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails

Message ID 1ab6341c3c73c6e0b501e7b25d6d64744d7cdbc0.1541459121.git.andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Nov. 5, 2018, 11:09 p.m. UTC
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(-)

Comments

Jim Wilson Nov. 5, 2018, 11:37 p.m. UTC | #1
On Mon, Nov 5, 2018 at 3:10 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> 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.
diff mbox

Patch

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 ();