From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753341AbdF2OEQ (ORCPT ); Thu, 29 Jun 2017 10:04:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41770 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753237AbdF2OEH (ORCPT ); Thu, 29 Jun 2017 10:04:07 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 43F0EC0C0568 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jpoimboe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 43F0EC0C0568 Date: Thu, 29 Jun 2017 09:04:04 -0500 From: Josh Poimboeuf To: Ingo Molnar Cc: x86@kernel.org, linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, Linus Torvalds , Andy Lutomirski , Jiri Slaby , "H. Peter Anvin" , Peter Zijlstra Subject: Re: [PATCH v2 4/8] objtool: add undwarf debuginfo generation Message-ID: <20170629140404.qgcvxhcgm7iywrkb@treble> References: <20170629072512.pmkfnrgq4dci6od7@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170629072512.pmkfnrgq4dci6od7@gmail.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 29 Jun 2017 14:04:06 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 29, 2017 at 09:25:12AM +0200, Ingo Molnar wrote: > > +/* > > + * This struct contains a simplified version of the DWARF Call Frame > > + * Information standard. It contains only the necessary parts of the real > > + * DWARF, simplified for ease of access by the in-kernel unwinder. It tells > > + * the unwinder how to find the previous SP and BP (and sometimes entry regs) > > + * on the stack for a given code address (IP). Each instance of the struct > > + * corresponds to one or more code locations. > > + */ > > +struct undwarf { > > + short cfa_offset; > > + short bp_offset; > > + unsigned cfa_reg:4; > > + unsigned bp_reg:4; > > + unsigned type:2; > > +}; > > I never know straight away what 'CFA' stands for - could we please use natural > names, i.e. something like: > > struct undwarf { > u16 sp_offset; > u16 bp_offset; > unsigned sp_reg:4; > unsigned bp_reg:4; > unsigned type:2; > }; > > ... > > struct unwind_hint { > u32 ip; > u16 sp_offset; > u8 sp_reg; > u8 type; > }; > > ? > > Also note the slightly cleaner vertical alignment, plus the conversion to more > stable data types: I believe various bits of tooling (perf and so) will eventually > learn about undwarf, so having a well defined cross-arch data structure is > probably of advantage. I agree with all your suggestions. (Though if we want to make it truly cross-arch, 'bp' should be 'fp', for frame pointer. But there were some objections to that, so I'll leave it 'bp' for now.) > Since we are not bound by DWARF anymore, we might as well use readable names and > such? > > Plus, shouldn't we use __packed for 'struct undwarf' to minimize the structure's > size (to 6 bytes AFAICS?) - or is optimal packing of the main undwarf array > already guaranteed on every platform with this layout? Ah yes, it should definitely be packed (assuming that doesn't affect performance negatively). -- Josh