Re: [PATCH v2 4/8] objtool: add undwarf debuginfo generation

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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