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

From: Ingo Molnar
Date: Thu Jun 29 2017 - 03:25:28 EST



* 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