From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28831 invoked by alias); 17 Oct 2007 00:30:23 -0000 Received: (qmail 28817 invoked by uid 22791); 17 Oct 2007 00:30:21 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 17 Oct 2007 00:30:16 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 330F198336; Wed, 17 Oct 2007 00:30:15 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id A8D3C98100; Wed, 17 Oct 2007 00:30:14 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.68) (envelope-from ) id 1Ihwnd-0000Gj-Se; Tue, 16 Oct 2007 20:30:13 -0400 Date: Wed, 17 Oct 2007 00:36:00 -0000 From: Daniel Jacobowitz To: Mark Kettenis Cc: brobecker@adacore.com, gdb-patches@sourceware.org, jimb@codesourcery.com Subject: Re: [rfc] Unwind the ARM CPSR Message-ID: <20071017003013.GA31318@caradoc.them.org> Mail-Followup-To: Mark Kettenis , brobecker@adacore.com, gdb-patches@sourceware.org, jimb@codesourcery.com References: <20071011145137.GA18336@caradoc.them.org> <200710162220.l9GMKmu1002397@brahms.sibelius.xs4all.nl> <20071011145137.GA18336@caradoc.them.org> <20071012035336.GA4038@adacore.com> <200710162201.l9GM13MR006755@brahms.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200710162220.l9GMKmu1002397@brahms.sibelius.xs4all.nl> <200710162201.l9GM13MR006755@brahms.sibelius.xs4all.nl> User-Agent: Mutt/1.5.15 (2007-04-09) X-IsSubscribed: yes 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 X-SW-Source: 2007-10/txt/msg00433.txt.bz2 Hi Mark, and thanks for the comments. I've been working on this again (all day...), so it's fresh in my mind. On Wed, Oct 17, 2007 at 12:01:03AM +0200, Mark Kettenis wrote: > > I can see why you'd like some feedback. This (amazing piece of) code > > has always been a bit difficult for me to grasp in its entirety... > > I hope others will take some time to think about it too, because > > I don't feel sufficiently proficient to provide a confident answer. > > > > FWIW, I don't see how we could end up breaking something with your > > approach. As it turns out, there is a comment that says about > > the prev_register method: > > > > Why not pass in THIS_FRAME? By passing in NEXT frame and THIS > > cache, the supplied parameters are consistent with the sibling > > function THIS_ID. > > > > So it sounds like the author wasn't seeing any issue with that > > either. > > I wouldn't be so sure. Andrew Cagney put quite a bit of thought in > the interface, and while it sometimes appears to be strange on first > sight, it has always proven to be the right choice. I agree that he put a lot of thought into it, but some of his decisions were forced by the path to get GDB from where it was then (ew) to where it is now (much nicer). I looked into the history of this today. The original version of frame-unwind.h (excluding development branches) was on 2003-01-18, and passed the current frame. It was changed to pass the next frame on 2003-03-17 as the result of Andrew's offbyone branch. Here's the patch: http://sourceware.org/ml/gdb-patches/2003-03/msg00339.html The problem was a mismatch between which cache the this_id function was passed and which cache the prev_register function for the same frame was passed. I rediscovered this problem today and I can see why it was such a pain to fix at the time - it rather ruined my evening. But the most interesting thing I found in the archives was this: http://sourceware.org/ml/gdb-patches/2003-03/msg00365.html That, a couple of days later, was the first step in not calculating the frame ID until we needed it. Before that happened, Andrew's fix for the off-by-one bug was the only workable solution: the previous frame wasn't usable until after its ID had been calculated. After the frame ID was calculated lazily, though, getting the previous frame became a cheap operation and we could postpone ID checks until we needed them. So the problem that Andrew originally solved by passing the NEXT frame to the unwinder is gone now. Just for kicks I took a look at Frysk, whose unwinder he's working on as we speak. A Frame object has a getFrameIdentifier method (equivalent to ->this_id taking this_frame), and a getRegister method. I can't tell if that's prev_register taking this_frame or my_register taking this_frame. > I think it is a very bad idea. It will take ages before all targets > are converted, and having two functions to do the same thing will be > even more confusing. Yes, that's true; it will be confusing. I've taken the opportunity to work on a related project at the same time though and so far I like the shape of the result: typedef struct value * (frame_prev_register_value_ftype) (struct frame_info *this_frame, void **this_prologue_cache, int prev_regnum); The mental exercise of having only the NEXT frame prevents some stupid things. But it also prevents some smart things, and it makes unwinders (in my personal opinion only, of course) much harder to write. This is probably the subtlest part of GDB; I like anything that makes it more obvious how it works. I'll have a concrete example finished in another day or so, and then I'll post a proper RFC for the frame changes. On Wed, Oct 17, 2007 at 12:20:49AM +0200, Mark Kettenis wrote: > > To handle that, I added a new DWARF2_FRAME_REG_FN which supplies a > > default unwinder. For the PC, I originally added > > DWARF2_FRAME_REG_RA_MASK instead to mask out the low bit, but > > that wasn't enough for the CPSR. And I discovered that > > there is no easy way, given the arguments to an unwinder's > > prev_register method, to request the unwound value of another > > register from the same frame! That's why the patch below had > > to make dwarf2_frame_prev_register global, and pass its opaque > > THIS_CACHE to the function supplied by DWARF2_FRAME_REG_FN. > > Two remarks: > > 1. Isn't this function always going to return a plain value? That > means we can probably simplify the register. Not pass all those extra arguments, you mean? Yes, maybe we can. It did always return a plain value in my case and we could change it later if more generality was needed. > 2. Perhaps the function should get passed the unwinder struct, such > that it can use the function pointer. Yeah, that's a nice alternative to making dwarf2_frame_prev_register global. It means we need to pass the frame, the unwinder, and the cache, though - that still suggests to me that there is a simpler and righter answer. > > During a prev_register method there are three frames of interest. The > > NEXT frame is passed; the CURRENT frame's cache is passed and some > > knowledge of the CURRENT frame is implicit in the called function; and > > the PREVIOUS frame's register value is calculated. Given a frame > > there are functions to get its register values or the values of the > > previous frame; since we start with NEXT that means we can use generic > > frame interfaces to get only NEXT or CURRENT registers. But to > > calculate PREVIOUS's CPSR we need PREVIOUS's LR so we have to call > > dwarf2_frame_prev_register directly. > > I think this means that making the unwinder reconstruct CPSR may not > be the right approach. Isn't it better to determine thumbness based > on the unwound LR? That should work for all frames, except the > topmost. But for the topmost frame we should be able to trust CPSR. I implemented that first, actually. In that case I need to check the types (get_frame_type) of various frames, since the frame before a signal frame or dummy frame are also "topmost". And I ended up with some assumptions that violated the abstraction boundaries of the unwinder, e.g., that signal and dummy frames would always have a saved CPSR. It seemed very inelegant to me; the right "unwound" value of the CPSR depends on how we unwound, so why not have the unwinder supply it? > I also see that you treat unwinding the PC specially in the unwinders, > to strip off bits. But isn't that why we have the unwind_pc gdbarch > method? The failing testcase that started me down this rabbit hole was return.exp, which uses frame_pop. The gdbarch unwind_pc method is always used when core GDB wants a PC or resume address, but it's not used when we want to restore the value of a register (which happens to be register number PC_REGNUM). And something needs to clean up the saved CPSR value when popping, for the same reason - you need interworking (mixed ARM and Thumb) for that to cause a failure though so the current testsuite never triggers it. -- Daniel Jacobowitz CodeSourcery