This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails
On Tue, Nov 6, 2018 at 3:18 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> 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.
> > 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.
> 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.
> 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