* [PATCH 0/2] RISC-V Prologue Scan And Test Improvement @ 2018-11-05 23:10 Andrew Burgess 2018-11-05 23:10 ` [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails Andrew Burgess 2018-11-05 23:10 ` [PATCH 2/2] gdb/riscv: Update test to support targets without FP hardware Andrew Burgess 0 siblings, 2 replies; 6+ messages in thread From: Andrew Burgess @ 2018-11-05 23:10 UTC (permalink / raw) To: gdb-patches; +Cc: jimw, palmer, jhb, 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(-) -- 2.14.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails 2018-11-05 23:10 [PATCH 0/2] RISC-V Prologue Scan And Test Improvement Andrew Burgess @ 2018-11-05 23:10 ` Andrew Burgess 2018-11-05 23:37 ` Jim Wilson 2018-11-05 23:10 ` [PATCH 2/2] gdb/riscv: Update test to support targets without FP hardware Andrew Burgess 1 sibling, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2018-11-05 23:10 UTC (permalink / raw) To: gdb-patches; +Cc: jimw, palmer, jhb, 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 (); -- 2.14.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails 2018-11-05 23:10 ` [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails Andrew Burgess @ 2018-11-05 23:37 ` Jim Wilson 2018-11-06 11:18 ` Andrew Burgess 0 siblings, 1 reply; 6+ messages in thread From: Jim Wilson @ 2018-11-05 23:37 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Palmer Dabbelt, John Baldwin 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails 2018-11-05 23:37 ` Jim Wilson @ 2018-11-06 11:18 ` Andrew Burgess 2018-11-06 19:40 ` Jim Wilson 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2018-11-06 11:18 UTC (permalink / raw) To: Jim Wilson; +Cc: gdb-patches, Palmer Dabbelt, John Baldwin Thanks for the feedback. * Jim Wilson <jimw@sifive.com> [2018-11-05 15:37:13 -0800]: > 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. 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... > 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 :) > 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. > 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. > 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. */ -- 2.14.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails 2018-11-06 11:18 ` Andrew Burgess @ 2018-11-06 19:40 ` Jim Wilson 0 siblings, 0 replies; 6+ messages in thread From: Jim Wilson @ 2018-11-06 19:40 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Palmer Dabbelt, John Baldwin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] gdb/riscv: Update test to support targets without FP hardware 2018-11-05 23:10 [PATCH 0/2] RISC-V Prologue Scan And Test Improvement Andrew Burgess 2018-11-05 23:10 ` [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails Andrew Burgess @ 2018-11-05 23:10 ` Andrew Burgess 1 sibling, 0 replies; 6+ messages in thread From: Andrew Burgess @ 2018-11-05 23:10 UTC (permalink / raw) To: gdb-patches; +Cc: jimw, palmer, jhb, 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 +} -- 2.14.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-06 19:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-05 23:10 [PATCH 0/2] RISC-V Prologue Scan And Test Improvement Andrew Burgess 2018-11-05 23:10 ` [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails Andrew Burgess 2018-11-05 23:37 ` Jim Wilson 2018-11-06 11:18 ` Andrew Burgess 2018-11-06 19:40 ` Jim Wilson 2018-11-05 23:10 ` [PATCH 2/2] gdb/riscv: Update test to support targets without FP hardware Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).