Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752294AbdF2HZX (ORCPT ); Thu, 29 Jun 2017 03:25:23 -0400 Received: from mail-wm0-f43.google.com ([74.125.82.43]:35222 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752026AbdF2HZR (ORCPT ); Thu, 29 Jun 2017 03:25:17 -0400 Date: Thu, 29 Jun 2017 09:25:12 +0200 From: Ingo Molnar To: Josh Poimboeuf 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: <20170629072512.pmkfnrgq4dci6od7@gmail.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3330 Lines: 103 * Josh Poimboeuf wrote: > +#ifndef _UNDWARF_TYPES_H > +#define _UNDWARF_TYPES_H > + > +/* > + * The UNDWARF_REG_* registers are base registers which are used to find other > + * registers on the stack. > + * > + * The CFA (call frame address) is the value of the stack pointer on the > + * previous frame, i.e. the caller's SP before it called the callee. > + * > + * The CFA is usually based on SP, unless a frame pointer has been saved, in > + * which case it's based on BP. > + * > + * BP is usually either based on CFA or is undefined (meaning its value didn't > + * change for the current frame). > + * > + * So the CFA base is usually either SP or BP, and the FP base is usually either > + * CFA or undefined. The rest of the base registers are needed for special > + * cases like entry code and gcc aligned stacks. > + */ > +#define UNDWARF_REG_UNDEFINED 0 > +#define UNDWARF_REG_CFA 1 > +#define UNDWARF_REG_DX 2 > +#define UNDWARF_REG_DI 3 > +#define UNDWARF_REG_BP 4 > +#define UNDWARF_REG_SP 5 > +#define UNDWARF_REG_R10 6 > +#define UNDWARF_REG_R13 7 > +#define UNDWARF_REG_BP_INDIRECT 8 > +#define UNDWARF_REG_SP_INDIRECT 9 > +#define UNDWARF_REG_MAX 15 > + > +/* > + * UNDWARF_TYPE_CFA: Indicates that cfa_reg+cfa_offset points to the caller's > + * stack pointer (aka the CFA in DWARF terms). Used for all callable > + * functions, i.e. all C code and all callable asm functions. > + * > + * UNDWARF_TYPE_REGS: Used in entry code to indicate that cfa_reg+cfa_offset > + * points to a fully populated pt_regs from a syscall, interrupt, or exception. > + * > + * UNDWARF_TYPE_REGS_IRET: Used in entry code to indicate that > + * cfa_reg+cfa_offset points to the iret return frame. > + */ > +#define UNDWARF_TYPE_CFA 0 > +#define UNDWARF_TYPE_REGS 1 > +#define UNDWARF_TYPE_REGS_IRET 2 > + > +/* > + * 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. 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? Thanks, Ingo