From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34070 invoked by alias); 19 Dec 2018 18:32:57 -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 34040 invoked by uid 89); 19 Dec 2018 18:32:57 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=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=Supply, H*r:sk:92.2018, remained, After X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Dec 2018 18:32:53 +0000 Received: by mail-wm1-f66.google.com with SMTP id d15so7067682wmb.3 for ; Wed, 19 Dec 2018 10:32:53 -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=hZsRxSF9Uyw+UjwSzsJCN0i+dWgUfeuFIEKuLpb3Bv0=; b=fGc7bAKBahSiOa21cNblVY2BM0UAb90jKffvxHqjTfsf1uP9rSJe9gRlSByKRZM3Gu BfGiZ+Mf+OhiqAKgLCrxFUBEzFwIDVlqg9L4q8qcVf89Hk1sYCvXh3koLLGHB+WezycQ GmOAWmZZxS9gexrEeVaEMfFNuK2w5kPe5IeUqQWvWCq+/37gpPSyutkP90eWaG0+COP7 VTlmD989iR0hGHHY91zcTqkUNDimVanikQfoYMziBDoCMHuh/GKF2dJIxIYv+q9cu4JY K1nXDTDhNj6bWacx1SRp+RRujXgTUaBm9AVNpzFP35/IAe/C7UTk/r1e7j52Ij2Kjn+3 Lo6A== Return-Path: Received: from localhost (host86-156-236-210.range86-156.btcentralplus.com. [86.156.236.210]) by smtp.gmail.com with ESMTPSA id f18sm4380717wrs.92.2018.12.19.10.32.49 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 19 Dec 2018 10:32:49 -0800 (PST) Date: Wed, 19 Dec 2018 18:32:00 -0000 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv5] gdb: Add default frame methods to gdbarch Message-ID: <20181219183248.GA3456@embecosm.com> References: <20181108214357.5364-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: I'm receiving a coded message from EUBIE BLAKE!! X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00225.txt.bz2 * Simon Marchi [2018-12-14 17:22:13 -0500]: > On 2018-11-08 16:43, Andrew Burgess wrote: > > This is another iteration of a patch that I last posted here: > > > > https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html > > > > previous versions can be found here: > > > > https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html > > https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html > > https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html > > > > I believe v4 actually got approved, but, when I went to merge it I > > realised I'd made a mistake! > > > > This new version is mostly the same as v4, but the default_unwind_pc > > method is simpler, but will, I think, still cover almost all targets. > > > > As far as testing, I don't believe that any target will be using these > > default methods after this initial commit, however, I did a build with > > all targets enabled, and it built fine. > > > > Out of tree I've tested this on the RISC-V target, with no regressions. > > > > Is this OK to apply? > > > > Thanks, > > Andrew > > > > > > --- > > > > Supply default gdbarch methods for gdbarch_dummy_id, > > gdbarch_unwind_pc, and gdbarch_unwind_sp. This patch doesn't actually > > convert any targets to use these methods, and so, there will be no > > user visible changes after this commit. > > > > The implementations for default_dummy_id and default_unwind_sp are > > fairly straight forward, these just take on the pattern used by most > > targets. Once these default methods are in place then most targets > > will be able to switch over. > > > > The implementation for default_unwind_pc is also fairly straight > > forward, but maybe needs some explanation. > > > > This patch has gone through a number of iterations: > > > > https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html > > https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html > > https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html > > https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html > > > > and the implementation of default_unwind_pc has changed over this > > time. Originally, I took an implementation like this: > > > > CORE_ADDR > > default_unwind_pc (struct gdbarch *gdbarch, struct frame_info > > *next_frame) > > { > > int pc_regnum = gdbarch_pc_regnum (gdbarch); > > return frame_unwind_register_unsigned (next_frame, pc_regnum); > > } > > > > This is basically a clone of default_unwind_sp, but using $pc. It was > > pointed out that we could potentially do better, and in version 2 the > > implementation became: > > > > CORE_ADDR > > default_unwind_pc (struct gdbarch *gdbarch, struct frame_info > > *next_frame) > > { > > struct type *type; > > int pc_regnum; > > CORE_ADDR addr; > > struct value *value; > > > > pc_regnum = gdbarch_pc_regnum (gdbarch); > > value = frame_unwind_register_value (next_frame, pc_regnum); > > type = builtin_type (gdbarch)->builtin_func_ptr; > > addr = extract_typed_address (value_contents_all (value), type); > > addr = gdbarch_addr_bits_remove (gdbarch, addr); > > release_value (value); > > value_free (value); > > return addr; > > } > > > > The idea was to try split out some of the steps of unwinding the $pc, > > steps that are on some (or many) targets no-ops, and so allow targets > > that do override these methods, to make use of default_unwind_pc. > > > > This implementation remained in place for version 2, 3, and 4. > > > > However, I realised that I'd made a mistake, most targets simply use > > frame_unwind_register_unsigned to unwind the $pc, and this throws an > > error if the register value is optimized out or unavailable. My new > > proposed implementation doesn't do this, I was going to end up > > breaking many targets. > > > > I considered duplicating the code from frame_unwind_register_unsigned > > that throws the errors into my new default_unwind_pc, however, this > > felt really overly complex. So, what I instead went with was to > > simply revert back to using frame_unwind_register_unsigned. Almost > > all existing targets already use this. Some of the ones that don't can > > be converted to, which means almost all targets could end up using the > > default. > > > > One addition I have made over the version 1 implementation is to add a > > call to gdbarch_addr_bits_remove. For most targets this is a no-op, > > but for a handful, having this call in place will mean that they can > > use the default method. After all this, the new default_unwind_pc now > > looks like this: > > > > CORE_ADDR > > default_unwind_pc (struct gdbarch *gdbarch, struct frame_info > > *next_frame) > > { > > int pc_regnum = gdbarch_pc_regnum (gdbarch); > > CORE_ADDR pc = frame_unwind_register_unsigned (next_frame, > > pc_regnum); > > pc = gdbarch_addr_bits_remove (gdbarch, pc); > > return pc; > > } > > That is fine with me. > > The patch LGTM, except the comment in gdbarch.sh, could we clarify it a bit? > > > diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh > > index bbfa8d22058..96afb01c098 100755 > > --- a/gdb/gdbarch.sh > > +++ b/gdb/gdbarch.sh > > @@ -480,7 +480,12 @@ m;const char *;register_name;int regnr;regnr;;0 > > # use "register_type". > > M;struct type *;register_type;int reg_nr;reg_nr > > > > -M;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame > > +# Generate a dummy frame_id for THIS_FRAME assuming that the frame is > > +# a dummy frame. A dummy frame is created before an inferior call, > > +# the frame_id returned here must match the base address returned by > > double space > > > +# gdbarch_push_dummy_call and the frame's pc must match the dummy > > +# frames breakpoint address. > > I have a bit of trouble following what must match what. Do you mean that: > > - the returned frame_id's stack_addr must match the base address returned by > gdbarch_push_dummy_call > - the returned frame_id's code_addr must match the frame's breakpoint > address > > I'm not sure what "frame's breakpoint address" mean, I guess it's the same > as the frame's pc? i.e. the pc we will jump back to when we go back to > executing this frame, which in the case of the dummy frame, is where we have > put a breakpoint? Thanks for the feedback. The original comment was lifted from some target's dummy_id method. Your interpretation is correct. I've tried to reword it to make the comment clearer, but I'm not totally sure this is better. What do you think? diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh index a876a21555e..845d9e27e4c 100755 --- a/gdb/gdbarch.sh +++ b/gdb/gdbarch.sh @@ -480,7 +480,15 @@ m;const char *;register_name;int regnr;regnr;;0 # use "register_type". M;struct type *;register_type;int reg_nr;reg_nr -M;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame +# Generate a dummy frame_id for THIS_FRAME assuming that the frame is +# a dummy frame. A dummy frame is created before an inferior call, +# the frame_id returned here must match the frame_id that was built +# for the inferior call. Usually this means the returned frame_id's +# stack address should match the address returned by +# gdbarch_push_dummy_call, and the retuned frame_id's code address +# should match the address at which the breakpoint was set in the dummy +# frame. +m;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame;;default_dummy_id;;0 # Implement DUMMY_ID and PUSH_DUMMY_CALL, then delete # deprecated_fp_regnum. v;int;deprecated_fp_regnum;;;-1;-1;;0 Thanks, Andrew