Re: [PATCH 7/7] DWARF: add the config option

[Andy Lutomirski]

On Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Fri, May 05, 2017 at 12:57:11PM -0700, Linus Torvalds wrote:
>> On Fri, May 5, 2017 at 5:22 AM, Jiri Slaby <jslaby@xxxxxxx> wrote:
>> > The DWARF unwinder is in place and ready. So introduce the config option
>> > to allow users to enable it. It is by default off due to missing
>> > assembly annotations.
>>
>> Who actually ends up using this?
>>
>> Because from the last time we had fancy unwindoers, and all the
>> problems it caused for oops handling with absolutely _zero_ upsides
>> ever, I do not ever again want to see fancy unwinders with complex
>> state machine handling used by the oopsing code.
>>
>> The fact that it gets disabled for KASAN also makes me suspicious. It
>> basically means that now all the accesses it does are not even
>> validated.
>>
>> The fact that the most of the code seems to be disabled for the first
>> six patches, and then just enabled in the last patch, also seems to
>> mean that the series also gets no bisection coverage or testing that
>> the individual patches make any sense. (ie there's a lot of code
>> inside "CONFIG_DWARF_UNWIND" in the early patches but that config
>> option cannot even be enabled until the last patch).
>>
>> We used to have nasty issues with not just missing dwarf info, but
>> also actively *wrong* dwarf info. Compiler versions that generated
>> subtly wrong info, because nobody actually really depended on it, and
>> the people who had tested it seldom did the kinds of things we do in
>> the kernel (eg inline asms etc).
>>
>> So I'm personally still very suspicious of these things.
>>
>> Last time I had huge issues with people also always blaming *anything*
>> else than that unwinder. It was always "oh, somebody wrote asm without
>> getting it right". Or "oh, the compiler generated bad tables, it's not
>> *my* fault that now the kernel oopsing code no longer works".
>>
>> When I asked for more stricter debug table validation to avoid issues,
>> it was always "oh, we fixed it, no worries", and then two months later
>> somebody hit another issue.
>>
>> Put another way; the last time we did crazy stuff like this, it got
>> reverted. For a damn good reason, despite some people being in denial
>> about those reasons.
>
> Here's another possible idea that's been rattling around in my head.
> It's purely theoretical at this point, so I don't know for sure that it
> would work.  But I haven't been able to find any major issues with it
> yet.
>
> DWARF is great for debuggers.  It helps you find all the registers on
> the stack, so you can see function arguments and local variables.  All
> expressed in a nice compact format.
>
> But that's overkill for unwinders.  We don't need all those registers,
> and the state machine is too complicated.  Unwinders basically only need
> to know one thing: given an instruction address and a stack pointer,
> where is the caller's stack frame?
>
> I'm thinking/hoping that information can be expressed in a simple, easy
> to parse, reasonably sized data structure.  Something like a sorted
> array of this:
>
>         struct undwarf {
>                 unsigned int ip;        /* instruction pointer (relative offset from base) */
>                 unsigned prev_frame:13; /* offset to previous frame from current stack pointer */
>                 unsigned regs:1;        /* whether prev_frame contains entry regs (regs->ip) */
>                 unsigned align:2;       /* some details for dealing with gcc stack realignment */
>         } __packed;
>
>         extern struct undwarf undwarves[];

Some comments in case you're actually planning to do this:

'unsigned int ip' is the majority of the size of this thing.  It might
be worth trying to store a lot fewer bits.  You could split the
structure into:

struct undwarf_header {
  unsigned long start_ip;
  unsigned align:2;  /* i'm assuming this rarely changes */
  ...;
  unsigned int offset_to_details;
};

and

struct undwarf_details {
  unsigned short ip_offset;
  unsigned short prev_frame;
};

and you'd find the details by first looking up the last header before
the ip and then finding the details starting at (uintptr_t)header +
header->offset_to_details.

Also, don't you need some indication of which reg is the base from
which you find previous frame?  After all, sometimes GCC will emit a
frame pointer even in an otherwise frame-pointer-omitting kernel.

--Andy