| Message ID | 20190712010556.248319-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] bug: always show source-tree-relative paths in WARN()/BUG() | expand |
On Fri, Jul 12, 2019 at 10:06 AM Brian Norris <briannorris@chromium.org> wrote: > > When building out-of-tree (e.g., 'make O=...'), __FILE__ ends up being > an absolute path, and so WARN() and BUG() end up putting path names from > the build system into the log text. For example: > > # echo BUG > /sys/kernel/debug/provoke-crash/DIRECT > ... > kernel BUG at /mnt/host/source/[...]/drivers/misc/lkdtm/bugs.c:71! > > Not only is this excessively verbose, it also adds extra noise into > tools that might parse this output. (For example, if builder paths > change across versions, we suddenly get a "new" crash signature.) > > All in all, this looks much better as: > > kernel BUG at drivers/misc/lkdtm/bugs.c:71! > > It appears the Kbuild system is fairly entrenched in using > $(KBUILD_OUTPUT) for the ${CWD}, which necessarily means that the > preprocessor will get handed an absolute path. It seems the only > solution then, is to do some sort of post-processing on __FILE__. > > It so happens that lib/dynamic_debug.c already solves this sort of > problem, so I steal its solution for use in panic/warn/bug code as well. BTW, did you see this? commit a73619a845d5625079cc1b3b820f44c899618388 Author: Masahiro Yamada <yamada.masahiro@socionext.com> Date: Fri Mar 30 13:15:26 2018 +0900 kbuild: use -fmacro-prefix-map to make __FILE__ a relative path > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > I'd be happy to entertain better solutions to this problem, but so far, > I haven't been creative enough to come up with one. > > I'm also unsure of who best to address this to. If anyone has better > pointers, I'm all ears. > > include/linux/bug.h | 2 ++ > kernel/panic.c | 21 +++++++++++++++++++-- > lib/bug.c | 3 ++- > lib/dynamic_debug.c | 18 ++++-------------- > 4 files changed, 27 insertions(+), 17 deletions(-) > > diff --git a/include/linux/bug.h b/include/linux/bug.h > index fe5916550da8..6ab59e53801d 100644 > --- a/include/linux/bug.h > +++ b/include/linux/bug.h > @@ -76,4 +76,6 @@ static inline __must_check bool check_data_corruption(bool v) { return v; } > corruption; \ > })) > > +const char *trim_filepath_prefix(const char *path); > + > #endif /* _LINUX_BUG_H */ > diff --git a/kernel/panic.c b/kernel/panic.c > index 4d9f55bf7d38..0bed3101f049 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -546,6 +546,23 @@ struct warn_args { > va_list args; > }; > > +/** > + * trim_filepath_prefix - retrieve source-root relative path from a __FILE__ > + * @path: a __FILE__-like path argument. > + * Return: path relative to source root. > + */ > +const char *trim_filepath_prefix(const char *path) > +{ > + int skip = strlen(__FILE__) - strlen("kernel/panic.c"); > + > + BUILD_BUG_ON(strlen(__FILE__) < strlen("kernel/panic.c")); > + > + if (strncmp(path, __FILE__, skip)) > + skip = 0; /* prefix mismatch, don't skip */ > + > + return path + skip; > +} > + > void __warn(const char *file, int line, void *caller, unsigned taint, > struct pt_regs *regs, struct warn_args *args) > { > @@ -556,8 +573,8 @@ void __warn(const char *file, int line, void *caller, unsigned taint, > > if (file) > pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n", > - raw_smp_processor_id(), current->pid, file, line, > - caller); > + raw_smp_processor_id(), current->pid, > + trim_filepath_prefix(file), line, caller); > else > pr_warn("WARNING: CPU: %d PID: %d at %pS\n", > raw_smp_processor_id(), current->pid, caller); > diff --git a/lib/bug.c b/lib/bug.c > index 1077366f496b..2aa91d330451 100644 > --- a/lib/bug.c > +++ b/lib/bug.c > @@ -191,7 +191,8 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > printk(KERN_DEFAULT CUT_HERE); > > if (file) > - pr_crit("kernel BUG at %s:%u!\n", file, line); > + pr_crit("kernel BUG at %s:%u!\n", trim_filepath_prefix(file), > + line); > else > pr_crit("Kernel BUG at %pB [verbose debug info unavailable]\n", > (void *)bugaddr); > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index 8a16c2d498e9..0896f067ba17 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -13,6 +13,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ > > +#include <linux/bug.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/moduleparam.h> > @@ -67,17 +68,6 @@ static LIST_HEAD(ddebug_tables); > static int verbose; > module_param(verbose, int, 0644); > > -/* Return the path relative to source root */ > -static inline const char *trim_prefix(const char *path) > -{ > - int skip = strlen(__FILE__) - strlen("lib/dynamic_debug.c"); > - > - if (strncmp(path, __FILE__, skip)) > - skip = 0; /* prefix mismatch, don't skip */ > - > - return path + skip; > -} > - > static struct { unsigned flag:8; char opt_char; } opt_array[] = { > { _DPRINTK_FLAGS_PRINT, 'p' }, > { _DPRINTK_FLAGS_INCL_MODNAME, 'm' }, > @@ -164,7 +154,7 @@ static int ddebug_change(const struct ddebug_query *query, > !match_wildcard(query->filename, > kbasename(dp->filename)) && > !match_wildcard(query->filename, > - trim_prefix(dp->filename))) > + trim_filepath_prefix(dp->filename))) > continue; > > /* match against the function */ > @@ -199,7 +189,7 @@ static int ddebug_change(const struct ddebug_query *query, > #endif > dp->flags = newflags; > vpr_info("changed %s:%d [%s]%s =%s\n", > - trim_prefix(dp->filename), dp->lineno, > + trim_filepath_prefix(dp->filename), dp->lineno, > dt->mod_name, dp->function, > ddebug_describe_flags(dp, flagbuf, > sizeof(flagbuf))); > @@ -827,7 +817,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p) > } > > seq_printf(m, "%s:%u [%s]%s =%s \"", > - trim_prefix(dp->filename), dp->lineno, > + trim_filepath_prefix(dp->filename), dp->lineno, > iter->table->mod_name, dp->function, > ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf))); > seq_escape(m, dp->format, "\t\r\n\""); > -- > 2.22.0.410.gd8fdbe21b5-goog >
On Thu, Jul 11, 2019 at 6:14 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > BTW, did you see this? > > commit a73619a845d5625079cc1b3b820f44c899618388 > Author: Masahiro Yamada <yamada.masahiro@socionext.com> > Date: Fri Mar 30 13:15:26 2018 +0900 > > kbuild: use -fmacro-prefix-map to make __FILE__ a relative path Oh, wow, no I did not. If my reading is correct, that's GCC only? I've been using various combinations of newer (5.2) and older (4.14.y -- didn't have that patch) kernels, older GCC (doesn't have that feature AFAICT), and newer Clang (doesn't appear to have that feature). So I'm not totally sure if I ever actually tried a combo that *could* make use of that. But I may give it another shot. In the event that this is GCC-specific...I don't suppose I could convince anybody to expend any effort (e.g., taking a patch like mine) to solve it for the non-GCC world? Thanks for the tip, Brian
On Fri, Jul 12, 2019 at 10:23 AM Brian Norris <briannorris@chromium.org> wrote: > > On Thu, Jul 11, 2019 at 6:14 PM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > BTW, did you see this? > > > > commit a73619a845d5625079cc1b3b820f44c899618388 > > Author: Masahiro Yamada <yamada.masahiro@socionext.com> > > Date: Fri Mar 30 13:15:26 2018 +0900 > > > > kbuild: use -fmacro-prefix-map to make __FILE__ a relative path > > Oh, wow, no I did not. If my reading is correct, that's GCC only? I've > been using various combinations of newer (5.2) and older (4.14.y -- > didn't have that patch) kernels, older GCC (doesn't have that feature > AFAICT), and newer Clang (doesn't appear to have that feature). So I'm > not totally sure if I ever actually tried a combo that *could* make > use of that. But I may give it another shot. > > In the event that this is GCC-specific...I don't suppose I could > convince anybody to expend any effort (e.g., taking a patch like mine) > to solve it for the non-GCC world? > > Thanks for the tip, > Brian GCC 8 added this flag. So, it will be eventually all solved in the GCC world. Clang has not supported it yet... Trimming absolute path at run-time is no help for reducing the kernel image. Turning __FILE__ into a relative path at compile-time is better. I hope Clang people will consider to support it. I guess implementing this feature should not be so hard.
On Thu, Jul 11, 2019 at 6:50 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > GCC 8 added this flag. > So, it will be eventually all solved in the GCC world. Ack. > Clang has not supported it yet... That's what it appeared like. I've bugged our Clang-loving toolchain folks to see if we can get parity. > Trimming absolute path at run-time > is no help for reducing the kernel image. Sure, but that's not my stated goal. It would indeed be nicer though. I guess if no one else speaks up with a favorable word toward my RFC, I'll just see what I can do on the toolchain side. Thanks for the help, Brian
diff --git a/include/linux/bug.h b/include/linux/bug.h index fe5916550da8..6ab59e53801d 100644 --- a/include/linux/bug.h +++ b/include/linux/bug.h @@ -76,4 +76,6 @@ static inline __must_check bool check_data_corruption(bool v) { return v; } corruption; \ })) +const char *trim_filepath_prefix(const char *path); + #endif /* _LINUX_BUG_H */ diff --git a/kernel/panic.c b/kernel/panic.c index 4d9f55bf7d38..0bed3101f049 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -546,6 +546,23 @@ struct warn_args { va_list args; }; +/** + * trim_filepath_prefix - retrieve source-root relative path from a __FILE__ + * @path: a __FILE__-like path argument. + * Return: path relative to source root. + */ +const char *trim_filepath_prefix(const char *path) +{ + int skip = strlen(__FILE__) - strlen("kernel/panic.c"); + + BUILD_BUG_ON(strlen(__FILE__) < strlen("kernel/panic.c")); + + if (strncmp(path, __FILE__, skip)) + skip = 0; /* prefix mismatch, don't skip */ + + return path + skip; +} + void __warn(const char *file, int line, void *caller, unsigned taint, struct pt_regs *regs, struct warn_args *args) { @@ -556,8 +573,8 @@ void __warn(const char *file, int line, void *caller, unsigned taint, if (file) pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n", - raw_smp_processor_id(), current->pid, file, line, - caller); + raw_smp_processor_id(), current->pid, + trim_filepath_prefix(file), line, caller); else pr_warn("WARNING: CPU: %d PID: %d at %pS\n", raw_smp_processor_id(), current->pid, caller); diff --git a/lib/bug.c b/lib/bug.c index 1077366f496b..2aa91d330451 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -191,7 +191,8 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) printk(KERN_DEFAULT CUT_HERE); if (file) - pr_crit("kernel BUG at %s:%u!\n", file, line); + pr_crit("kernel BUG at %s:%u!\n", trim_filepath_prefix(file), + line); else pr_crit("Kernel BUG at %pB [verbose debug info unavailable]\n", (void *)bugaddr); diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index 8a16c2d498e9..0896f067ba17 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -13,6 +13,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ +#include <linux/bug.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/moduleparam.h> @@ -67,17 +68,6 @@ static LIST_HEAD(ddebug_tables); static int verbose; module_param(verbose, int, 0644); -/* Return the path relative to source root */ -static inline const char *trim_prefix(const char *path) -{ - int skip = strlen(__FILE__) - strlen("lib/dynamic_debug.c"); - - if (strncmp(path, __FILE__, skip)) - skip = 0; /* prefix mismatch, don't skip */ - - return path + skip; -} - static struct { unsigned flag:8; char opt_char; } opt_array[] = { { _DPRINTK_FLAGS_PRINT, 'p' }, { _DPRINTK_FLAGS_INCL_MODNAME, 'm' }, @@ -164,7 +154,7 @@ static int ddebug_change(const struct ddebug_query *query, !match_wildcard(query->filename, kbasename(dp->filename)) && !match_wildcard(query->filename, - trim_prefix(dp->filename))) + trim_filepath_prefix(dp->filename))) continue; /* match against the function */ @@ -199,7 +189,7 @@ static int ddebug_change(const struct ddebug_query *query, #endif dp->flags = newflags; vpr_info("changed %s:%d [%s]%s =%s\n", - trim_prefix(dp->filename), dp->lineno, + trim_filepath_prefix(dp->filename), dp->lineno, dt->mod_name, dp->function, ddebug_describe_flags(dp, flagbuf, sizeof(flagbuf))); @@ -827,7 +817,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p) } seq_printf(m, "%s:%u [%s]%s =%s \"", - trim_prefix(dp->filename), dp->lineno, + trim_filepath_prefix(dp->filename), dp->lineno, iter->table->mod_name, dp->function, ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf))); seq_escape(m, dp->format, "\t\r\n\"");
When building out-of-tree (e.g., 'make O=...'), __FILE__ ends up being an absolute path, and so WARN() and BUG() end up putting path names from the build system into the log text. For example: # echo BUG > /sys/kernel/debug/provoke-crash/DIRECT ... kernel BUG at /mnt/host/source/[...]/drivers/misc/lkdtm/bugs.c:71! Not only is this excessively verbose, it also adds extra noise into tools that might parse this output. (For example, if builder paths change across versions, we suddenly get a "new" crash signature.) All in all, this looks much better as: kernel BUG at drivers/misc/lkdtm/bugs.c:71! It appears the Kbuild system is fairly entrenched in using $(KBUILD_OUTPUT) for the ${CWD}, which necessarily means that the preprocessor will get handed an absolute path. It seems the only solution then, is to do some sort of post-processing on __FILE__. It so happens that lib/dynamic_debug.c already solves this sort of problem, so I steal its solution for use in panic/warn/bug code as well. Signed-off-by: Brian Norris <briannorris@chromium.org> --- I'd be happy to entertain better solutions to this problem, but so far, I haven't been creative enough to come up with one. I'm also unsure of who best to address this to. If anyone has better pointers, I'm all ears. include/linux/bug.h | 2 ++ kernel/panic.c | 21 +++++++++++++++++++-- lib/bug.c | 3 ++- lib/dynamic_debug.c | 18 ++++-------------- 4 files changed, 27 insertions(+), 17 deletions(-)