From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102654 invoked by alias); 19 Dec 2018 18:37:52 -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 100509 invoked by uid 89); 19 Dec 2018 18:37:48 -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,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Dec 2018 18:37:45 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id wBJIbdwV025432 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 19 Dec 2018 13:37:43 -0500 Received: by simark.ca (Postfix, from userid 112) id 20EB81E7B1; Wed, 19 Dec 2018 13:37:39 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id EA4C21E059; Wed, 19 Dec 2018 13:37:35 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 19 Dec 2018 18:37:00 -0000 From: Simon Marchi To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv5] gdb: Add default frame methods to gdbarch In-Reply-To: <20181219183248.GA3456@embecosm.com> References: <20181108214357.5364-1-andrew.burgess@embecosm.com> <20181219183248.GA3456@embecosm.com> Message-ID: <198ef7c34561a818fdd4792477dbf135@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00226.txt.bz2 On 2018-12-19 13:32, Andrew Burgess wrote: > * 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 "retuned" > +# should match the address at which the breakpoint was set in the > dummy > +# frame. > +m;struct frame_id;dummy_id;struct frame_info Thanks, this is fine with me, please push. Simon