[RFC] bug: always show source-tree-relative paths in WARN()/BUG()
diff mbox series

Message ID 20190712010556.248319-1-briannorris@chromium.org
State New
Headers show
Series
  • [RFC] bug: always show source-tree-relative paths in WARN()/BUG()
Related show

Commit Message

Brian Norris July 12, 2019, 1:05 a.m. UTC
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(-)

Comments

Masahiro Yamada July 12, 2019, 1:13 a.m. UTC | #1
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
>
Brian Norris July 12, 2019, 1:23 a.m. UTC | #2
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
Masahiro Yamada July 12, 2019, 1:49 a.m. UTC | #3
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.
Brian Norris July 13, 2019, 1:46 a.m. UTC | #4
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

Patch
diff mbox series

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\"");