From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47311 invoked by alias); 6 Nov 2018 19:40:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 47277 invoked by uid 89); 6 Nov 2018 19:40:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=H*RU:209.85.222.68, Hx-spam-relays-external:209.85.222.68 X-HELO: mail-ua1-f68.google.com Received: from mail-ua1-f68.google.com (HELO mail-ua1-f68.google.com) (209.85.222.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 06 Nov 2018 19:40:27 +0000 Received: by mail-ua1-f68.google.com with SMTP id s26so4992190uao.0 for ; Tue, 06 Nov 2018 11:40:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RAlewsxXPaSIICK+BLJksMvJBsw8qSPU+qinbZvTGtQ=; b=R1jAer5kVRBVn+n2gwN/TaZPxEjkyBQNloBHCnQ53ocytfOsSJd8gLX9p7gVVqEcYG Pj6ixsDOW/9Rugpat5E7F3AAv3+PowkZMncX7hDQGUx8jIIX9P6LYndwG9TzN8WRFRBw ErT1dWdKbCXkoOtKKjcaNVOK5IDxgIzfWEtipZD2II0yiR1xlL+pl8QxsQ67DYZYmdBd Bvy98ycv4c14QnRfK41rL9lF7lHMUOWwFLzDxDmmZnkNT3x/6Mp4KLMmGbCTF3YX4Hma 1ZWf2xYZJNPZDOMt/3ydQFaeyBBnMwf2KXkvTqxCi4lTeRxcnf3jDYwpiO7CXnyHGj2g EI+w== MIME-Version: 1.0 References: <1ab6341c3c73c6e0b501e7b25d6d64744d7cdbc0.1541459121.git.andrew.burgess@embecosm.com> <20181106111845.GI16539@embecosm.com> In-Reply-To: <20181106111845.GI16539@embecosm.com> From: Jim Wilson Date: Tue, 06 Nov 2018 19:40:00 -0000 Message-ID: Subject: Re: [PATCH 1/2] gdb/riscv: Stop prologue scan if instruction fetch/decode fails To: Andrew Burgess Cc: gdb-patches@sourceware.org, Palmer Dabbelt , John Baldwin Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-11/txt/msg00075.txt.bz2 On Tue, Nov 6, 2018 at 3:18 AM Andrew Burgess 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