lkml.org 
[lkml]   [2017]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 7/7] DWARF: add the config option
    On Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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@suse.cz> 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

    \
     
     \ /
      Last update: 2017-05-10 21:17    [W:3.632 / U:0.280 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site