From patchwork Tue Nov 6 11:18:45 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 30035 Received: (qmail 5631 invoked by alias); 6 Nov 2018 11:18:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 5285 invoked by uid 89); 6 Nov 2018 11:18:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=replied, Reading, HX-Received:sk:t9-v6mr, unwinder X-HELO: mail-wr1-f47.google.com Received: from mail-wr1-f47.google.com (HELO mail-wr1-f47.google.com) (209.85.221.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 06 Nov 2018 11:18:50 +0000 Received: by mail-wr1-f47.google.com with SMTP id u9-v6so2678664wrr.0 for ; Tue, 06 Nov 2018 03:18:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=SqrmwqwiaCRT6WUK02wmI9R8NsWZf82vVwbTCEcc2Dw=; b=U6srUug+uC9nXoz46EL7y86+QKslNIsj/PtY3CuhJImTkQgiYuZTHBIEcYxV83ZnTh kUuLx4gfIEMa1d+4THtjp4Z7UQdG4zbFV28oljLV9rFCF8Pice8ajGuaSmjNoFL61XQf HGp3h0fErvhOnApDNdotjEwkIYZUpTbdmAgG74DTrBx5+YYjwsLgef5AYs7Ruft16s7s 3bP5VedU+46Zae96YKqe1I7BL/IlRPGf0X+A0G918WwlleMF+9x9nxTqJdyznd0dPHkk J6onpxnAa5bHJWOiW44E1gyID1zMjLKoeZjhybf7U2wRCnVL6y07ttv/PpdEA13wECqE BKQQ== Return-Path: Received: from localhost (host81-148-252-35.range81-148.btcentralplus.com. [81.148.252.35]) by smtp.gmail.com with ESMTPSA id 137-v6sm2499677wmo.43.2018.11.06.03.18.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 06 Nov 2018 03:18:47 -0800 (PST) Date: Tue, 6 Nov 2018 11:18:45 +0000 From: Andrew Burgess To: Jim Wilson Cc: gdb-patches@sourceware.org, Palmer Dabbelt , John Baldwin Subject: Re: [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails Message-ID: <20181106111845.GI16539@embecosm.com> References: <1ab6341c3c73c6e0b501e7b25d6d64744d7cdbc0.1541459121.git.andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Fortune: The longer the title, the less important the job. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes Thanks for the feedback. * Jim Wilson [2018-11-05 15:37:13 -0800]: > On Mon, Nov 5, 2018 at 3:10 PM Andrew Burgess > 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 = 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. */