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

[Jim Wilson <jimw at sifive dot com>]

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