Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces

[Stephen Boyd]

Quoting Andy Shevchenko (2021-04-12 04:58:02)
> On Fri, Apr 09, 2021 at 06:52:52PM -0700, Stephen Boyd wrote:
> > Let's make kernel stacktraces easier to identify by including the build
> > ID[1] of a module if the stacktrace is printing a symbol from a module.
> > This makes it simpler for developers to locate a kernel module's full
> > debuginfo for a particular stacktrace. Combined with
> > scripts/decode_stracktrace.sh, a developer can download the matching
> > debuginfo from a debuginfod[2] server and find the exact file and line
> > number for the functions plus offsets in a stacktrace that match the
> > module. This is especially useful for pstore crash debugging where the
> > kernel crashes are recorded in something like console-ramoops and the
> > recovery kernel/modules are different or the debuginfo doesn't exist on
> > the device due to space concerns (the debuginfo can be too large for
> > space limited devices).
> > 
> > Originally, I put this on the %pS format, but that was quickly rejected
> > given that %pS is used in other places such as ftrace where build IDs
> > aren't meaningful. There was some discussions on the list to put every
> > module build ID into the "Modules linked in:" section of the stacktrace
> > message but that quickly becomes very hard to read once you have more
> > than three or four modules linked in. It also provides too much
> > information when we don't expect each module to be traversed in a
> > stacktrace. Having the build ID for modules that aren't important just
> > makes things messy. Splitting it to multiple lines for each module
> > quickly explodes the number of lines printed in an oops too, possibly
> > wrapping the warning off the console. And finally, trying to stash away
> > each module used in a callstack to provide the ID of each symbol printed
> > is cumbersome and would require changes to each architecture to stash
> > away modules and return their build IDs once unwinding has completed.
> > 
> > Instead, we opt for the simpler approach of introducing new printk
> > formats '%pS[R]b' for "pointer symbolic backtrace with module build ID"
> > and '%pBb' for "pointer backtrace with module build ID" and then
> > updating the few places in the architecture layer where the stacktrace
> > is printed to use this new format.
> > 
> > Example:
> 
> Can you trim a bit the example, so we will see only important lines.
> In such case you may provide "before" and "after" variants.
> 
> ...
> 
> > -     if (modname)
> > -             len += sprintf(buffer + len, " [%s]", modname);
> > +     if (modname) {
> > +             len += sprintf(buffer + len, " [%s", modname);
> 
> > +             /* build ID should match length of sprintf below */
> > +             BUILD_BUG_ON(BUILD_ID_SIZE_MAX != 20);
> 
> First of all, why not static_assert() defined near to the actual macro?

Which macro? BUILD_ID_SIZE_MAX? I tried static_assert() and it didn't
work for me but maybe I missed something. Why is static_assert()
preferred?

> 
> > +             if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) && add_buildid && buildid)
> > +                     len += sprintf(buffer + len, " %20phN", buildid);
> 
>                         len += sprintf(buffer + len, " %*phN", BUILD_ID_SIZE_MAX, buildid);
> 

Are you suggesting to use sprintf format here so that the size is part
of the printf? Sounds good to me. Thanks.