[v2,5/9] objtool: Add verbose option for disassembling affected functions
Commit Message
When a warning is associated with a function, add an option to
disassemble that function.
This makes it easier for reporters to submit the information needed to
diagnose objtool warnings.
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
tools/objtool/Documentation/objtool.txt | 5 ++
tools/objtool/builtin-check.c | 5 ++
tools/objtool/check.c | 77 +++++++++++++++++++++++++
tools/objtool/include/objtool/builtin.h | 1 +
4 files changed, 88 insertions(+)
Comments
On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> When a warning is associated with a function, add an option to
> disassemble that function.
>
> This makes it easier for reporters to submit the information needed to
> diagnose objtool warnings.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
> tools/objtool/Documentation/objtool.txt | 5 ++
> tools/objtool/builtin-check.c | 5 ++
> tools/objtool/check.c | 77 +++++++++++++++++++++++++
> tools/objtool/include/objtool/builtin.h | 1 +
> 4 files changed, 88 insertions(+)
>
> diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
> index 8e53fc6735ef..4d6c5acde7a3 100644
> --- a/tools/objtool/Documentation/objtool.txt
> +++ b/tools/objtool/Documentation/objtool.txt
> @@ -244,6 +244,11 @@ To achieve the validation, objtool enforces the following rules:
> Objtool warnings
> ----------------
>
> +NOTE: When requesting help with an objtool warning, please recreate with
> +OBJTOOL_VERBOSE=1 (e.g., "make OBJTOOL_VERBOSE=1") and send the full
> +output, including any disassembly below the warning, to the objtool
> +maintainers.
> +
> For asm files, if you're getting an error which doesn't make sense,
> first make sure that the affected code follows the above rules.
>
> diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
> index 7c175198d09f..5e21cfb7661d 100644
> --- a/tools/objtool/builtin-check.c
> +++ b/tools/objtool/builtin-check.c
> @@ -93,6 +93,7 @@ static const struct option check_options[] = {
> OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
> OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
> OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
> + OPT_BOOLEAN('v', "verbose", &opts.verbose, "verbose warnings"),
>
> OPT_END(),
> };
> @@ -118,6 +119,10 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
> parse_options(envc, envv, check_options, env_usage, 0);
> }
>
> + env = getenv("OBJTOOL_VERBOSE");
> + if (env && !strcmp(env, "1"))
> + opts.verbose = true;
> +
> argc = parse_options(argc, argv, check_options, usage, 0);
> if (argc != 1)
> usage_with_options(usage, check_options);
No real objection; but I do feel obliged to point out that:
OBJTOOL_ARGS="-v" achieves much the same.
On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> + objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
> + "BEGIN { split(_funcs, funcs); }"
> + "/^$/ { func_match = 0; }"
> + "/<.*>:/ { "
> + "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
> + "for (i in funcs) {"
> + "if (funcs[i] == f) {"
> + "func_match = 1;"
> + "base = strtonum(\"0x\" $1);"
> + "break;"
> + "}"
> + "}"
> + "}"
> + "{"
> + "if (func_match) {"
> + "addr = strtonum(\"0x\" $1);"
> + "printf(\"%%04x \", addr - base);"
> + "print;"
> + "}"
> + "}' 1>&2";
Do we want to have scripts/objdump-func.awk and use that to avoid the
duplication and eventual divergence of these awk thingies?
On Thu, 13 Apr 2023, Peter Zijlstra wrote:
> On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> > + objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
> > + "BEGIN { split(_funcs, funcs); }"
> > + "/^$/ { func_match = 0; }"
> > + "/<.*>:/ { "
> > + "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
> > + "for (i in funcs) {"
> > + "if (funcs[i] == f) {"
> > + "func_match = 1;"
> > + "base = strtonum(\"0x\" $1);"
> > + "break;"
> > + "}"
> > + "}"
> > + "}"
> > + "{"
> > + "if (func_match) {"
> > + "addr = strtonum(\"0x\" $1);"
> > + "printf(\"%%04x \", addr - base);"
> > + "print;"
> > + "}"
> > + "}' 1>&2";
>
> Do we want to have scripts/objdump-func.awk and use that to avoid the
> duplication and eventual divergence of these awk thingies?
I vote for that as well. To keep everything in sync can be a nightmare.
Miroslav
On Thu, Apr 13, 2023 at 10:05:27AM +0200, Peter Zijlstra wrote:
> > @@ -118,6 +119,10 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
> > parse_options(envc, envv, check_options, env_usage, 0);
> > }
> >
> > + env = getenv("OBJTOOL_VERBOSE");
> > + if (env && !strcmp(env, "1"))
> > + opts.verbose = true;
> > +
> > argc = parse_options(argc, argv, check_options, usage, 0);
> > if (argc != 1)
> > usage_with_options(usage, check_options);
>
> No real objection; but I do feel obliged to point out that:
> OBJTOOL_ARGS="-v" achieves much the same.
But OBJTOOL_VERBOSE=1 just rolls off the fingers :-)
On Thu, Apr 13, 2023 at 04:03:07PM +0200, Miroslav Benes wrote:
> On Thu, 13 Apr 2023, Peter Zijlstra wrote:
>
> > On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> > > + objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
> > > + "BEGIN { split(_funcs, funcs); }"
> > > + "/^$/ { func_match = 0; }"
> > > + "/<.*>:/ { "
> > > + "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
> > > + "for (i in funcs) {"
> > > + "if (funcs[i] == f) {"
> > > + "func_match = 1;"
> > > + "base = strtonum(\"0x\" $1);"
> > > + "break;"
> > > + "}"
> > > + "}"
> > > + "}"
> > > + "{"
> > > + "if (func_match) {"
> > > + "addr = strtonum(\"0x\" $1);"
> > > + "printf(\"%%04x \", addr - base);"
> > > + "print;"
> > > + "}"
> > > + "}' 1>&2";
> >
> > Do we want to have scripts/objdump-func.awk and use that to avoid the
> > duplication and eventual divergence of these awk thingies?
>
> I vote for that as well. To keep everything in sync can be a nightmare.
Reasons I did it this way for v2:
1) The awk implementation is slightly different (this one doesn't match
*.cold, etc)
2) Objtool is self-sufficient (doesn't need any other files)
3) Any changes to the objdump-func interface might break objtool
4) It's clearly bug-free and self-contained and I don't expect the code
to change much in the future
@@ -244,6 +244,11 @@ To achieve the validation, objtool enforces the following rules:
Objtool warnings
----------------
+NOTE: When requesting help with an objtool warning, please recreate with
+OBJTOOL_VERBOSE=1 (e.g., "make OBJTOOL_VERBOSE=1") and send the full
+output, including any disassembly below the warning, to the objtool
+maintainers.
+
For asm files, if you're getting an error which doesn't make sense,
first make sure that the affected code follows the above rules.
@@ -93,6 +93,7 @@ static const struct option check_options[] = {
OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
+ OPT_BOOLEAN('v', "verbose", &opts.verbose, "verbose warnings"),
OPT_END(),
};
@@ -118,6 +119,10 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
parse_options(envc, envv, check_options, env_usage, 0);
}
+ env = getenv("OBJTOOL_VERBOSE");
+ if (env && !strcmp(env, "1"))
+ opts.verbose = true;
+
argc = parse_options(argc, argv, check_options, usage, 0);
if (argc != 1)
usage_with_options(usage, check_options);
@@ -4509,6 +4509,81 @@ static int validate_reachable_instructions(struct objtool_file *file)
return warnings;
}
+/* 'funcs' is a space-separated list of function names */
+static int disas_funcs(const char *funcs)
+{
+ const char *objdump_str, *cross_compile;
+ int size, ret;
+ char *cmd;
+
+ cross_compile = getenv("CROSS_COMPILE");
+
+ objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
+ "BEGIN { split(_funcs, funcs); }"
+ "/^$/ { func_match = 0; }"
+ "/<.*>:/ { "
+ "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
+ "for (i in funcs) {"
+ "if (funcs[i] == f) {"
+ "func_match = 1;"
+ "base = strtonum(\"0x\" $1);"
+ "break;"
+ "}"
+ "}"
+ "}"
+ "{"
+ "if (func_match) {"
+ "addr = strtonum(\"0x\" $1);"
+ "printf(\"%%04x \", addr - base);"
+ "print;"
+ "}"
+ "}' 1>&2";
+
+ /* fake snprintf() to calculate the size */
+ size = snprintf(NULL, 0, objdump_str, cross_compile, objname, funcs) + 1;
+ if (size <= 0) {
+ WARN("objdump string size calculation failed");
+ return -1;
+ }
+
+ cmd = malloc(size);
+
+ /* real snprintf() */
+ snprintf(cmd, size, objdump_str, cross_compile, objname, funcs);
+ ret = system(cmd);
+ if (ret) {
+ WARN("disassembly failed: %d", ret);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int disas_warned_funcs(struct objtool_file *file)
+{
+ struct symbol *sym;
+ char *funcs = NULL, *tmp;
+
+ for_each_sym(file, sym) {
+ if (sym->warned) {
+ if (!funcs) {
+ funcs = malloc(strlen(sym->name) + 1);
+ strcpy(funcs, sym->name);
+ } else {
+ tmp = malloc(strlen(funcs) + strlen(sym->name) + 2);
+ sprintf(tmp, "%s %s", funcs, sym->name);
+ free(funcs);
+ funcs = tmp;
+ }
+ }
+ }
+
+ if (funcs)
+ disas_funcs(funcs);
+
+ return 0;
+}
+
int check(struct objtool_file *file)
{
int ret, warnings = 0;
@@ -4646,6 +4721,8 @@ int check(struct objtool_file *file)
warnings += ret;
}
+ if (opts.verbose)
+ disas_warned_funcs(file);
if (opts.stats) {
printf("nr_insns_visited: %ld\n", nr_insns_visited);
@@ -37,6 +37,7 @@ struct opts {
bool no_unreachable;
bool sec_address;
bool stats;
+ bool verbose;
};
extern struct opts opts;