* [RFC 01/13] objtool: Move disassembly functions to a separated file
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-06 15:34 ` [RFC 02/13] objtool: Create disassembly context Alexandre Chartre
` (13 subsequent siblings)
14 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
objtool disassembles functions which have warnings. Move the code
to do that to a dedicated file. The code is just moved, it is not
changed.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/Build | 1 +
tools/objtool/check.c | 81 ----------------------
tools/objtool/disas.c | 90 +++++++++++++++++++++++++
tools/objtool/include/objtool/objtool.h | 2 +
4 files changed, 93 insertions(+), 81 deletions(-)
create mode 100644 tools/objtool/disas.c
diff --git a/tools/objtool/Build b/tools/objtool/Build
index a3cdf8af6635..677bf9148cba 100644
--- a/tools/objtool/Build
+++ b/tools/objtool/Build
@@ -7,6 +7,7 @@ objtool-y += special.o
objtool-y += builtin-check.o
objtool-y += elf.o
objtool-y += objtool.o
+objtool-y += disas.o
objtool-$(BUILD_ORC) += orc_gen.o
objtool-$(BUILD_ORC) += orc_dump.o
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f23bdda737aa..bd1974717fa3 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4561,87 +4561,6 @@ static int validate_reachable_instructions(struct objtool_file *file)
return warnings;
}
-/* 'funcs' is a space-separated list of function names */
-static void disas_funcs(const char *funcs)
-{
- const char *objdump_str, *cross_compile;
- int size, ret;
- char *cmd;
-
- cross_compile = getenv("CROSS_COMPILE");
- if (!cross_compile)
- 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;
- }
-
- 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;
- }
-}
-
-static void 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);
- if (!funcs) {
- ERROR_GLIBC("malloc");
- return;
- }
- strcpy(funcs, sym->name);
- } else {
- tmp = malloc(strlen(funcs) + strlen(sym->name) + 2);
- if (!tmp) {
- ERROR_GLIBC("malloc");
- return;
- }
- sprintf(tmp, "%s %s", funcs, sym->name);
- free(funcs);
- funcs = tmp;
- }
- }
- }
-
- if (funcs)
- disas_funcs(funcs);
-}
-
struct insn_chunk {
void *addr;
struct insn_chunk *next;
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
new file mode 100644
index 000000000000..77de46beb496
--- /dev/null
+++ b/tools/objtool/disas.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2015-2017 Josh Poimboeuf <jpoimboe@redhat.com>
+ */
+
+#include <objtool/arch.h>
+#include <objtool/warn.h>
+
+#include <linux/string.h>
+
+/* 'funcs' is a space-separated list of function names */
+static void disas_funcs(const char *funcs)
+{
+ const char *objdump_str, *cross_compile;
+ int size, ret;
+ char *cmd;
+
+ cross_compile = getenv("CROSS_COMPILE");
+ if (!cross_compile)
+ 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;
+ }
+
+ 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;
+ }
+}
+
+void 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);
+ if (!funcs) {
+ ERROR_GLIBC("malloc");
+ return;
+ }
+ strcpy(funcs, sym->name);
+ } else {
+ tmp = malloc(strlen(funcs) + strlen(sym->name) + 2);
+ if (!tmp) {
+ ERROR_GLIBC("malloc");
+ return;
+ }
+ sprintf(tmp, "%s %s", funcs, sym->name);
+ free(funcs);
+ funcs = tmp;
+ }
+ }
+ }
+
+ if (funcs)
+ disas_funcs(funcs);
+}
diff --git a/tools/objtool/include/objtool/objtool.h b/tools/objtool/include/objtool/objtool.h
index c0dc86a78ff6..4d3e94b70fd8 100644
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -47,4 +47,6 @@ int check(struct objtool_file *file);
int orc_dump(const char *objname);
int orc_create(struct objtool_file *file);
+void disas_warned_funcs(struct objtool_file *file);
+
#endif /* _OBJTOOL_H */
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* [RFC 02/13] objtool: Create disassembly context
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
2025-06-06 15:34 ` [RFC 01/13] objtool: Move disassembly functions to a separated file Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-10 21:12 ` Josh Poimboeuf
2025-06-06 15:34 ` [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump Alexandre Chartre
` (12 subsequent siblings)
14 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
Create a structure to store information for disassembling functions.
For now, it is just a wrapper around an objtool file.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 5 +++-
tools/objtool/disas.c | 33 +++++++++++++++++++++++--
tools/objtool/include/objtool/objtool.h | 5 +++-
3 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index bd1974717fa3..085fcc1b643b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4591,6 +4591,7 @@ static void free_insns(struct objtool_file *file)
int check(struct objtool_file *file)
{
+ struct disas_context *disas_ctx;
int ret = 0, warnings = 0;
arch_initial_func_cfi_state(&initial_func_cfi);
@@ -4720,7 +4721,9 @@ int check(struct objtool_file *file)
if (opts.werror && warnings)
WARN("%d warning(s) upgraded to errors", warnings);
print_args();
- disas_warned_funcs(file);
+ disas_ctx = disas_context_create(file);
+ disas_warned_funcs(disas_ctx);
+ disas_context_destroy(disas_ctx);
}
return ret;
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index 77de46beb496..ed74554bccbf 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -8,6 +8,30 @@
#include <linux/string.h>
+struct disas_context {
+ struct objtool_file *file;
+};
+
+struct disas_context *disas_context_create(struct objtool_file *file)
+{
+ struct disas_context *dctx;
+
+ dctx = malloc(sizeof(*dctx));
+ if (!dctx) {
+ WARN("failed too allocate disassembly context\n");
+ return NULL;
+ }
+
+ dctx->file = file;
+
+ return dctx;
+}
+
+void disas_context_destroy(struct disas_context *dctx)
+{
+ free(dctx);
+}
+
/* 'funcs' is a space-separated list of function names */
static void disas_funcs(const char *funcs)
{
@@ -58,12 +82,17 @@ static void disas_funcs(const char *funcs)
}
}
-void disas_warned_funcs(struct objtool_file *file)
+void disas_warned_funcs(struct disas_context *dctx)
{
struct symbol *sym;
char *funcs = NULL, *tmp;
- for_each_sym(file, sym) {
+ if (!dctx) {
+ ERROR("disassembly context is not defined");
+ return;
+ }
+
+ for_each_sym(dctx->file, sym) {
if (sym->warned) {
if (!funcs) {
funcs = malloc(strlen(sym->name) + 1);
diff --git a/tools/objtool/include/objtool/objtool.h b/tools/objtool/include/objtool/objtool.h
index 4d3e94b70fd8..f5ab71f07f5c 100644
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -47,6 +47,9 @@ int check(struct objtool_file *file);
int orc_dump(const char *objname);
int orc_create(struct objtool_file *file);
-void disas_warned_funcs(struct objtool_file *file);
+struct disas_context;
+struct disas_context *disas_context_create(struct objtool_file *file);
+void disas_context_destroy(struct disas_context *dctx);
+void disas_warned_funcs(struct disas_context *dctx);
#endif /* _OBJTOOL_H */
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 02/13] objtool: Create disassembly context
2025-06-06 15:34 ` [RFC 02/13] objtool: Create disassembly context Alexandre Chartre
@ 2025-06-10 21:12 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-10 21:12 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:29PM +0200, Alexandre Chartre wrote:
> +struct disas_context *disas_context_create(struct objtool_file *file)
> +{
> + struct disas_context *dctx;
> +
> + dctx = malloc(sizeof(*dctx));
> + if (!dctx) {
> + WARN("failed too allocate disassembly context\n");
> + return NULL;
"too" -> "to".
Also, no newline needed for objtool warnings/error strings.
Also, might want to use WARN_GLIBC() here.
> +void disas_context_destroy(struct disas_context *dctx)
> +{
> + free(dctx);
> +}
In general we try to avoid freeing memory in objtool for performance
reasons, though this is the error path so I suppose it's harmless.
> -void disas_warned_funcs(struct objtool_file *file)
> +void disas_warned_funcs(struct disas_context *dctx)
> {
> struct symbol *sym;
> char *funcs = NULL, *tmp;
>
> - for_each_sym(file, sym) {
> + if (!dctx) {
> + ERROR("disassembly context is not defined");
> + return;
This will have come from the error in disas_context_create(), no need to
print an additional error.
> +++ b/tools/objtool/include/objtool/objtool.h
> @@ -47,6 +47,9 @@ int check(struct objtool_file *file);
> int orc_dump(const char *objname);
> int orc_create(struct objtool_file *file);
>
> -void disas_warned_funcs(struct objtool_file *file);
> +struct disas_context;
> +struct disas_context *disas_context_create(struct objtool_file *file);
> +void disas_context_destroy(struct disas_context *dctx);
> +void disas_warned_funcs(struct disas_context *dctx);
Can these go in a new disas.h file?
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
2025-06-06 15:34 ` [RFC 01/13] objtool: Move disassembly functions to a separated file Alexandre Chartre
2025-06-06 15:34 ` [RFC 02/13] objtool: Create disassembly context Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-10 21:22 ` Josh Poimboeuf
2025-06-11 12:23 ` Peter Zijlstra
2025-06-06 15:34 ` [RFC 04/13] objtool: Print symbol during disassembly Alexandre Chartre
` (11 subsequent siblings)
14 siblings, 2 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
objtool executes the objdump command to disassemble code. Use libopcodes
instead to have more control about the disassembly scope and output.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/Makefile | 2 +-
tools/objtool/arch/loongarch/decode.c | 6 +
tools/objtool/arch/powerpc/decode.c | 6 +
tools/objtool/arch/x86/decode.c | 7 +
tools/objtool/check.c | 4 +-
tools/objtool/disas.c | 186 +++++++++++++++---------
tools/objtool/include/objtool/arch.h | 5 +
tools/objtool/include/objtool/check.h | 5 +
tools/objtool/include/objtool/objtool.h | 4 +
9 files changed, 154 insertions(+), 71 deletions(-)
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 8c20361dd100..00350fc7c662 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -34,7 +34,7 @@ INCLUDES := -I$(srctree)/tools/include \
# is passed here to match a legacy behavior.
WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
OBJTOOL_CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
-OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
+OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
# Allow old libelf to be used:
elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(HOSTCC) $(OBJTOOL_CFLAGS) -x c -E - 2>/dev/null | grep elf_getshdr)
diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
index b6fdc68053cc..bf5ac6750512 100644
--- a/tools/objtool/arch/loongarch/decode.c
+++ b/tools/objtool/arch/loongarch/decode.c
@@ -386,4 +386,10 @@ unsigned long arch_jump_table_sym_offset(struct reloc *reloc, struct reloc *tabl
default:
return reloc->sym->offset + reloc_addend(reloc);
}
+
+int arch_disas_info_init(struct disassemble_info *dinfo)
+{
+ return disas_info_init(dinfo, bfd_arch_loongarch,
+ bfd_mach_loongarch32, bfd_mach_loongarch64,
+ NULL);
}
diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
index c851c51d4bd3..c0fcab2d643c 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -127,4 +127,10 @@ unsigned int arch_reloc_size(struct reloc *reloc)
default:
return 8;
}
+
+int arch_disas_info_init(struct disassemble_info *dinfo)
+{
+ return disas_info_init(dinfo, bfd_arch_powerpc,
+ bfd_mach_ppc, bfd_mach_ppc64,
+ NULL);
}
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 98c4713c1b09..6c13c67ed9b9 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -880,3 +880,10 @@ unsigned int arch_reloc_size(struct reloc *reloc)
return 8;
}
}
+
+int arch_disas_info_init(struct disassemble_info *dinfo)
+{
+ return disas_info_init(dinfo, bfd_arch_i386,
+ bfd_mach_i386_i386, bfd_mach_x86_64,
+ "att");
+}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 085fcc1b643b..9cfac23185b8 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4701,8 +4701,6 @@ int check(struct objtool_file *file)
goto out;
}
- free_insns(file);
-
if (opts.stats) {
printf("nr_insns_visited: %ld\n", nr_insns_visited);
printf("nr_cfi: %ld\n", nr_cfi);
@@ -4726,5 +4724,7 @@ int check(struct objtool_file *file)
disas_context_destroy(disas_ctx);
}
+ free_insns(file);
+
return ret;
}
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index ed74554bccbf..f2eb1050ce11 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -4,17 +4,52 @@
*/
#include <objtool/arch.h>
+#include <objtool/check.h>
#include <objtool/warn.h>
+#include <bfd.h>
#include <linux/string.h>
+#include <tools/dis-asm-compat.h>
struct disas_context {
struct objtool_file *file;
+ disassembler_ftype disassembler;
+ struct disassemble_info info;
};
+/*
+ * Initialize disassemble info arch, mach (32 or 64-bit) and options.
+ */
+int disas_info_init(struct disassemble_info *dinfo,
+ int arch, int mach32, int mach64,
+ const char *options)
+{
+ struct disas_context *dctx = dinfo->application_data;
+ struct objtool_file *file = dctx->file;
+
+ dinfo->arch = arch;
+
+ switch (file->elf->ehdr.e_ident[EI_CLASS]) {
+ case ELFCLASS32:
+ dinfo->mach = mach32;
+ break;
+ case ELFCLASS64:
+ dinfo->mach = mach64;
+ break;
+ default:
+ return -1;
+ }
+
+ dinfo->disassembler_options = options;
+
+ return 0;
+}
+
struct disas_context *disas_context_create(struct objtool_file *file)
{
struct disas_context *dctx;
+ struct disassemble_info *dinfo;
+ int err;
dctx = malloc(sizeof(*dctx));
if (!dctx) {
@@ -23,8 +58,49 @@ struct disas_context *disas_context_create(struct objtool_file *file)
}
dctx->file = file;
+ dinfo = &dctx->info;
+
+ init_disassemble_info_compat(dinfo, stdout,
+ (fprintf_ftype)fprintf,
+ fprintf_styled);
+
+ dinfo->read_memory_func = buffer_read_memory;
+ dinfo->application_data = dctx;
+
+ /*
+ * bfd_openr() is not used to avoid doing ELF data processing
+ * and caching that has already being done. Here, we just need
+ * to identify the target file so we call an arch specific
+ * function to fill some disassemble info (arch, mach).
+ */
+
+ dinfo->arch = bfd_arch_unknown;
+ dinfo->mach = 0;
+
+ err = arch_disas_info_init(dinfo);
+ if (err || dinfo->arch == bfd_arch_unknown || dinfo->mach == 0) {
+ WARN("failed to init disassembly arch\n");
+ goto error;
+ }
+
+ dinfo->endian = (file->elf->ehdr.e_ident[EI_DATA] == ELFDATA2MSB) ?
+ BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
+
+ disassemble_init_for_target(dinfo);
+
+ dctx->disassembler = disassembler(dinfo->arch,
+ dinfo->endian == BFD_ENDIAN_BIG,
+ dinfo->mach, NULL);
+ if (!dctx->disassembler) {
+ WARN("failed to create disassembler function\n");
+ goto error;
+ }
return dctx;
+
+error:
+ free(dctx);
+ return NULL;
}
void disas_context_destroy(struct disas_context *dctx)
@@ -32,60 +108,54 @@ void disas_context_destroy(struct disas_context *dctx)
free(dctx);
}
-/* 'funcs' is a space-separated list of function names */
-static void disas_funcs(const char *funcs)
+/*
+ * Disassemble a single instruction. Return the size of the instruction.
+ */
+static size_t disas_insn(struct disas_context *dctx,
+ struct instruction *insn)
{
- const char *objdump_str, *cross_compile;
- int size, ret;
- char *cmd;
-
- cross_compile = getenv("CROSS_COMPILE");
- if (!cross_compile)
- 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;
- }
-
- cmd = malloc(size);
+ disassembler_ftype disasm = dctx->disassembler;
+ struct disassemble_info *dinfo = &dctx->info;
+
+ /*
+ * Set the disassembler buffer to read data from the section
+ * containing the instruction to disassemble.
+ */
+ dinfo->buffer = insn->sec->data->d_buf;
+ dinfo->buffer_vma = 0;
+ dinfo->buffer_length = insn->sec->sh.sh_size;
+
+ return disasm(insn->offset, &dctx->info);
+}
- /* real snprintf() */
- snprintf(cmd, size, objdump_str, cross_compile, objname, funcs);
- ret = system(cmd);
- if (ret) {
- WARN("disassembly failed: %d", ret);
- return;
+/*
+ * Disassemble a function.
+ */
+static void disas_func(struct disas_context *dctx, struct symbol *func)
+{
+ struct instruction *insn;
+ size_t addr, size;
+
+ printf("%s:\n", func->name);
+ sym_for_each_insn(dctx->file, func, insn) {
+
+ addr = insn->offset;
+ printf(" %6lx: %s+0x%-6lx ",
+ addr, func->name, addr - func->offset);
+ size = disas_insn(dctx, insn);
+ printf("\n");
+ if (size != insn->len)
+ WARN("inconsistent insn size (%ld and %d)\n", size, insn->len);
}
+ printf("\n");
}
+/*
+ * Disassemble all warned functions.
+ */
void disas_warned_funcs(struct disas_context *dctx)
{
struct symbol *sym;
- char *funcs = NULL, *tmp;
if (!dctx) {
ERROR("disassembly context is not defined");
@@ -93,27 +163,7 @@ void disas_warned_funcs(struct disas_context *dctx)
}
for_each_sym(dctx->file, sym) {
- if (sym->warned) {
- if (!funcs) {
- funcs = malloc(strlen(sym->name) + 1);
- if (!funcs) {
- ERROR_GLIBC("malloc");
- return;
- }
- strcpy(funcs, sym->name);
- } else {
- tmp = malloc(strlen(funcs) + strlen(sym->name) + 2);
- if (!tmp) {
- ERROR_GLIBC("malloc");
- return;
- }
- sprintf(tmp, "%s %s", funcs, sym->name);
- free(funcs);
- funcs = tmp;
- }
- }
+ if (sym->warned)
+ disas_func(dctx, sym);
}
-
- if (funcs)
- disas_funcs(funcs);
}
diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
index 01ef6f415adf..aecf8fc29571 100644
--- a/tools/objtool/include/objtool/arch.h
+++ b/tools/objtool/include/objtool/arch.h
@@ -6,6 +6,8 @@
#ifndef _ARCH_H
#define _ARCH_H
+#include <bfd.h>
+#include <dis-asm.h>
#include <stdbool.h>
#include <linux/list.h>
#include <objtool/objtool.h>
@@ -98,7 +100,10 @@ int arch_rewrite_retpolines(struct objtool_file *file);
bool arch_pc_relative_reloc(struct reloc *reloc);
+
unsigned int arch_reloc_size(struct reloc *reloc);
unsigned long arch_jump_table_sym_offset(struct reloc *reloc, struct reloc *table);
+int arch_disas_info_init(struct disassemble_info *dinfo);
+
#endif /* _ARCH_H */
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 00fb745e7233..5290ac1ebbc1 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -125,4 +125,9 @@ struct instruction *next_insn_same_sec(struct objtool_file *file, struct instruc
insn && insn->sec == _sec; \
insn = next_insn_same_sec(file, insn))
+#define sym_for_each_insn(file, sym, insn) \
+ for (insn = find_insn(file, sym->sec, sym->offset); \
+ insn && insn->offset < sym->offset + sym->len; \
+ insn = next_insn_same_sec(file, insn))
+
#endif /* _CHECK_H */
diff --git a/tools/objtool/include/objtool/objtool.h b/tools/objtool/include/objtool/objtool.h
index f5ab71f07f5c..0b404cfd81c0 100644
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -48,8 +48,12 @@ int orc_dump(const char *objname);
int orc_create(struct objtool_file *file);
struct disas_context;
+struct disassemble_info;
struct disas_context *disas_context_create(struct objtool_file *file);
void disas_context_destroy(struct disas_context *dctx);
void disas_warned_funcs(struct disas_context *dctx);
+int disas_info_init(struct disassemble_info *dinfo,
+ int arch, int mach32, int mach64,
+ const char *options);
#endif /* _OBJTOOL_H */
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump
2025-06-06 15:34 ` [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump Alexandre Chartre
@ 2025-06-10 21:22 ` Josh Poimboeuf
2025-06-11 12:23 ` Peter Zijlstra
1 sibling, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-10 21:22 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:30PM +0200, Alexandre Chartre wrote:
> + dctx->disassembler = disassembler(dinfo->arch,
> + dinfo->endian == BFD_ENDIAN_BIG,
> + dinfo->mach, NULL);
These lines should be aligned like:
dctx->disassembler = disassembler(dinfo->arch,
dinfo->endian == BFD_ENDIAN_BIG,
dinfo->mach, NULL);
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump
2025-06-06 15:34 ` [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump Alexandre Chartre
2025-06-10 21:22 ` Josh Poimboeuf
@ 2025-06-11 12:23 ` Peter Zijlstra
2025-06-11 13:35 ` Alexandre Chartre
1 sibling, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2025-06-11 12:23 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, jpoimboe
On Fri, Jun 06, 2025 at 05:34:30PM +0200, Alexandre Chartre wrote:
> objtool executes the objdump command to disassemble code. Use libopcodes
> instead to have more control about the disassembly scope and output.
>
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
> tools/objtool/Makefile | 2 +-
> tools/objtool/arch/loongarch/decode.c | 6 +
> tools/objtool/arch/powerpc/decode.c | 6 +
> tools/objtool/arch/x86/decode.c | 7 +
> tools/objtool/check.c | 4 +-
> tools/objtool/disas.c | 186 +++++++++++++++---------
> tools/objtool/include/objtool/arch.h | 5 +
> tools/objtool/include/objtool/check.h | 5 +
> tools/objtool/include/objtool/objtool.h | 4 +
> 9 files changed, 154 insertions(+), 71 deletions(-)
>
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index 8c20361dd100..00350fc7c662 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -34,7 +34,7 @@ INCLUDES := -I$(srctree)/tools/include \
> # is passed here to match a legacy behavior.
> WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> OBJTOOL_CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> -OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> +OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
Would it be possible to make this optional? Such that when people do not
have libopcodes installed they can still build the kernel.
Or is libopcodes a mandatory part of any binutils installation?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump
2025-06-11 12:23 ` Peter Zijlstra
@ 2025-06-11 13:35 ` Alexandre Chartre
2025-06-11 19:25 ` Josh Poimboeuf
0 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-11 13:35 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: alexandre.chartre, linux-kernel, mingo, jpoimboe
On 6/11/25 14:23, Peter Zijlstra wrote:
> On Fri, Jun 06, 2025 at 05:34:30PM +0200, Alexandre Chartre wrote:
>> objtool executes the objdump command to disassemble code. Use libopcodes
>> instead to have more control about the disassembly scope and output.
>>
>> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
>> ---
>> tools/objtool/Makefile | 2 +-
>> tools/objtool/arch/loongarch/decode.c | 6 +
>> tools/objtool/arch/powerpc/decode.c | 6 +
>> tools/objtool/arch/x86/decode.c | 7 +
>> tools/objtool/check.c | 4 +-
>> tools/objtool/disas.c | 186 +++++++++++++++---------
>> tools/objtool/include/objtool/arch.h | 5 +
>> tools/objtool/include/objtool/check.h | 5 +
>> tools/objtool/include/objtool/objtool.h | 4 +
>> 9 files changed, 154 insertions(+), 71 deletions(-)
>>
>> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
>> index 8c20361dd100..00350fc7c662 100644
>> --- a/tools/objtool/Makefile
>> +++ b/tools/objtool/Makefile
>> @@ -34,7 +34,7 @@ INCLUDES := -I$(srctree)/tools/include \
>> # is passed here to match a legacy behavior.
>> WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
>> OBJTOOL_CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
>> -OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
>> +OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
>
> Would it be possible to make this optional? Such that when people do not
> have libopcodes installed they can still build the kernel.
>
> Or is libopcodes a mandatory part of any binutils installation?
I guess that libopcodes is mandatory because it is used by as (and also objdump).
But I can check if it is effectively present anyway.
alex.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump
2025-06-11 13:35 ` Alexandre Chartre
@ 2025-06-11 19:25 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-11 19:25 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: Peter Zijlstra, linux-kernel, mingo
On Wed, Jun 11, 2025 at 03:35:38PM +0200, Alexandre Chartre wrote:
>
> On 6/11/25 14:23, Peter Zijlstra wrote:
> > On Fri, Jun 06, 2025 at 05:34:30PM +0200, Alexandre Chartre wrote:
> > > objtool executes the objdump command to disassemble code. Use libopcodes
> > > instead to have more control about the disassembly scope and output.
> > >
> > > Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> > > ---
> > > tools/objtool/Makefile | 2 +-
> > > tools/objtool/arch/loongarch/decode.c | 6 +
> > > tools/objtool/arch/powerpc/decode.c | 6 +
> > > tools/objtool/arch/x86/decode.c | 7 +
> > > tools/objtool/check.c | 4 +-
> > > tools/objtool/disas.c | 186 +++++++++++++++---------
> > > tools/objtool/include/objtool/arch.h | 5 +
> > > tools/objtool/include/objtool/check.h | 5 +
> > > tools/objtool/include/objtool/objtool.h | 4 +
> > > 9 files changed, 154 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > > index 8c20361dd100..00350fc7c662 100644
> > > --- a/tools/objtool/Makefile
> > > +++ b/tools/objtool/Makefile
> > > @@ -34,7 +34,7 @@ INCLUDES := -I$(srctree)/tools/include \
> > > # is passed here to match a legacy behavior.
> > > WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> > > OBJTOOL_CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > > -OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> > > +OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
> >
> > Would it be possible to make this optional? Such that when people do not
> > have libopcodes installed they can still build the kernel.
> >
> > Or is libopcodes a mandatory part of any binutils installation?
>
> I guess that libopcodes is mandatory because it is used by as (and also objdump).
> But I can check if it is effectively present anyway.
libopcodes should always be present, but compiling/linking against it
shouldn't be possible unless a binutils devel pkg is installed.
So I think Peter's basically asking for what I asked for: make all this
optional depending on whether the binutils devel pkg is installed, and
print a helpful error message if somebody tries to use without.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 04/13] objtool: Print symbol during disassembly
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (2 preceding siblings ...)
2025-06-06 15:34 ` [RFC 03/13] objtool: Disassemble code with libopcodes instead of running objdump Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-10 21:55 ` Josh Poimboeuf
2025-06-06 15:34 ` [RFC 05/13] objtool: Store instruction disassembly result Alexandre Chartre
` (10 subsequent siblings)
14 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
Print symbols referenced during disassembly instead of just printing
raw addresses. Also handle address relocation.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 9 ---
tools/objtool/disas.c | 101 ++++++++++++++++++++++++++
tools/objtool/include/objtool/check.h | 9 +++
3 files changed, 110 insertions(+), 9 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9cfac23185b8..ee613f03e57d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -131,15 +131,6 @@ static struct instruction *prev_insn_same_sym(struct objtool_file *file,
for (insn = next_insn_same_sec(file, insn); insn; \
insn = next_insn_same_sec(file, insn))
-static inline struct symbol *insn_call_dest(struct instruction *insn)
-{
- if (insn->type == INSN_JUMP_DYNAMIC ||
- insn->type == INSN_CALL_DYNAMIC)
- return NULL;
-
- return insn->_call_dest;
-}
-
static inline struct reloc *insn_jump_table(struct instruction *insn)
{
if (insn->type == INSN_JUMP_DYNAMIC ||
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index f2eb1050ce11..83fe2c018c4b 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -13,10 +13,108 @@
struct disas_context {
struct objtool_file *file;
+ struct instruction *insn;
disassembler_ftype disassembler;
struct disassemble_info info;
};
+#define DINFO_FPRINTF(dinfo, ...) \
+ ((*(dinfo)->fprintf_func)((dinfo)->stream, __VA_ARGS__))
+
+static void disas_print_address(bfd_vma addr, struct disassemble_info *dinfo)
+{
+ struct disas_context *dctx = dinfo->application_data;
+ struct instruction *insn = dctx->insn;
+ struct objtool_file *file = dctx->file;
+ struct symbol *call_dest, *sym;
+ struct instruction *jump_dest;
+ struct section *sec;
+ struct reloc *reloc;
+ bool is_reloc;
+ s64 offset;
+
+ /*
+ * If the instruction is a call/jump and it references a
+ * destination then this is likely the address we are looking
+ * up. So check it first.
+ */
+ jump_dest = insn->jump_dest;
+ if (jump_dest && jump_dest->offset == addr) {
+ DINFO_FPRINTF(dinfo, "%lx <%s+0x%lx>", addr,
+ jump_dest->sym->name,
+ jump_dest->offset - jump_dest->sym->offset);
+ return;
+ }
+
+ /*
+ * Assume the address is a relocation if it points to the next
+ * instruction.
+ */
+ is_reloc = (addr == insn->offset + insn->len);
+
+ /*
+ * The call destination offset can be the address we are looking
+ * up, or 0 if there is a relocation.
+ */
+ call_dest = insn_call_dest(insn);
+ if (call_dest) {
+ if (call_dest->offset == addr) {
+ DINFO_FPRINTF(dinfo, "%lx <%s>", addr, call_dest->name);
+ return;
+ }
+ if (call_dest->offset == 0 && is_reloc) {
+ DINFO_FPRINTF(dinfo, "%s", call_dest->name);
+ return;
+ }
+ }
+
+ if (!is_reloc) {
+ DINFO_FPRINTF(dinfo, "0x%lx", addr);
+ return;
+ }
+
+ /*
+ * If this is a relocation, check if we have relocation information
+ * for this instruction.
+ */
+ reloc = find_reloc_by_dest_range(file->elf, insn->sec,
+ insn->offset, insn->len);
+ if (!reloc) {
+ DINFO_FPRINTF(dinfo, "0x%lx", addr);
+ return;
+ }
+
+ if (reloc_type(reloc) == R_X86_64_PC32 ||
+ reloc_type(reloc) == R_X86_64_PLT32)
+ offset = arch_dest_reloc_offset(reloc_addend(reloc));
+ else
+ offset = reloc_addend(reloc);
+
+ /*
+ * If the relocation symbol is a section name (for example ".bss")
+ * then we try to further resolve the name.
+ */
+ sec = find_section_by_name(file->elf, reloc->sym->name);
+ if (sec) {
+ sym = find_symbol_containing(sec, offset);
+ if (sym) {
+ if (sym->offset == offset)
+ DINFO_FPRINTF(dinfo, "%s+0x%lx = %s",
+ reloc->sym->name, offset, sym->name);
+ else
+ DINFO_FPRINTF(dinfo, "%s+0x%lx = %s+0x%lx",
+ reloc->sym->name, offset,
+ sym->name, offset - sym->offset);
+ return;
+ }
+ }
+
+ if (offset)
+ DINFO_FPRINTF(dinfo, "%s+0x%lx", reloc->sym->name, offset);
+ else
+ DINFO_FPRINTF(dinfo, "%s", reloc->sym->name);
+}
+
/*
* Initialize disassemble info arch, mach (32 or 64-bit) and options.
*/
@@ -65,6 +163,7 @@ struct disas_context *disas_context_create(struct objtool_file *file)
fprintf_styled);
dinfo->read_memory_func = buffer_read_memory;
+ dinfo->print_address_func = disas_print_address;
dinfo->application_data = dctx;
/*
@@ -117,6 +216,8 @@ static size_t disas_insn(struct disas_context *dctx,
disassembler_ftype disasm = dctx->disassembler;
struct disassemble_info *dinfo = &dctx->info;
+ dctx->insn = insn;
+
/*
* Set the disassembler buffer to read data from the section
* containing the instruction to disassemble.
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 5290ac1ebbc1..4adbcd760c6f 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -115,6 +115,15 @@ static inline bool is_jump(struct instruction *insn)
return is_static_jump(insn) || is_dynamic_jump(insn);
}
+static inline struct symbol *insn_call_dest(struct instruction *insn)
+{
+ if (insn->type == INSN_JUMP_DYNAMIC ||
+ insn->type == INSN_CALL_DYNAMIC)
+ return NULL;
+
+ return insn->_call_dest;
+}
+
struct instruction *find_insn(struct objtool_file *file,
struct section *sec, unsigned long offset);
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 04/13] objtool: Print symbol during disassembly
2025-06-06 15:34 ` [RFC 04/13] objtool: Print symbol during disassembly Alexandre Chartre
@ 2025-06-10 21:55 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-10 21:55 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:31PM +0200, Alexandre Chartre wrote:
> +static void disas_print_address(bfd_vma addr, struct disassemble_info *dinfo)
> +{
> + struct disas_context *dctx = dinfo->application_data;
> + struct instruction *insn = dctx->insn;
> + struct objtool_file *file = dctx->file;
> + struct symbol *call_dest, *sym;
> + struct instruction *jump_dest;
> + struct section *sec;
> + struct reloc *reloc;
> + bool is_reloc;
> + s64 offset;
> +
> + /*
> + * If the instruction is a call/jump and it references a
> + * destination then this is likely the address we are looking
> + * up. So check it first.
> + */
> + jump_dest = insn->jump_dest;
> + if (jump_dest && jump_dest->offset == addr) {
> + DINFO_FPRINTF(dinfo, "%lx <%s+0x%lx>", addr,
> + jump_dest->sym->name,
> + jump_dest->offset - jump_dest->sym->offset);
> + return;
> + }
IIRC, there may be a few cases where an instruction's 'sym' field can be
NULL, might want to check for !jump_dest->sym here.
> + /*
> + * If this is a relocation, check if we have relocation information
> + * for this instruction.
> + */
> + reloc = find_reloc_by_dest_range(file->elf, insn->sec,
> + insn->offset, insn->len);
> + if (!reloc) {
> + DINFO_FPRINTF(dinfo, "0x%lx", addr);
> + return;
> + }
> +
> + if (reloc_type(reloc) == R_X86_64_PC32 ||
> + reloc_type(reloc) == R_X86_64_PLT32)
Can use arch_pc_relative_reloc() here.
> + offset = arch_dest_reloc_offset(reloc_addend(reloc));
> + else
> + offset = reloc_addend(reloc);
> +
> + /*
> + * If the relocation symbol is a section name (for example ".bss")
> + * then we try to further resolve the name.
> + */
This can be checked with reloc->sym->type == STT_SECTION.
> + sec = find_section_by_name(file->elf, reloc->sym->name);
> + if (sec) {
> + sym = find_symbol_containing(sec, offset);
> + if (sym) {
> + if (sym->offset == offset)
> + DINFO_FPRINTF(dinfo, "%s+0x%lx = %s",
> + reloc->sym->name, offset, sym->name);
> + else
> + DINFO_FPRINTF(dinfo, "%s+0x%lx = %s+0x%lx",
> + reloc->sym->name, offset,
> + sym->name, offset - sym->offset);
> + return;
> + }
> + }
> +
> + if (offset)
> + DINFO_FPRINTF(dinfo, "%s+0x%lx", reloc->sym->name, offset);
> + else
> + DINFO_FPRINTF(dinfo, "%s", reloc->sym->name);
We have offstr() which does similar things. You might be able to get
away with replacing the above hunk with something like:
DINFO_FPRINTF(dinfo, "%s", offstr(reloc->sym->sec, sym->offset + offset));
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 05/13] objtool: Store instruction disassembly result
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (3 preceding siblings ...)
2025-06-06 15:34 ` [RFC 04/13] objtool: Print symbol during disassembly Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-10 22:40 ` Josh Poimboeuf
2025-06-06 15:34 ` [RFC 06/13] objtool: Disassemble instruction on warning or backtrace Alexandre Chartre
` (9 subsequent siblings)
14 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
When disassembling an instruction store the result instead of directly
printing it.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/disas.c | 161 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 155 insertions(+), 6 deletions(-)
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index 83fe2c018c4b..f86b9b04ef97 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -11,9 +11,16 @@
#include <linux/string.h>
#include <tools/dis-asm-compat.h>
+struct dbuffer {
+ char *addr;
+ size_t size;
+ size_t used;
+};
+
struct disas_context {
struct objtool_file *file;
struct instruction *insn;
+ struct dbuffer result;
disassembler_ftype disassembler;
struct disassemble_info info;
};
@@ -21,6 +28,129 @@ struct disas_context {
#define DINFO_FPRINTF(dinfo, ...) \
((*(dinfo)->fprintf_func)((dinfo)->stream, __VA_ARGS__))
+
+static int dbuffer_init(struct dbuffer *dbuf, size_t size)
+{
+ dbuf->used = 0;
+ dbuf->size = size;
+
+ if (!size) {
+ dbuf->addr = NULL;
+ return 0;
+ }
+
+ dbuf->addr = malloc(size);
+ if (!dbuf->addr)
+ return -1;
+
+ return 0;
+}
+
+static void dbuffer_fini(struct dbuffer *dbuf)
+{
+ free(dbuf->addr);
+ dbuf->size = 0;
+ dbuf->used = 0;
+}
+
+static void dbuffer_reset(struct dbuffer *dbuf)
+{
+ dbuf->used = 0;
+}
+
+static char *dbuffer_data(struct dbuffer *dbuf)
+{
+ return dbuf->addr;
+}
+
+static int dbuffer_expand(struct dbuffer *dbuf, size_t space)
+{
+ size_t size;
+ char *addr;
+
+ size = dbuf->size + space;
+ addr = realloc(dbuf->addr, size);
+ if (!addr)
+ return -1;
+
+ dbuf->addr = addr;
+ dbuf->size = size;
+
+ return 0;
+}
+
+static int dbuffer_vappendf_noexpand(struct dbuffer *dbuf, const char *fmt, va_list ap)
+{
+ int free, len;
+
+ free = dbuf->size - dbuf->used;
+
+ len = vsnprintf(dbuf->addr + dbuf->used, free, fmt, ap);
+
+ if (len < 0)
+ return -1;
+
+ if (len < free) {
+ dbuf->used += len;
+ return 0;
+ }
+
+ return (len - free) + 1;
+}
+
+static int dbuffer_vappendf(struct dbuffer *dbuf, const char *fmt, va_list ap)
+{
+ int space_needed, err;
+
+ space_needed = dbuffer_vappendf_noexpand(dbuf, fmt, ap);
+ if (space_needed <= 0)
+ return space_needed;
+
+ /*
+ * The buffer is not large enough to store all data. Expand
+ * the buffer and retry. The buffer is expanded with enough
+ * space to store all data.
+ */
+ err = dbuffer_expand(dbuf, space_needed * 2);
+ if (err) {
+ WARN("failed to expand buffer\n");
+ return -1;
+ }
+
+ return dbuffer_vappendf_noexpand(dbuf, fmt, ap);
+}
+
+static int disas_fprintf(void *stream, const char *fmt, ...)
+{
+ va_list arg;
+ int len;
+
+ va_start(arg, fmt);
+ len = dbuffer_vappendf(stream, fmt, arg);
+ va_end(arg);
+
+ return len == 0 ? 0 : -1;
+}
+
+/*
+ * For init_disassemble_info_compat().
+ */
+static int disas_fprintf_styled(void *stream,
+ enum disassembler_style style,
+ const char *fmt, ...)
+{
+ va_list arg;
+ int len;
+
+ (void)style;
+
+ va_start(arg, fmt);
+ len = dbuffer_vappendf(stream, fmt, arg);
+ va_end(arg);
+
+ return len == 0 ? 0 : -1;
+}
+
static void disas_print_address(bfd_vma addr, struct disassemble_info *dinfo)
{
struct disas_context *dctx = dinfo->application_data;
@@ -147,6 +277,7 @@ struct disas_context *disas_context_create(struct objtool_file *file)
{
struct disas_context *dctx;
struct disassemble_info *dinfo;
+ struct dbuffer *dbuf;
int err;
dctx = malloc(sizeof(*dctx));
@@ -157,10 +288,16 @@ struct disas_context *disas_context_create(struct objtool_file *file)
dctx->file = file;
dinfo = &dctx->info;
+ dbuf = &dctx->result;
+
+ err = dbuffer_init(dbuf, 1024);
+ if (err) {
+ WARN("failed to initialize buffer\n");
+ return NULL;
+ }
- init_disassemble_info_compat(dinfo, stdout,
- (fprintf_ftype)fprintf,
- fprintf_styled);
+ init_disassemble_info_compat(dinfo, dbuf,
+ disas_fprintf, disas_fprintf_styled);
dinfo->read_memory_func = buffer_read_memory;
dinfo->print_address_func = disas_print_address;
@@ -204,9 +341,18 @@ struct disas_context *disas_context_create(struct objtool_file *file)
void disas_context_destroy(struct disas_context *dctx)
{
+ if (!dctx)
+ return;
+
+ dbuffer_fini(&dctx->result);
free(dctx);
}
+static char *disas_result(struct disas_context *dctx)
+{
+ return dbuffer_data(&dctx->result);
+}
+
/*
* Disassemble a single instruction. Return the size of the instruction.
*/
@@ -216,6 +362,7 @@ static size_t disas_insn(struct disas_context *dctx,
disassembler_ftype disasm = dctx->disassembler;
struct disassemble_info *dinfo = &dctx->info;
+ dbuffer_reset(&dctx->result);
dctx->insn = insn;
/*
@@ -241,10 +388,12 @@ static void disas_func(struct disas_context *dctx, struct symbol *func)
sym_for_each_insn(dctx->file, func, insn) {
addr = insn->offset;
- printf(" %6lx: %s+0x%-6lx ",
- addr, func->name, addr - func->offset);
size = disas_insn(dctx, insn);
- printf("\n");
+
+ printf(" %6lx: %s+0x%-6lx %s\n",
+ addr, func->name, addr - func->offset,
+ disas_result(dctx));
+
if (size != insn->len)
WARN("inconsistent insn size (%ld and %d)\n", size, insn->len);
}
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 05/13] objtool: Store instruction disassembly result
2025-06-06 15:34 ` [RFC 05/13] objtool: Store instruction disassembly result Alexandre Chartre
@ 2025-06-10 22:40 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-10 22:40 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:32PM +0200, Alexandre Chartre wrote:
> +static int dbuffer_init(struct dbuffer *dbuf, size_t size)
> +{
> + dbuf->used = 0;
> + dbuf->size = size;
> +
> + if (!size) {
> + dbuf->addr = NULL;
> + return 0;
> + }
> +
> + dbuf->addr = malloc(size);
> + if (!dbuf->addr)
> + return -1;
> +
> + return 0;
> +}
> +
> +static void dbuffer_fini(struct dbuffer *dbuf)
> +{
> + free(dbuf->addr);
> + dbuf->size = 0;
> + dbuf->used = 0;
> +}
> +
> +static void dbuffer_reset(struct dbuffer *dbuf)
> +{
> + dbuf->used = 0;
> +}
> +
> +static char *dbuffer_data(struct dbuffer *dbuf)
> +{
> + return dbuf->addr;
> +}
> +
> +static int dbuffer_expand(struct dbuffer *dbuf, size_t space)
> +{
> + size_t size;
> + char *addr;
> +
> + size = dbuf->size + space;
> + addr = realloc(dbuf->addr, size);
> + if (!addr)
> + return -1;
> +
> + dbuf->addr = addr;
> + dbuf->size = size;
> +
> + return 0;
> +}
> +
> +static int dbuffer_vappendf_noexpand(struct dbuffer *dbuf, const char *fmt, va_list ap)
> +{
> + int free, len;
> +
> + free = dbuf->size - dbuf->used;
> +
> + len = vsnprintf(dbuf->addr + dbuf->used, free, fmt, ap);
> +
> + if (len < 0)
> + return -1;
> +
> + if (len < free) {
> + dbuf->used += len;
> + return 0;
> + }
> +
> + return (len - free) + 1;
> +}
> +
> +static int dbuffer_vappendf(struct dbuffer *dbuf, const char *fmt, va_list ap)
> +{
> + int space_needed, err;
> +
> + space_needed = dbuffer_vappendf_noexpand(dbuf, fmt, ap);
> + if (space_needed <= 0)
> + return space_needed;
> +
> + /*
> + * The buffer is not large enough to store all data. Expand
> + * the buffer and retry. The buffer is expanded with enough
> + * space to store all data.
> + */
> + err = dbuffer_expand(dbuf, space_needed * 2);
> + if (err) {
> + WARN("failed to expand buffer\n");
> + return -1;
> + }
> +
> + return dbuffer_vappendf_noexpand(dbuf, fmt, ap);
> +}
I don't quite get the need for all this dbuffer stuff.
The buffer only needs to contain the output for a single instruction,
right? Is there any reason not to just make it a 1k char array which
gets appended via strncat()? If it exceeds that, it could just print a
warning and truncate the string.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 06/13] objtool: Disassemble instruction on warning or backtrace
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (4 preceding siblings ...)
2025-06-06 15:34 ` [RFC 05/13] objtool: Store instruction disassembly result Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-06 15:34 ` [RFC 07/13] objtool: Extract code to validate instruction from the validate branch loop Alexandre Chartre
` (8 subsequent siblings)
14 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
When an instruction warning (WARN_INSN) or backtrace (BT_INSN) is issued,
disassemble the instruction to provide more context.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 30 +++++++++++++++++++++++--
tools/objtool/disas.c | 5 ++---
tools/objtool/include/objtool/check.h | 12 ++++++++++
tools/objtool/include/objtool/objtool.h | 9 --------
tools/objtool/include/objtool/warn.h | 14 ++++++++----
5 files changed, 52 insertions(+), 18 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index ee613f03e57d..2c73c8d3515d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4580,11 +4580,34 @@ static void free_insns(struct objtool_file *file)
free(chunk->addr);
}
+static struct disas_context *objtool_disas_ctx;
+
+const char *objtool_disas_insn(struct instruction *insn)
+{
+ struct disas_context *dctx = objtool_disas_ctx;
+
+ if (!dctx)
+ return "";
+
+ disas_insn(dctx, insn);
+ return disas_result(dctx);
+}
+
int check(struct objtool_file *file)
{
- struct disas_context *disas_ctx;
+ struct disas_context *disas_ctx = NULL;
int ret = 0, warnings = 0;
+ /*
+ * If the verbose or backtrace option is used then we need a
+ * disassembly context to disassemble instruction or function
+ * on warning or backtrace.
+ */
+ if (opts.verbose || opts.backtrace) {
+ disas_ctx = disas_context_create(file);
+ objtool_disas_ctx = disas_ctx;
+ }
+
arch_initial_func_cfi_state(&initial_func_cfi);
init_cfi_state(&init_cfi);
init_cfi_state(&func_cfi);
@@ -4710,9 +4733,12 @@ int check(struct objtool_file *file)
if (opts.werror && warnings)
WARN("%d warning(s) upgraded to errors", warnings);
print_args();
- disas_ctx = disas_context_create(file);
disas_warned_funcs(disas_ctx);
+ }
+
+ if (disas_ctx) {
disas_context_destroy(disas_ctx);
+ objtool_disas_ctx = NULL;
}
free_insns(file);
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index f86b9b04ef97..1e198d5f9205 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -348,7 +348,7 @@ void disas_context_destroy(struct disas_context *dctx)
free(dctx);
}
-static char *disas_result(struct disas_context *dctx)
+char *disas_result(struct disas_context *dctx)
{
return dbuffer_data(&dctx->result);
}
@@ -356,8 +356,7 @@ static char *disas_result(struct disas_context *dctx)
/*
* Disassemble a single instruction. Return the size of the instruction.
*/
-static size_t disas_insn(struct disas_context *dctx,
- struct instruction *insn)
+size_t disas_insn(struct disas_context *dctx, struct instruction *insn)
{
disassembler_ftype disasm = dctx->disassembler;
struct disassemble_info *dinfo = &dctx->info;
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 4adbcd760c6f..92bfe6b209ad 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -139,4 +139,16 @@ struct instruction *next_insn_same_sec(struct objtool_file *file, struct instruc
insn && insn->offset < sym->offset + sym->len; \
insn = next_insn_same_sec(file, insn))
+struct disas_context;
+struct disassemble_info;
+struct disas_context *disas_context_create(struct objtool_file *file);
+void disas_context_destroy(struct disas_context *dctx);
+void disas_warned_funcs(struct disas_context *dctx);
+int disas_info_init(struct disassemble_info *dinfo,
+ int arch, int mach32, int mach64,
+ const char *options);
+size_t disas_insn(struct disas_context *dctx, struct instruction *insn);
+char *disas_result(struct disas_context *dctx);
+const char *objtool_disas_insn(struct instruction *insn);
+
#endif /* _CHECK_H */
diff --git a/tools/objtool/include/objtool/objtool.h b/tools/objtool/include/objtool/objtool.h
index 0b404cfd81c0..c0dc86a78ff6 100644
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -47,13 +47,4 @@ int check(struct objtool_file *file);
int orc_dump(const char *objname);
int orc_create(struct objtool_file *file);
-struct disas_context;
-struct disassemble_info;
-struct disas_context *disas_context_create(struct objtool_file *file);
-void disas_context_destroy(struct disas_context *dctx);
-void disas_warned_funcs(struct disas_context *dctx);
-int disas_info_init(struct disassemble_info *dinfo,
- int arch, int mach32, int mach64,
- const char *options);
-
#endif /* _OBJTOOL_H */
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index cb8fe846d9dd..32a8dd299c87 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -77,9 +77,11 @@ static inline char *offstr(struct section *sec, unsigned long offset)
#define WARN_INSN(insn, format, ...) \
({ \
struct instruction *_insn = (insn); \
- if (!_insn->sym || !_insn->sym->warned) \
+ if (!_insn->sym || !_insn->sym->warned) { \
WARN_FUNC(_insn->sec, _insn->offset, format, \
##__VA_ARGS__); \
+ BT_INSN(_insn, ""); \
+ } \
if (_insn->sym) \
_insn->sym->warned = 1; \
})
@@ -87,9 +89,13 @@ static inline char *offstr(struct section *sec, unsigned long offset)
#define BT_INSN(insn, format, ...) \
({ \
if (opts.verbose || opts.backtrace) { \
- struct instruction *_insn = (insn); \
- char *_str = offstr(_insn->sec, _insn->offset); \
- WARN(" %s: " format, _str, ##__VA_ARGS__); \
+ struct instruction *__insn = (insn); \
+ char *_str = offstr(__insn->sec, __insn->offset); \
+ const char *_istr = objtool_disas_insn(__insn); \
+ int len; \
+ len = snprintf(NULL, 0, " %s: " format, _str, ##__VA_ARGS__); \
+ len = (len < 50) ? 50 - len : 0; \
+ WARN(" %s: " format " %*s%s", _str, ##__VA_ARGS__, len, "", _istr); \
free(_str); \
} \
})
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* [RFC 07/13] objtool: Extract code to validate instruction from the validate branch loop
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (5 preceding siblings ...)
2025-06-06 15:34 ` [RFC 06/13] objtool: Disassemble instruction on warning or backtrace Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-10 23:31 ` Josh Poimboeuf
2025-06-06 15:34 ` [RFC 08/13] objtool: Record symbol name max length Alexandre Chartre
` (7 subsequent siblings)
14 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
The code to validate a branch loops through all instructions of the
branch and validate each instruction. Move the code to validate an
instruction to a separated function.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 375 +++++++++++++++++++++++-------------------
1 file changed, 208 insertions(+), 167 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2c73c8d3515d..36ec08b8d654 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3527,6 +3527,11 @@ static bool skip_alt_group(struct instruction *insn)
return alt_insn->type == INSN_CLAC || alt_insn->type == INSN_STAC;
}
+static int validate_insn(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state *statep,
+ struct instruction *prev_insn, struct instruction *next_insn,
+ bool *validate_nextp);
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -3536,10 +3541,9 @@ static bool skip_alt_group(struct instruction *insn)
static int validate_branch(struct objtool_file *file, struct symbol *func,
struct instruction *insn, struct insn_state state)
{
- struct alternative *alt;
struct instruction *next_insn, *prev_insn = NULL;
struct section *sec;
- u8 visited;
+ bool validate_next;
int ret;
if (func && func->ignore)
@@ -3566,232 +3570,269 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 1;
}
- visited = VISITED_BRANCH << state.uaccess;
- if (insn->visited & VISITED_BRANCH_MASK) {
- if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
- return 1;
+ ret = validate_insn(file, func, insn, &state,
+ prev_insn, next_insn,
+ &validate_next);
+ if (!validate_next)
+ break;
- if (insn->visited & visited)
+ if (!next_insn) {
+ if (state.cfi.cfa.base == CFI_UNDEFINED)
return 0;
- } else {
- nr_insns_visited++;
+ if (file->ignore_unreachables)
+ return 0;
+
+ WARN("%s%sunexpected end of section %s",
+ func ? func->name : "", func ? "(): " : "",
+ sec->name);
+ return 1;
}
- if (state.noinstr)
- state.instr += insn->instr;
+ prev_insn = insn;
+ insn = next_insn;
+ }
- if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
+ return ret;
+}
- i = insn;
- save_insn = NULL;
+static int validate_insn(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state *statep,
+ struct instruction *prev_insn, struct instruction *next_insn,
+ bool *validate_nextp)
+{
+ struct alternative *alt;
+ u8 visited;
+ int ret;
- sym_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
+ /*
+ * Indicate that, by default, the calling function should not
+ * validate the next instruction and validation should be
+ * stopped. That way this function can stop validation by just
+ * returning at any point before reaching the end of the function.
+ *
+ * If the end of this function is reached then that means that the
+ * validation should continue and the caller should validate the
+ * next instruction, so *validate_nextp will be set to true at
+ * that point.
+ */
+ *validate_nextp = false;
- if (!save_insn) {
- WARN_INSN(insn, "no corresponding CFI save for CFI restore");
- return 1;
- }
+ visited = VISITED_BRANCH << statep->uaccess;
+ if (insn->visited & VISITED_BRANCH_MASK) {
+ if (!insn->hint && !insn_cfi_match(insn, &statep->cfi))
+ return 1;
- if (!save_insn->visited) {
- /*
- * If the restore hint insn is at the
- * beginning of a basic block and was
- * branched to from elsewhere, and the
- * save insn hasn't been visited yet,
- * defer following this branch for now.
- * It will be seen later via the
- * straight-line path.
- */
- if (!prev_insn)
- return 0;
+ if (insn->visited & visited)
+ return 0;
+ } else {
+ nr_insns_visited++;
+ }
- WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
- return 1;
- }
+ if (statep->noinstr)
+ statep->instr += insn->instr;
- insn->cfi = save_insn->cfi;
- nr_cfi_reused++;
- }
+ if (insn->hint) {
+ if (insn->restore) {
+ struct instruction *save_insn, *i;
- state.cfi = *insn->cfi;
- } else {
- /* XXX track if we actually changed state.cfi */
+ i = insn;
+ save_insn = NULL;
- if (prev_insn && !cficmp(prev_insn->cfi, &state.cfi)) {
- insn->cfi = prev_insn->cfi;
- nr_cfi_reused++;
- } else {
- insn->cfi = cfi_hash_find_or_add(&state.cfi);
+ sym_for_each_insn_continue_reverse(file, func, i) {
+ if (i->save) {
+ save_insn = i;
+ break;
+ }
}
- }
- insn->visited |= visited;
+ if (!save_insn) {
+ WARN_INSN(insn, "no corresponding CFI save for CFI restore");
+ return 1;
+ }
- if (propagate_alt_cfi(file, insn))
- return 1;
+ if (!save_insn->visited) {
+ /*
+ * If the restore hint insn is at the
+ * beginning of a basic block and was
+ * branched to from elsewhere, and the
+ * save insn hasn't been visited yet,
+ * defer following this branch for now.
+ * It will be seen later via the
+ * straight-line path.
+ */
+ if (!prev_insn)
+ return 0;
- if (insn->alts) {
- for (alt = insn->alts; alt; alt = alt->next) {
- ret = validate_branch(file, func, alt->insn, state);
- if (ret) {
- BT_INSN(insn, "(alt)");
- return ret;
- }
+ WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
+ return 1;
}
+
+ insn->cfi = save_insn->cfi;
+ nr_cfi_reused++;
}
- if (skip_alt_group(insn))
- return 0;
+ statep->cfi = *insn->cfi;
+ } else {
+ /* XXX track if we actually changed statep->cfi */
- if (handle_insn_ops(insn, next_insn, &state))
- return 1;
+ if (prev_insn && !cficmp(prev_insn->cfi, &statep->cfi)) {
+ insn->cfi = prev_insn->cfi;
+ nr_cfi_reused++;
+ } else {
+ insn->cfi = cfi_hash_find_or_add(&statep->cfi);
+ }
+ }
- switch (insn->type) {
+ insn->visited |= visited;
- case INSN_RETURN:
- return validate_return(func, insn, &state);
+ if (propagate_alt_cfi(file, insn))
+ return 1;
- case INSN_CALL:
- case INSN_CALL_DYNAMIC:
- ret = validate_call(file, insn, &state);
- if (ret)
+ if (insn->alts) {
+ for (alt = insn->alts; alt; alt = alt->next) {
+ ret = validate_branch(file, func, alt->insn, *statep);
+ if (ret) {
+ BT_INSN(insn, "(alt)");
return ret;
-
- if (opts.stackval && func && !is_special_call(insn) &&
- !has_valid_stack_frame(&state)) {
- WARN_INSN(insn, "call without frame pointer save/setup");
- return 1;
}
+ }
+ }
- break;
-
- case INSN_JUMP_CONDITIONAL:
- case INSN_JUMP_UNCONDITIONAL:
- if (is_sibling_call(insn)) {
- ret = validate_sibling_call(file, insn, &state);
- if (ret)
- return ret;
-
- } else if (insn->jump_dest) {
- ret = validate_branch(file, func,
- insn->jump_dest, state);
- if (ret) {
- BT_INSN(insn, "(branch)");
- return ret;
- }
- }
+ if (skip_alt_group(insn))
+ return 0;
- if (insn->type == INSN_JUMP_UNCONDITIONAL)
- return 0;
+ if (handle_insn_ops(insn, next_insn, statep))
+ return 1;
- break;
+ switch (insn->type) {
- case INSN_JUMP_DYNAMIC:
- case INSN_JUMP_DYNAMIC_CONDITIONAL:
- if (is_sibling_call(insn)) {
- ret = validate_sibling_call(file, insn, &state);
- if (ret)
- return ret;
- }
+ case INSN_RETURN:
+ return validate_return(func, insn, statep);
- if (insn->type == INSN_JUMP_DYNAMIC)
- return 0;
+ case INSN_CALL:
+ case INSN_CALL_DYNAMIC:
+ ret = validate_call(file, insn, statep);
+ if (ret)
+ return ret;
- break;
+ if (opts.stackval && func && !is_special_call(insn) &&
+ !has_valid_stack_frame(statep)) {
+ WARN_INSN(insn, "call without frame pointer save/setup");
+ return 1;
+ }
- case INSN_SYSCALL:
- if (func && (!next_insn || !next_insn->hint)) {
- WARN_INSN(insn, "unsupported instruction in callable function");
- return 1;
- }
+ break;
- break;
+ case INSN_JUMP_CONDITIONAL:
+ case INSN_JUMP_UNCONDITIONAL:
+ if (is_sibling_call(insn)) {
+ ret = validate_sibling_call(file, insn, statep);
+ if (ret)
+ return ret;
- case INSN_SYSRET:
- if (func && (!next_insn || !next_insn->hint)) {
- WARN_INSN(insn, "unsupported instruction in callable function");
- return 1;
+ } else if (insn->jump_dest) {
+ ret = validate_branch(file, func,
+ insn->jump_dest, *statep);
+ if (ret) {
+ BT_INSN(insn, "(branch)");
+ return ret;
}
+ }
+ if (insn->type == INSN_JUMP_UNCONDITIONAL)
return 0;
- case INSN_STAC:
- if (!opts.uaccess)
- break;
+ break;
- if (state.uaccess) {
- WARN_INSN(insn, "recursive UACCESS enable");
- return 1;
- }
+ case INSN_JUMP_DYNAMIC:
+ case INSN_JUMP_DYNAMIC_CONDITIONAL:
+ if (is_sibling_call(insn)) {
+ ret = validate_sibling_call(file, insn, statep);
+ if (ret)
+ return ret;
+ }
- state.uaccess = true;
- break;
+ if (insn->type == INSN_JUMP_DYNAMIC)
+ return 0;
- case INSN_CLAC:
- if (!opts.uaccess)
- break;
+ break;
- if (!state.uaccess && func) {
- WARN_INSN(insn, "redundant UACCESS disable");
- return 1;
- }
+ case INSN_SYSCALL:
+ if (func && (!next_insn || !next_insn->hint)) {
+ WARN_INSN(insn, "unsupported instruction in callable function");
+ return 1;
+ }
- if (func_uaccess_safe(func) && !state.uaccess_stack) {
- WARN_INSN(insn, "UACCESS-safe disables UACCESS");
- return 1;
- }
+ break;
- state.uaccess = false;
- break;
+ case INSN_SYSRET:
+ if (func && (!next_insn || !next_insn->hint)) {
+ WARN_INSN(insn, "unsupported instruction in callable function");
+ return 1;
+ }
- case INSN_STD:
- if (state.df) {
- WARN_INSN(insn, "recursive STD");
- return 1;
- }
+ return 0;
- state.df = true;
+ case INSN_STAC:
+ if (!opts.uaccess)
break;
- case INSN_CLD:
- if (!state.df && func) {
- WARN_INSN(insn, "redundant CLD");
- return 1;
- }
+ if (statep->uaccess) {
+ WARN_INSN(insn, "recursive UACCESS enable");
+ return 1;
+ }
- state.df = false;
- break;
+ statep->uaccess = true;
+ break;
- default:
+ case INSN_CLAC:
+ if (!opts.uaccess)
break;
+
+ if (!statep->uaccess && func) {
+ WARN_INSN(insn, "redundant UACCESS disable");
+ return 1;
}
- if (insn->dead_end)
- return 0;
+ if (func_uaccess_safe(func) && !statep->uaccess_stack) {
+ WARN_INSN(insn, "UACCESS-safe disables UACCESS");
+ return 1;
+ }
- if (!next_insn) {
- if (state.cfi.cfa.base == CFI_UNDEFINED)
- return 0;
- if (file->ignore_unreachables)
- return 0;
+ statep->uaccess = false;
+ break;
- WARN("%s%sunexpected end of section %s",
- func ? func->name : "", func ? "(): " : "",
- sec->name);
+ case INSN_STD:
+ if (statep->df) {
+ WARN_INSN(insn, "recursive STD");
return 1;
}
- prev_insn = insn;
- insn = next_insn;
+ statep->df = true;
+ break;
+
+ case INSN_CLD:
+ if (!statep->df && func) {
+ WARN_INSN(insn, "redundant CLD");
+ return 1;
+ }
+
+ statep->df = false;
+ break;
+
+ default:
+ break;
}
+ if (insn->dead_end)
+ return 0;
+
+ /*
+ * Indicate that the caller should validate the next
+ * instruction and continue the validation.
+ */
+ *validate_nextp = true;
+
return 0;
}
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 07/13] objtool: Extract code to validate instruction from the validate branch loop
2025-06-06 15:34 ` [RFC 07/13] objtool: Extract code to validate instruction from the validate branch loop Alexandre Chartre
@ 2025-06-10 23:31 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-10 23:31 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:34PM +0200, Alexandre Chartre wrote:
> The code to validate a branch loops through all instructions of the
> branch and validate each instruction. Move the code to validate an
> instruction to a separated function.
>
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
> tools/objtool/check.c | 375 +++++++++++++++++++++++-------------------
> 1 file changed, 208 insertions(+), 167 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 2c73c8d3515d..36ec08b8d654 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -3527,6 +3527,11 @@ static bool skip_alt_group(struct instruction *insn)
> return alt_insn->type == INSN_CLAC || alt_insn->type == INSN_STAC;
> }
>
> +static int validate_insn(struct objtool_file *file, struct symbol *func,
> + struct instruction *insn, struct insn_state *statep,
> + struct instruction *prev_insn, struct instruction *next_insn,
> + bool *validate_nextp);
Since validate_insn() is always called by validate_branch(), put the
validate_insn() implementation here above validate_branch().
> /*
> * Follow the branch starting at the given instruction, and recursively follow
> * any other branches (jumps). Meanwhile, track the frame pointer state at
> @@ -3536,10 +3541,9 @@ static bool skip_alt_group(struct instruction *insn)
> static int validate_branch(struct objtool_file *file, struct symbol *func,
> struct instruction *insn, struct insn_state state)
> {
> - struct alternative *alt;
> struct instruction *next_insn, *prev_insn = NULL;
> struct section *sec;
> - u8 visited;
> + bool validate_next;
I think it's clearer if we reverse the polarity and call it "dead_end".
> @@ -3566,232 +3570,269 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> return 1;
> }
>
> - visited = VISITED_BRANCH << state.uaccess;
> - if (insn->visited & VISITED_BRANCH_MASK) {
> - if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
> - return 1;
> + ret = validate_insn(file, func, insn, &state,
> + prev_insn, next_insn,
> + &validate_next);
This can fit in two lines:
ret = validate_insn(file, func, insn, &state, prev_insn,
next_insn, &dead_end);
> +static int validate_insn(struct objtool_file *file, struct symbol *func,
> + struct instruction *insn, struct insn_state *statep,
> + struct instruction *prev_insn, struct instruction *next_insn,
> + bool *validate_nextp)
> +{
> + struct alternative *alt;
> + u8 visited;
> + int ret;
>
> - sym_for_each_insn_continue_reverse(file, func, i) {
> - if (i->save) {
> - save_insn = i;
> - break;
> - }
> - }
> + /*
> + * Indicate that, by default, the calling function should not
> + * validate the next instruction and validation should be
> + * stopped. That way this function can stop validation by just
> + * returning at any point before reaching the end of the function.
> + *
> + * If the end of this function is reached then that means that the
> + * validation should continue and the caller should validate the
> + * next instruction, so *validate_nextp will be set to true at
> + * that point.
> + */
> + *validate_nextp = false;
This can be much more succinct:
/*
* Any returns before the end of this function are effectively dead
* ends, i.e. validate_branch() has reached the end of the branch.
*/
*dead_end = true;
> + default:
> + break;
> }
>
> + if (insn->dead_end)
> + return 0;
> +
> + /*
> + * Indicate that the caller should validate the next
> + * instruction and continue the validation.
> + */
> + *validate_nextp = true;
> +
> return 0;
No comment needed here. Also this can be condensed:
*dead_end = insn->dead_end;
return 0;
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 08/13] objtool: Record symbol name max length
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (6 preceding siblings ...)
2025-06-06 15:34 ` [RFC 07/13] objtool: Extract code to validate instruction from the validate branch loop Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-06 15:34 ` [RFC 09/13] objtool: Add option to trace function validation Alexandre Chartre
` (6 subsequent siblings)
14 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
Keep track of the maximum length of symbol names. This will help
formatting the code flow between different functions.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 36ec08b8d654..300428cb5c2c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -34,6 +34,8 @@ static struct cfi_state init_cfi;
static struct cfi_state func_cfi;
static struct cfi_state force_undefined_cfi;
+static size_t sym_name_max_len;
+
struct instruction *find_insn(struct objtool_file *file,
struct section *sec, unsigned long offset)
{
@@ -2458,6 +2460,7 @@ static bool is_profiling_func(const char *name)
static int classify_symbols(struct objtool_file *file)
{
struct symbol *func;
+ size_t len;
for_each_sym(file, func) {
if (func->type == STT_NOTYPE && strstarts(func->name, ".L"))
@@ -2484,6 +2487,10 @@ static int classify_symbols(struct objtool_file *file)
if (is_profiling_func(func->name))
func->profiling_func = true;
+
+ len = strlen(func->name);
+ if (len > sym_name_max_len)
+ sym_name_max_len = len;
}
return 0;
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* [RFC 09/13] objtool: Add option to trace function validation
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (7 preceding siblings ...)
2025-06-06 15:34 ` [RFC 08/13] objtool: Record symbol name max length Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-11 0:48 ` Josh Poimboeuf
2025-06-06 15:34 ` [RFC 10/13] objtool: Trace instruction state changes during " Alexandre Chartre
` (5 subsequent siblings)
14 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
Add an option to trace and have information during the validation
of specified functions. Functions are specified with the --trace
option which can be a single function name (e.g. --trace foo to
trace the function with the name "foo"), or a shell wildcard
pattern (e.g. --trace foo* to trace all functions with a name
starting with "foo").
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/builtin-check.c | 1 +
tools/objtool/check.c | 179 ++++++++++++++++++++++--
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/check.h | 2 +
tools/objtool/include/objtool/warn.h | 3 +-
5 files changed, 171 insertions(+), 15 deletions(-)
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 80239843e9f0..ac7baf95f5bf 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -99,6 +99,7 @@ static const struct option check_options[] = {
OPT_STRING('o', "output", &opts.output, "file", "output file name"),
OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
+ OPT_STRING(0, "trace", &opts.trace, "func", "trace function validation"),
OPT_BOOLEAN('v', "verbose", &opts.verbose, "verbose warnings"),
OPT_BOOLEAN(0, "Werror", &opts.werror, "return error on warnings"),
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 300428cb5c2c..40eaac4b5756 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3,6 +3,7 @@
* Copyright (C) 2015-2017 Josh Poimboeuf <jpoimboe@redhat.com>
*/
+#include <fnmatch.h>
#include <string.h>
#include <stdlib.h>
#include <inttypes.h>
@@ -36,6 +37,80 @@ static struct cfi_state force_undefined_cfi;
static size_t sym_name_max_len;
+static bool vtrace;
+static int vtrace_depth;
+
+/*
+ * Validation traces are sent to stderr so that they are output
+ * on the same flow as warnings.
+ */
+#define VTRACE_PRINTF(fmt, ...) fprintf(stderr, fmt, ##__VA_ARGS__)
+
+#define VTRACE_INSN(insn, fmt, ...) \
+ do { \
+ if (vtrace) { \
+ vtrace_insn(insn, fmt, ##__VA_ARGS__); \
+ VTRACE_PRINTF("\n"); \
+ } \
+ } while (0)
+
+#define VTRACE_INSN_OFFSET_SPACE 10
+#define VTRACE_INSN_SPACE 60
+
+/*
+ * Print an instruction address (offset and function), the instruction itself
+ * and an optional message.
+ */
+static void vtrace_insn(struct instruction *insn, const char *format, ...)
+{
+ char fake_nop_insn[32];
+ const char *addr_str, *insn_str;
+ bool fake_nop;
+ va_list args;
+ int i, len;
+
+ len = sym_name_max_len + VTRACE_INSN_OFFSET_SPACE;
+
+ /*
+ * Alternative can insert a fake nop, sometimes with no
+ * associated section so nothing to disassemble.
+ */
+ fake_nop = (!insn->sec && insn->type == INSN_NOP);
+ if (fake_nop) {
+ addr_str = "";
+ snprintf(fake_nop_insn, 32, "<fake nop> (%d bytes)", insn->len);
+ insn_str = fake_nop_insn;
+ } else {
+ addr_str = offstr(insn->sec, insn->offset);
+ insn_str = objtool_disas_insn(insn);
+ }
+
+ /* print the instruction address */
+ VTRACE_PRINTF("%6lx: %-*s ", insn->offset, len, addr_str);
+
+ /* print vertical bars to show the validation flow */
+ for (i = 1; i < vtrace_depth; i++)
+ VTRACE_PRINTF("| ");
+
+ /* print the instruction */
+ len = vtrace_depth * 2 < VTRACE_INSN_SPACE ?
+ VTRACE_INSN_SPACE - vtrace_depth * 2 : 1;
+ VTRACE_PRINTF("%-*s", len, insn_str);
+
+ /* print message if any */
+ if (format) {
+ VTRACE_PRINTF(" - ");
+ va_start(args, format);
+ vfprintf(stderr, format, args);
+ va_end(args);
+ }
+
+ insn->vtrace++;
+
+ if (!fake_nop)
+ free((char *)addr_str);
+}
+
struct instruction *find_insn(struct objtool_file *file,
struct section *sec, unsigned long offset)
{
@@ -3511,8 +3586,10 @@ static bool skip_alt_group(struct instruction *insn)
struct instruction *alt_insn = insn->alts ? insn->alts->insn : NULL;
/* ANNOTATE_IGNORE_ALTERNATIVE */
- if (insn->alt_group && insn->alt_group->ignore)
+ if (insn->alt_group && insn->alt_group->ignore) {
+ VTRACE_INSN(insn, "alt group ignored");
return true;
+ }
/*
* For NOP patched with CLAC/STAC, only follow the latter to avoid
@@ -3539,14 +3616,17 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
struct instruction *prev_insn, struct instruction *next_insn,
bool *validate_nextp);
+static int validate_branch(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state state);
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
* each instruction and validate all the rules described in
* tools/objtool/Documentation/objtool.txt.
*/
-static int validate_branch(struct objtool_file *file, struct symbol *func,
- struct instruction *insn, struct insn_state state)
+static int do_validate_branch(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state state)
{
struct instruction *next_insn, *prev_insn = NULL;
struct section *sec;
@@ -3558,7 +3638,10 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
sec = insn->sec;
- while (1) {
+ do {
+
+ insn->vtrace = 0;
+
next_insn = next_insn_to_validate(file, insn);
if (func && insn_func(insn) && func != insn_func(insn)->pfunc) {
@@ -3570,6 +3653,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (file->ignore_unreachables)
return 0;
+ VTRACE_INSN(insn, "falls through to next function");
+
WARN("%s() falls through to next function %s()",
func->name, insn_func(insn)->name);
func->warned = 1;
@@ -3580,10 +3665,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
ret = validate_insn(file, func, insn, &state,
prev_insn, next_insn,
&validate_next);
- if (!validate_next)
- break;
- if (!next_insn) {
+ if (validate_next && !next_insn) {
if (state.cfi.cfa.base == CFI_UNDEFINED)
return 0;
if (file->ignore_unreachables)
@@ -3595,9 +3678,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 1;
}
+ if (!insn->vtrace) {
+ if (ret)
+ VTRACE_INSN(insn, "validated (%d)", ret);
+ else
+ VTRACE_INSN(insn, NULL);
+ }
+
prev_insn = insn;
insn = next_insn;
- }
+
+ } while (validate_next);
return ret;
}
@@ -3629,8 +3720,10 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
if (!insn->hint && !insn_cfi_match(insn, &statep->cfi))
return 1;
- if (insn->visited & visited)
+ if (insn->visited & visited) {
+ VTRACE_INSN(insn, "already visited");
return 0;
+ }
} else {
nr_insns_visited++;
}
@@ -3667,8 +3760,10 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
* It will be seen later via the
* straight-line path.
*/
- if (!prev_insn)
+ if (!prev_insn) {
+ VTRACE_INSN(insn, "defer restore");
return 0;
+ }
WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
return 1;
@@ -3696,13 +3791,24 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
return 1;
if (insn->alts) {
+ int i, count;
+
+ count = 0;
+ for (alt = insn->alts; alt; alt = alt->next)
+ count++;
+
+ i = 1;
for (alt = insn->alts; alt; alt = alt->next) {
+ VTRACE_INSN(insn, "alternative %d/%d", i, count);
ret = validate_branch(file, func, alt->insn, *statep);
if (ret) {
BT_INSN(insn, "(alt)");
return ret;
}
+ i++;
}
+
+ VTRACE_INSN(insn, "alternative orig");
}
if (skip_alt_group(insn))
@@ -3714,10 +3820,12 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
switch (insn->type) {
case INSN_RETURN:
+ VTRACE_INSN(insn, "return");
return validate_return(func, insn, statep);
case INSN_CALL:
case INSN_CALL_DYNAMIC:
+ VTRACE_INSN(insn, "call");
ret = validate_call(file, insn, statep);
if (ret)
return ret;
@@ -3733,13 +3841,21 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
case INSN_JUMP_CONDITIONAL:
case INSN_JUMP_UNCONDITIONAL:
if (is_sibling_call(insn)) {
+ VTRACE_INSN(insn, "sibling call");
ret = validate_sibling_call(file, insn, statep);
if (ret)
return ret;
} else if (insn->jump_dest) {
- ret = validate_branch(file, func,
- insn->jump_dest, *statep);
+ if (insn->type == INSN_JUMP_UNCONDITIONAL) {
+ VTRACE_INSN(insn, "unconditional jump");
+ ret = do_validate_branch(file, func,
+ insn->jump_dest, *statep);
+ } else {
+ VTRACE_INSN(insn, "jump taken");
+ ret = validate_branch(file, func,
+ insn->jump_dest, *statep);
+ }
if (ret) {
BT_INSN(insn, "(branch)");
return ret;
@@ -3749,10 +3865,12 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
if (insn->type == INSN_JUMP_UNCONDITIONAL)
return 0;
+ VTRACE_INSN(insn, "jump not taken");
break;
case INSN_JUMP_DYNAMIC:
case INSN_JUMP_DYNAMIC_CONDITIONAL:
+ VTRACE_INSN(insn, "dynamic jump");
if (is_sibling_call(insn)) {
ret = validate_sibling_call(file, insn, statep);
if (ret)
@@ -3765,6 +3883,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
break;
case INSN_SYSCALL:
+ VTRACE_INSN(insn, "syscall");
if (func && (!next_insn || !next_insn->hint)) {
WARN_INSN(insn, "unsupported instruction in callable function");
return 1;
@@ -3773,6 +3892,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
break;
case INSN_SYSRET:
+ VTRACE_INSN(insn, "sysret");
if (func && (!next_insn || !next_insn->hint)) {
WARN_INSN(insn, "unsupported instruction in callable function");
return 1;
@@ -3781,6 +3901,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
return 0;
case INSN_STAC:
+ VTRACE_INSN(insn, "stac");
if (!opts.uaccess)
break;
@@ -3793,6 +3914,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
break;
case INSN_CLAC:
+ VTRACE_INSN(insn, "clac");
if (!opts.uaccess)
break;
@@ -3810,6 +3932,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
break;
case INSN_STD:
+ VTRACE_INSN(insn, "std");
if (statep->df) {
WARN_INSN(insn, "recursive STD");
return 1;
@@ -3819,6 +3942,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
break;
case INSN_CLD:
+ VTRACE_INSN(insn, "cld");
if (!statep->df && func) {
WARN_INSN(insn, "redundant CLD");
return 1;
@@ -3831,8 +3955,10 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
break;
}
- if (insn->dead_end)
+ if (insn->dead_end) {
+ VTRACE_INSN(insn, "dead end");
return 0;
+ }
/*
* Indicate that the caller should validate the next
@@ -3843,6 +3969,18 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
return 0;
}
+static int validate_branch(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state state)
+{
+ int ret;
+
+ vtrace_depth++;
+ ret = do_validate_branch(file, func, insn, state);
+ vtrace_depth--;
+
+ return ret;
+}
+
static int validate_unwind_hint(struct objtool_file *file,
struct instruction *insn,
struct insn_state *state)
@@ -4253,9 +4391,22 @@ static int validate_symbol(struct objtool_file *file, struct section *sec,
if (opts.uaccess)
state->uaccess = sym->uaccess_safe;
+ if (opts.trace && fnmatch(opts.trace, sym->name, 0) == 0) {
+ vtrace = true;
+ vtrace_depth = 0;
+ VTRACE_PRINTF("%s: validation begin\n", sym->name);
+ }
+
ret = validate_branch(file, insn_func(insn), insn, *state);
if (ret)
BT_INSN(insn, "<=== (sym)");
+
+ if (vtrace) {
+ VTRACE_PRINTF("%s: validation %s\n\n",
+ sym->name, ret ? "failed" : "end");
+ vtrace = false;
+ }
+
return ret;
}
@@ -4651,7 +4802,7 @@ int check(struct objtool_file *file)
* disassembly context to disassemble instruction or function
* on warning or backtrace.
*/
- if (opts.verbose || opts.backtrace) {
+ if (opts.verbose || opts.backtrace || opts.trace) {
disas_ctx = disas_context_create(file);
objtool_disas_ctx = disas_ctx;
}
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 6b08666fa69d..b3c84b6fdc5f 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -37,6 +37,7 @@ struct opts {
const char *output;
bool sec_address;
bool stats;
+ const char *trace;
bool verbose;
bool werror;
};
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 92bfe6b209ad..1b9b399578ea 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -81,6 +81,8 @@ struct instruction {
struct symbol *sym;
struct stack_op *stack_ops;
struct cfi_state *cfi;
+
+ u32 vtrace;
};
static inline struct symbol *insn_func(struct instruction *insn)
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index 32a8dd299c87..0bb94f2d3ae4 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -96,7 +96,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
len = snprintf(NULL, 0, " %s: " format, _str, ##__VA_ARGS__); \
len = (len < 50) ? 50 - len : 0; \
WARN(" %s: " format " %*s%s", _str, ##__VA_ARGS__, len, "", _istr); \
- free(_str); \
+ free(_str); \
+ __insn->vtrace++; \
} \
})
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 09/13] objtool: Add option to trace function validation
2025-06-06 15:34 ` [RFC 09/13] objtool: Add option to trace function validation Alexandre Chartre
@ 2025-06-11 0:48 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-11 0:48 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:36PM +0200, Alexandre Chartre wrote:
> +++ b/tools/objtool/builtin-check.c
> @@ -99,6 +99,7 @@ static const struct option check_options[] = {
> OPT_STRING('o', "output", &opts.output, "file", "output file name"),
> OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
> OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
> + OPT_STRING(0, "trace", &opts.trace, "func", "trace function validation"),
Here, "trace" is vertically misaligned with the other long options.
> @@ -36,6 +37,80 @@ static struct cfi_state force_undefined_cfi;
>
> static size_t sym_name_max_len;
>
> +static bool vtrace;
> +static int vtrace_depth;
I'm not sure what the "v" means here and elsewhere, can we remove it to
improve readability? The option is called "trace" after all.
> +/*
> + * Validation traces are sent to stderr so that they are output
> + * on the same flow as warnings.
> + */
> +#define VTRACE_PRINTF(fmt, ...) fprintf(stderr, fmt, ##__VA_ARGS__)
I'm not sure this comment is really needed. At least the reason for the
stderr seems obvious to me. Besides that, it might make sense for
tracing to use stderr anyway, regardless of whether warnings existed.
Also I think the "_PRINTF" is self-evident, can we call it TRACE() or
so? That would be more analagous to the existing WARN()/ERROR().
> + /* print message if any */
> + if (format) {
> + VTRACE_PRINTF(" - ");
> + va_start(args, format);
> + vfprintf(stderr, format, args);
This breaks the macro abstraction, do we need a wrapper for
"vfprintf(stderr...)" as well?
VTRACE() maybe? (assuming the base macro gets renamed to TRACE)
> @@ -3580,10 +3665,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> ret = validate_insn(file, func, insn, &state,
> prev_insn, next_insn,
> &validate_next);
> - if (!validate_next)
> - break;
>
> - if (!next_insn) {
> + if (validate_next && !next_insn) {
> if (state.cfi.cfa.base == CFI_UNDEFINED)
> return 0;
> if (file->ignore_unreachables)
> @@ -3595,9 +3678,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> return 1;
> }
>
> + if (!insn->vtrace) {
> + if (ret)
> + VTRACE_INSN(insn, "validated (%d)", ret);
I'm not sure what "validated" communicates here, should this instead
indicate there was an warning?
> + else
> + VTRACE_INSN(insn, NULL);
> + }
Should these VTRACE_INSN()s be done before the !next_insn clause above
so we can see the last instruction before the branch validation stopped?
> @@ -3696,13 +3791,24 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
> return 1;
>
> if (insn->alts) {
> + int i, count;
> +
> + count = 0;
> + for (alt = insn->alts; alt; alt = alt->next)
> + count++;
"count" is rather vague, how about "num_alts" or so.
> @@ -3733,13 +3841,21 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
> case INSN_JUMP_CONDITIONAL:
> case INSN_JUMP_UNCONDITIONAL:
> if (is_sibling_call(insn)) {
> + VTRACE_INSN(insn, "sibling call");
> ret = validate_sibling_call(file, insn, statep);
> if (ret)
> return ret;
>
> } else if (insn->jump_dest) {
> - ret = validate_branch(file, func,
> - insn->jump_dest, *statep);
> + if (insn->type == INSN_JUMP_UNCONDITIONAL) {
> + VTRACE_INSN(insn, "unconditional jump");
> + ret = do_validate_branch(file, func,
> + insn->jump_dest, *statep);
> + } else {
> + VTRACE_INSN(insn, "jump taken");
> + ret = validate_branch(file, func,
> + insn->jump_dest, *statep);
> + }
This can be simplified:
if (insn->type == INSN_JUMP_UNCONDITIONAL)
VTRACE_INSN(insn, "unconditional jump");
else
VTRACE_INSN(insn, "jump taken");
ret = do_validate_branch(file, func, insn->jump_dest, *statep);
> if (ret) {
> BT_INSN(insn, "(branch)");
> return ret;
> @@ -3749,10 +3865,12 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
> if (insn->type == INSN_JUMP_UNCONDITIONAL)
> return 0;
>
> + VTRACE_INSN(insn, "jump not taken");
> break;
>
> case INSN_JUMP_DYNAMIC:
> case INSN_JUMP_DYNAMIC_CONDITIONAL:
> + VTRACE_INSN(insn, "dynamic jump");
Let's call it "indirect jump" (and yes, those enums are poorly named).
Shall we have a distinct trace for indirect calls (INSN_CALL_DYNAMIC) as
well?
> @@ -4253,9 +4391,22 @@ static int validate_symbol(struct objtool_file *file, struct section *sec,
> if (opts.uaccess)
> state->uaccess = sym->uaccess_safe;
>
> + if (opts.trace && fnmatch(opts.trace, sym->name, 0) == 0) {
Please use "!fnmatch(...)" instead of "fnmatch(...) == 0".
> +++ b/tools/objtool/include/objtool/check.h
> @@ -81,6 +81,8 @@ struct instruction {
> struct symbol *sym;
> struct stack_op *stack_ops;
> struct cfi_state *cfi;
> +
> + u32 vtrace;
> };
Can this just be a single bit instead of u32? This struct is one of the
biggest memory hogs, vmlinux.o can have a gazillion instructions. There
are actually some bits already reserved in this struct.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 10/13] objtool: Trace instruction state changes during function validation
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (8 preceding siblings ...)
2025-06-06 15:34 ` [RFC 09/13] objtool: Add option to trace function validation Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-11 1:23 ` Josh Poimboeuf
2025-06-06 15:34 ` [RFC 11/13] objtool: Improve register reporting " Alexandre Chartre
` (4 subsequent siblings)
14 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
During function validation, objtool maintains a per-instruction state,
in particular to track call frame information. When tracing validation,
print any instruction state changes.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 116 +++++++++++++++++++++++++-
tools/objtool/disas.c | 27 ++++++
tools/objtool/include/objtool/check.h | 1 +
3 files changed, 143 insertions(+), 1 deletion(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 40eaac4b5756..050d34930372 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -111,6 +111,111 @@ static void vtrace_insn(struct instruction *insn, const char *format, ...)
free((char *)addr_str);
}
+/*
+ * Macros to trace CFI state attributes changes.
+ */
+
+#define VTRACE_CFI_ATTR(attr, prev, next, fmt, ...) \
+ do { \
+ if ((prev)->attr != (next)->attr) \
+ VTRACE_PRINTF("%s=" fmt " ", #attr, __VA_ARGS__); \
+ } while (0)
+
+#define VTRACE_CFI_ATTR_BOOL(attr, prev, next) \
+ VTRACE_CFI_ATTR(attr, prev, next, \
+ "%s", (next)->attr ? "true" : "false")
+
+#define VTRACE_CFI_ATTR_NUM(attr, prev, next, fmt) \
+ VTRACE_CFI_ATTR(attr, prev, next, fmt, (next)->attr)
+
+/*
+ * Functions and macros to trace CFI registers changes.
+ */
+
+static void vtrace_cfi_register(const char *prefix, int reg, const char *fmt,
+ int base_prev, int offset_prev,
+ int base_next, int offset_next)
+{
+ const char *rname;
+
+ if (base_prev == base_next && offset_prev == offset_next)
+ return;
+
+ if (prefix)
+ VTRACE_PRINTF("%s:", prefix);
+
+ rname = register_name(reg);
+
+ if (base_next == CFI_UNDEFINED) {
+ VTRACE_PRINTF("%1$s=<undef> ", rname);
+ } else {
+ VTRACE_PRINTF(fmt, rname,
+ register_name(base_next), offset_next);
+ }
+}
+
+static void vtrace_cfi_reg_value(const char *prefix, int reg,
+ int base_prev, int offset_prev,
+ int base_next, int offset_next)
+{
+ vtrace_cfi_register(prefix, reg, "%1$s=%2$s%3$+d ",
+ base_prev, offset_prev, base_next, offset_next);
+}
+
+static void vtrace_cfi_reg_reference(const char *prefix, int reg,
+ int base_prev, int offset_prev,
+ int base_next, int offset_next)
+{
+ vtrace_cfi_register(prefix, reg, "%1$s=(%2$s%3$+d) ",
+ base_prev, offset_prev, base_next, offset_next);
+}
+
+#define VTRACE_CFI_REG_VAL(reg, prev, next) \
+ vtrace_cfi_reg_value(NULL, reg, prev.base, prev.offset, \
+ next.base, next.offset)
+
+#define VTRACE_CFI_REG_REF(reg, prev, next) \
+ vtrace_cfi_reg_reference(NULL, reg, prev.base, prev.offset, \
+ next.base, next.offset)
+
+static void vtrace_insn_state(struct instruction *insn,
+ struct insn_state *sprev,
+ struct insn_state *snext)
+{
+ struct cfi_state *cprev, *cnext;
+ int i;
+
+ if (memcmp(sprev, snext, sizeof(struct insn_state)) == 0)
+ return;
+
+ cprev = &sprev->cfi;
+ cnext = &snext->cfi;
+
+ vtrace_insn(insn, NULL);
+ VTRACE_PRINTF(" - state: ");
+
+ /* print registers changes */
+ VTRACE_CFI_REG_VAL(CFI_CFA, cprev->cfa, cnext->cfa);
+ for (i = 0; i < CFI_NUM_REGS; i++) {
+ VTRACE_CFI_REG_VAL(i, cprev->vals[i], cnext->vals[i]);
+ VTRACE_CFI_REG_REF(i, cprev->regs[i], cnext->regs[i]);
+ }
+
+ /* print attributes changes */
+ VTRACE_CFI_ATTR_NUM(stack_size, cprev, cnext, "%d");
+ VTRACE_CFI_ATTR_BOOL(drap, cprev, cnext);
+ if (cnext->drap) {
+ vtrace_cfi_reg_value("drap", cnext->drap_reg,
+ cprev->drap_reg, cprev->drap_offset,
+ cnext->drap_reg, cnext->drap_offset);
+ }
+ VTRACE_CFI_ATTR_BOOL(bp_scratch, cprev, cnext);
+ VTRACE_CFI_ATTR_NUM(instr, sprev, snext, "%d");
+ VTRACE_CFI_ATTR_NUM(uaccess_stack, sprev, snext, "%u");
+
+ VTRACE_PRINTF("\n");
+}
+
struct instruction *find_insn(struct objtool_file *file,
struct section *sec, unsigned long offset)
{
@@ -3698,6 +3803,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
struct instruction *prev_insn, struct instruction *next_insn,
bool *validate_nextp)
{
+ struct insn_state state_prev;
struct alternative *alt;
u8 visited;
int ret;
@@ -3814,7 +3920,15 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
if (skip_alt_group(insn))
return 0;
- if (handle_insn_ops(insn, next_insn, statep))
+ if (vtrace)
+ state_prev = *statep;
+
+ ret = handle_insn_ops(insn, next_insn, statep);
+
+ if (vtrace)
+ vtrace_insn_state(insn, &state_prev, statep);
+
+ if (ret)
return 1;
switch (insn->type) {
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index 1e198d5f9205..4326c608f925 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -29,6 +29,33 @@ struct disas_context {
((*(dinfo)->fprintf_func)((dinfo)->stream, __VA_ARGS__))
+#define REGISTER_NAME_MAXLEN 16
+
+/*
+ * Return the name of a register. Note that the same static buffer
+ * is returned if the name is dynamically generated.
+ */
+const char *register_name(unsigned int reg)
+{
+ static char rname_buffer[REGISTER_NAME_MAXLEN];
+
+ switch (reg) {
+ case CFI_UNDEFINED:
+ return "<undefined>";
+ case CFI_CFA:
+ return "cfa";
+ case CFI_SP_INDIRECT:
+ return "(sp)";
+ case CFI_BP_INDIRECT:
+ return "(bp)";
+ }
+
+ if (snprintf(rname_buffer, REGISTER_NAME_MAXLEN, "r%d", reg) == 1)
+ return NULL;
+
+ return (const char *)rname_buffer;
+}
+
static int dbuffer_init(struct dbuffer *dbuf, size_t size)
{
dbuf->used = 0;
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 1b9b399578ea..137d20963921 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -152,5 +152,6 @@ int disas_info_init(struct disassemble_info *dinfo,
size_t disas_insn(struct disas_context *dctx, struct instruction *insn);
char *disas_result(struct disas_context *dctx);
const char *objtool_disas_insn(struct instruction *insn);
+const char *register_name(unsigned int reg);
#endif /* _CHECK_H */
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 10/13] objtool: Trace instruction state changes during function validation
2025-06-06 15:34 ` [RFC 10/13] objtool: Trace instruction state changes during " Alexandre Chartre
@ 2025-06-11 1:23 ` Josh Poimboeuf
0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-11 1:23 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:37PM +0200, Alexandre Chartre wrote:
> +++ b/tools/objtool/check.c
> @@ -111,6 +111,111 @@ static void vtrace_insn(struct instruction *insn, const char *format, ...)
> free((char *)addr_str);
> }
>
> +/*
> + * Macros to trace CFI state attributes changes.
> + */
> +
> +#define VTRACE_CFI_ATTR(attr, prev, next, fmt, ...) \
> + do { \
> + if ((prev)->attr != (next)->attr) \
> + VTRACE_PRINTF("%s=" fmt " ", #attr, __VA_ARGS__); \
> + } while (0)
> +
> +#define VTRACE_CFI_ATTR_BOOL(attr, prev, next) \
> + VTRACE_CFI_ATTR(attr, prev, next, \
> + "%s", (next)->attr ? "true" : "false")
> +
> +#define VTRACE_CFI_ATTR_NUM(attr, prev, next, fmt) \
> + VTRACE_CFI_ATTR(attr, prev, next, fmt, (next)->attr)
> +
> +/*
> + * Functions and macros to trace CFI registers changes.
> + */
> +
> +static void vtrace_cfi_register(const char *prefix, int reg, const char *fmt,
> + int base_prev, int offset_prev,
> + int base_next, int offset_next)
> +{
In keeping with the namespace of the other vtrace_cfi_reg_*()
functions/macros, I'd suggest calling this vtrace_cfi_reg()
(or trace_cfi_reg() if we go with my previous suggestion to remove the "v"
everywhere)
> + const char *rname;
> +
> + if (base_prev == base_next && offset_prev == offset_next)
> + return;
> +
> + if (prefix)
> + VTRACE_PRINTF("%s:", prefix);
> +
> + rname = register_name(reg);
For similar reasons, "register_name()" -> "reg_name()".
> +
> + if (base_next == CFI_UNDEFINED) {
> + VTRACE_PRINTF("%1$s=<undef> ", rname);
> + } else {
> + VTRACE_PRINTF(fmt, rname,
> + register_name(base_next), offset_next);
> + }
> +}
> +
> +static void vtrace_cfi_reg_value(const char *prefix, int reg,
> + int base_prev, int offset_prev,
> + int base_next, int offset_next)
> +{
> + vtrace_cfi_register(prefix, reg, "%1$s=%2$s%3$+d ",
> + base_prev, offset_prev, base_next, offset_next);
> +}
> +
> +static void vtrace_cfi_reg_reference(const char *prefix, int reg,
> + int base_prev, int offset_prev,
> + int base_next, int offset_next)
> +{
> + vtrace_cfi_register(prefix, reg, "%1$s=(%2$s%3$+d) ",
> + base_prev, offset_prev, base_next, offset_next);
> +}
> +
> +#define VTRACE_CFI_REG_VAL(reg, prev, next) \
> + vtrace_cfi_reg_value(NULL, reg, prev.base, prev.offset, \
> + next.base, next.offset)
> +
> +#define VTRACE_CFI_REG_REF(reg, prev, next) \
> + vtrace_cfi_reg_reference(NULL, reg, prev.base, prev.offset, \
> + next.base, next.offset)
For similar reasons, "*_val()" and "*_ref()".
> +
> +static void vtrace_insn_state(struct instruction *insn,
> + struct insn_state *sprev,
> + struct insn_state *snext)
> +{
> + struct cfi_state *cprev, *cnext;
> + int i;
> +
> + if (memcmp(sprev, snext, sizeof(struct insn_state)) == 0)
> + return;
"!memcmp()"
> @@ -3698,6 +3803,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
> struct instruction *prev_insn, struct instruction *next_insn,
> bool *validate_nextp)
> {
> + struct insn_state state_prev;
Can we call it prev_state for consistency with prev_insn?
> struct alternative *alt;
> u8 visited;
> int ret;
> @@ -3814,7 +3920,15 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
> if (skip_alt_group(insn))
> return 0;
>
> - if (handle_insn_ops(insn, next_insn, statep))
> + if (vtrace)
> + state_prev = *statep;
> +
> + ret = handle_insn_ops(insn, next_insn, statep);
> +
> + if (vtrace)
> + vtrace_insn_state(insn, &state_prev, statep);
For consistency with the other tracing, can the trace statement be a
capitalized macro, with the "vtrace" check hidden?
state_prev = *statep;
ret = handle_insn_ops(insn, next_insn, statep);
TRACE_INSN_STATE(insn, &state_prev, statep);
> +++ b/tools/objtool/disas.c
> @@ -29,6 +29,33 @@ struct disas_context {
> ((*(dinfo)->fprintf_func)((dinfo)->stream, __VA_ARGS__))
>
>
> +#define REGISTER_NAME_MAXLEN 16
> +
> +/*
> + * Return the name of a register. Note that the same static buffer
> + * is returned if the name is dynamically generated.
> + */
> +const char *register_name(unsigned int reg)
> +{
> + static char rname_buffer[REGISTER_NAME_MAXLEN];
> +
> + switch (reg) {
> + case CFI_UNDEFINED:
> + return "<undefined>";
> + case CFI_CFA:
> + return "cfa";
> + case CFI_SP_INDIRECT:
> + return "(sp)";
> + case CFI_BP_INDIRECT:
> + return "(bp)";
> + }
> +
> + if (snprintf(rname_buffer, REGISTER_NAME_MAXLEN, "r%d", reg) == 1)
> + return NULL;
> +
> + return (const char *)rname_buffer;
> +}
> +
Would this not be a better fit as a static function in check.c? Its
only callers are there. And it references the CFI stuff.
Or we might also want to consider moving all the trace_*() functions to
their own file, trace.c or so.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 11/13] objtool: Improve register reporting during function validation
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (9 preceding siblings ...)
2025-06-06 15:34 ` [RFC 10/13] objtool: Trace instruction state changes during " Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-06 15:34 ` [RFC 12/13] objtool: Improve tracing of alternative instructions Alexandre Chartre
` (3 subsequent siblings)
14 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
When tracing function validation, instruction state changes can
report changes involving registers. These registers are reported
with the name "r<num>" (e.g. "r3"). Print the CPU specific register
name instead of a generic name (e.g. print "rbx" instead of "r3"
on x86).
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/arch/loongarch/decode.c | 11 +++++++++++
tools/objtool/arch/powerpc/decode.c | 12 ++++++++++++
tools/objtool/arch/x86/decode.c | 8 ++++++++
tools/objtool/disas.c | 7 +++++++
tools/objtool/include/objtool/arch.h | 2 ++
5 files changed, 40 insertions(+)
diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
index bf5ac6750512..fbad237e54fb 100644
--- a/tools/objtool/arch/loongarch/decode.c
+++ b/tools/objtool/arch/loongarch/decode.c
@@ -7,6 +7,17 @@
#include <linux/objtool_types.h>
#include <arch/elf.h>
+const char *arch_reg_name[CFI_NUM_REGS] = {
+ "zero", "ra", "tp", "sp",
+ "a0", "a1", "a2", "a3",
+ "a4", "a5", "a6", "a7",
+ "t0", "t1", "t2", "t3",
+ "t4", "t5", "t6", "t7",
+ "t8", "u0", "fp", "s0",
+ "s1", "s2", "s3", "s4",
+ "s5", "s6", "s7", "s8"
+};
+
int arch_ftrace_match(char *name)
{
return !strcmp(name, "_mcount");
diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
index c0fcab2d643c..df5bf6476b78 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -9,6 +9,18 @@
#include <objtool/builtin.h>
#include <objtool/endianness.h>
+const char *arch_reg_name[CFI_NUM_REGS] = {
+ "r0", "sp", "r2", "r3",
+ "r4", "r5", "r6", "r7",
+ "r8", "r9", "r10", "r11",
+ "r12", "r13", "r14", "r15",
+ "r16", "r17", "r18", "r19",
+ "r20", "r21", "r22", "r23",
+ "r24", "r25", "r26", "r27",
+ "r28", "r29", "r30", "r31",
+ "ra"
+};
+
int arch_ftrace_match(char *name)
{
return !strcmp(name, "_mcount");
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 6c13c67ed9b9..56e2df35c9ee 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -23,6 +23,14 @@
#include <objtool/builtin.h>
#include <arch/elf.h>
+const char *arch_reg_name[CFI_NUM_REGS] = {
+ "rax", "rcx", "rdx", "rbx",
+ "rsp", "rbp", "rsi", "rdi",
+ "r8", "r9", "r10", "r11",
+ "r12", "r13", "r14", "r15",
+ "ra"
+};
+
int arch_ftrace_match(char *name)
{
return !strcmp(name, "__fentry__");
diff --git a/tools/objtool/disas.c b/tools/objtool/disas.c
index 4326c608f925..8265ad7479a3 100644
--- a/tools/objtool/disas.c
+++ b/tools/objtool/disas.c
@@ -38,6 +38,7 @@ struct disas_context {
const char *register_name(unsigned int reg)
{
static char rname_buffer[REGISTER_NAME_MAXLEN];
+ const char *rname;
switch (reg) {
case CFI_UNDEFINED:
@@ -50,6 +51,12 @@ const char *register_name(unsigned int reg)
return "(bp)";
}
+ if (reg < CFI_NUM_REGS) {
+ rname = arch_reg_name[reg];
+ if (rname)
+ return rname;
+ }
+
if (snprintf(rname_buffer, REGISTER_NAME_MAXLEN, "r%d", reg) == 1)
return NULL;
diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
index aecf8fc29571..4736b08805d6 100644
--- a/tools/objtool/include/objtool/arch.h
+++ b/tools/objtool/include/objtool/arch.h
@@ -106,4 +106,6 @@ unsigned long arch_jump_table_sym_offset(struct reloc *reloc, struct reloc *tabl
int arch_disas_info_init(struct disassemble_info *dinfo);
+extern const char *arch_reg_name[CFI_NUM_REGS];
+
#endif /* _ARCH_H */
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* [RFC 12/13] objtool: Improve tracing of alternative instructions
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (10 preceding siblings ...)
2025-06-06 15:34 ` [RFC 11/13] objtool: Improve register reporting " Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-06 15:34 ` [RFC 13/13] objtool: Do not validate IBT for .return_sites and .call_sites Alexandre Chartre
` (2 subsequent siblings)
14 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
When tracing function validation, improve the reporting of
alternative instruction by more clearly showing the different
alternatives beginning and end.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 59 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 56 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 050d34930372..afcb6c67daa9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -54,9 +54,61 @@ static int vtrace_depth;
} \
} while (0)
+#define VTRACE_INFO(insn, fmt, ...) \
+ do { \
+ if (vtrace) \
+ vtrace_info(insn, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define VTRACE_ALT_FMT(fmt) "<alternative.%lx> alt " fmt
+
+#define VTRACE_ALT(insn, fmt, ...) \
+ VTRACE_INSN(insn, VTRACE_ALT_FMT(fmt), \
+ (insn)->offset, ##__VA_ARGS__)
+
+#define VTRACE_ALT_INFO(insn, fmt, ...) \
+ VTRACE_INFO(insn, VTRACE_ALT_FMT(fmt), \
+ (insn)->offset, ##__VA_ARGS__)
+
+#define VTRACE_ALT_INFO_NOADDR(insn, fmt, ...) \
+ VTRACE_INFO(NULL, VTRACE_ALT_FMT(fmt), \
+ (insn)->offset, ##__VA_ARGS__)
+
#define VTRACE_INSN_OFFSET_SPACE 10
#define VTRACE_INSN_SPACE 60
+/*
+ * Print a message in the instruction flow. If insn is not NULL then
+ * the instruction address is printed in addition of the message,
+ * otherwise only the message is printed. In all cases, the instruction
+ * itself is not printed.
+ */
+static void vtrace_info(struct instruction *insn, const char *format, ...)
+{
+ const char *addr_str;
+ va_list args;
+ int len;
+ int i;
+
+ len = sym_name_max_len + VTRACE_INSN_OFFSET_SPACE;
+ if (insn) {
+ addr_str = offstr(insn->sec, insn->offset);
+ VTRACE_PRINTF("%6lx: %-*s ", insn->offset, len, addr_str);
+ } else {
+ len += 11;
+ VTRACE_PRINTF("%-*s", len, "");
+ }
+
+ /* print vertical bars to show the validation flow */
+ for (i = 1; i < vtrace_depth; i++)
+ VTRACE_PRINTF("| ");
+
+ va_start(args, format);
+ vfprintf(stderr, format, args);
+ va_end(args);
+ VTRACE_PRINTF("\n");
+}
+
/*
* Print an instruction address (offset and function), the instruction itself
* and an optional message.
@@ -3692,7 +3744,7 @@ static bool skip_alt_group(struct instruction *insn)
/* ANNOTATE_IGNORE_ALTERNATIVE */
if (insn->alt_group && insn->alt_group->ignore) {
- VTRACE_INSN(insn, "alt group ignored");
+ VTRACE_ALT(insn, "alt group ignored");
return true;
}
@@ -3905,8 +3957,9 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
i = 1;
for (alt = insn->alts; alt; alt = alt->next) {
- VTRACE_INSN(insn, "alternative %d/%d", i, count);
+ VTRACE_ALT_INFO(insn, "%d/%d begin", i, count);
ret = validate_branch(file, func, alt->insn, *statep);
+ VTRACE_ALT_INFO_NOADDR(insn, "%d/%d end", i, count);
if (ret) {
BT_INSN(insn, "(alt)");
return ret;
@@ -3914,7 +3967,7 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
i++;
}
- VTRACE_INSN(insn, "alternative orig");
+ VTRACE_ALT_INFO(insn, "default");
}
if (skip_alt_group(insn))
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* [RFC 13/13] objtool: Do not validate IBT for .return_sites and .call_sites
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (11 preceding siblings ...)
2025-06-06 15:34 ` [RFC 12/13] objtool: Improve tracing of alternative instructions Alexandre Chartre
@ 2025-06-06 15:34 ` Alexandre Chartre
2025-06-06 15:58 ` [RFC 00/13] objtool: Function validation tracing Josh Poimboeuf
2025-06-09 18:31 ` Josh Poimboeuf
14 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 15:34 UTC (permalink / raw)
To: linux-kernel, mingo, jpoimboe, peterz; +Cc: alexandre.chartre
The .return_sites and .call_sites sections reference text addresses,
but not with the intent to indirect branch to them, so they don't
need to be validated for IBT.
This is useful when running objtool on object files which already
have .return_sites or .call_sites sections, for example to re-run
objtool after it has reported an error or a warning.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index afcb6c67daa9..6d81dabef64e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4844,6 +4844,8 @@ static int validate_ibt(struct objtool_file *file)
!strcmp(sec->name, ".llvm.call-graph-profile") ||
!strcmp(sec->name, ".llvm_bb_addr_map") ||
!strcmp(sec->name, "__tracepoints") ||
+ !strcmp(sec->name, ".return_sites") ||
+ !strcmp(sec->name, ".call_sites") ||
strstr(sec->name, "__patchable_function_entries"))
continue;
--
2.43.5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (12 preceding siblings ...)
2025-06-06 15:34 ` [RFC 13/13] objtool: Do not validate IBT for .return_sites and .call_sites Alexandre Chartre
@ 2025-06-06 15:58 ` Josh Poimboeuf
2025-06-06 19:29 ` Alexandre Chartre
2025-06-09 18:31 ` Josh Poimboeuf
14 siblings, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-06 15:58 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:27PM +0200, Alexandre Chartre wrote:
> Hi,
>
> This RFC provides two changes to objtool.
>
> - Disassemble code with libopcodes instead of running objdump
>
> objtool executes the objdump command to disassemble code. In particular,
> if objtool fails to validate a function then it will use objdump to
> disassemble the entire file which is not very helpful when processing
> a large file (like vmlinux.o).
>
> Using libopcodes provides more control about the disassembly scope and
> output, and it is possible to disassemble a single instruction or
> a single function. Now when objtool fails to validate a function it
> will disassemble that single function instead of disassembling the
> entire file.
Ah, nice to get rid of that awful objdump hack.
> - Add the --trace <function> option to trace function validation
>
> Figuring out why a function validation has failed can be difficult because
> objtool checks all code flows (including alternatives) and maintains
> instructions states (in particular call frame information).
>
> The trace option allows to follow the function validation done by objtool
> instruction per instruction, see what objtool is doing and get function
> validation information. An output example is shown below.
This is pretty freaking awesome!
I assume we could eventually build on this work to have an "objtool
disas" subcommand, which would basically be an improved "objdump -d"
which annotates alternatives and other runtime patching.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-06 15:58 ` [RFC 00/13] objtool: Function validation tracing Josh Poimboeuf
@ 2025-06-06 19:29 ` Alexandre Chartre
0 siblings, 0 replies; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-06 19:29 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: alexandre.chartre, linux-kernel, mingo, peterz
On 6/6/25 17:58, Josh Poimboeuf wrote:
> On Fri, Jun 06, 2025 at 05:34:27PM +0200, Alexandre Chartre wrote:
>> Hi,
>>
>> This RFC provides two changes to objtool.
>>
>> - Disassemble code with libopcodes instead of running objdump
>>
>> objtool executes the objdump command to disassemble code. In particular,
>> if objtool fails to validate a function then it will use objdump to
>> disassemble the entire file which is not very helpful when processing
>> a large file (like vmlinux.o).
>>
>> Using libopcodes provides more control about the disassembly scope and
>> output, and it is possible to disassemble a single instruction or
>> a single function. Now when objtool fails to validate a function it
>> will disassemble that single function instead of disassembling the
>> entire file.
>
> Ah, nice to get rid of that awful objdump hack.
>
>> - Add the --trace <function> option to trace function validation
>>
>> Figuring out why a function validation has failed can be difficult because
>> objtool checks all code flows (including alternatives) and maintains
>> instructions states (in particular call frame information).
>>
>> The trace option allows to follow the function validation done by objtool
>> instruction per instruction, see what objtool is doing and get function
>> validation information. An output example is shown below.
>
> This is pretty freaking awesome!
>
> I assume we could eventually build on this work to have an "objtool
> disas" subcommand, which would basically be an improved "objdump -d"
> which annotates alternatives and other runtime patching.
>
Yes, good idea. I will look at it.
alex.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-06 15:34 [RFC 00/13] objtool: Function validation tracing Alexandre Chartre
` (13 preceding siblings ...)
2025-06-06 15:58 ` [RFC 00/13] objtool: Function validation tracing Josh Poimboeuf
@ 2025-06-09 18:31 ` Josh Poimboeuf
2025-06-10 7:07 ` Alexandre Chartre
14 siblings, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-09 18:31 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Fri, Jun 06, 2025 at 05:34:27PM +0200, Alexandre Chartre wrote:
> Hi,
>
> This RFC provides two changes to objtool.
>
> - Disassemble code with libopcodes instead of running objdump
>
> objtool executes the objdump command to disassemble code. In particular,
> if objtool fails to validate a function then it will use objdump to
> disassemble the entire file which is not very helpful when processing
> a large file (like vmlinux.o).
>
> Using libopcodes provides more control about the disassembly scope and
> output, and it is possible to disassemble a single instruction or
> a single function. Now when objtool fails to validate a function it
> will disassemble that single function instead of disassembling the
> entire file.
>
> - Add the --trace <function> option to trace function validation
>
> Figuring out why a function validation has failed can be difficult because
> objtool checks all code flows (including alternatives) and maintains
> instructions states (in particular call frame information).
>
> The trace option allows to follow the function validation done by objtool
> instruction per instruction, see what objtool is doing and get function
> validation information. An output example is shown below.
What do I need for this to build? It wasn't compiling due to missing
bfd.h, so I installed binutils-devel (Fedora) and now I get:
In file included from disas.c:12:
/home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h:10:6: error: redeclaration of ‘enum disassembler_style’
10 | enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
| ^~~~~~~~~~~~~~~~~~
In file included from /home/jpoimboe/git/linux/tools/objtool/include/objtool/arch.h:10,
from disas.c:6:
/usr/include/dis-asm.h:53:6: note: originally defined here
53 | enum disassembler_style
| ^~~~~~~~~~~~~~~~~~
/home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h: In function ‘init_disassemble_info_compat’:
/home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h:50:9: error: too few arguments to function ‘init_disassemble_info’
50 | init_disassemble_info(info, stream,
| ^~~~~~~~~~~~~~~~~~~~~
/usr/include/dis-asm.h:480:13: note: declared here
480 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
| ^~~~~~~~~~~~~~~~~~~~~
make[4]: *** [/home/jpoimboe/git/linux/tools/build/Makefile.build:86: /home/jpoimboe/git/linux/tools/objtool/disas.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [Makefile:65: /home/jpoimboe/git/linux/tools/objtool/objtool-in.o] Error 2
make[2]: *** [Makefile:73: objtool] Error 2
make[1]: *** [/home/jpoimboe/git/linux/Makefile:1448: tools/objtool] Error 2
make: *** [Makefile:248: __sub-make] Error 2
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-09 18:31 ` Josh Poimboeuf
@ 2025-06-10 7:07 ` Alexandre Chartre
2025-06-10 13:00 ` Alexandre Chartre
0 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-10 7:07 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: alexandre.chartre, linux-kernel, mingo, peterz
On 6/9/25 20:31, Josh Poimboeuf wrote:
> On Fri, Jun 06, 2025 at 05:34:27PM +0200, Alexandre Chartre wrote:
>> Hi,
>>
>> This RFC provides two changes to objtool.
>>
>> - Disassemble code with libopcodes instead of running objdump
>>
>> objtool executes the objdump command to disassemble code. In particular,
>> if objtool fails to validate a function then it will use objdump to
>> disassemble the entire file which is not very helpful when processing
>> a large file (like vmlinux.o).
>>
>> Using libopcodes provides more control about the disassembly scope and
>> output, and it is possible to disassemble a single instruction or
>> a single function. Now when objtool fails to validate a function it
>> will disassemble that single function instead of disassembling the
>> entire file.
>>
>> - Add the --trace <function> option to trace function validation
>>
>> Figuring out why a function validation has failed can be difficult because
>> objtool checks all code flows (including alternatives) and maintains
>> instructions states (in particular call frame information).
>>
>> The trace option allows to follow the function validation done by objtool
>> instruction per instruction, see what objtool is doing and get function
>> validation information. An output example is shown below.
>
> What do I need for this to build? It wasn't compiling due to missing
> bfd.h, so I installed binutils-devel (Fedora) and now I get:
That's because of the more recent binutils versions, while I have been using an old
one (2.30) and some functions have changed. But tools/dis-asm-compat.h handles that
when the appropriate #define is set.
Below is a quick fix (tested with binutils 2.41), and I will work on a proper fix
(by using the tools/ features to check the disassembler version).
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 00350fc7c662..300ea727e454 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -36,6 +36,8 @@ WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -
OBJTOOL_CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
+OBJTOOL_CFLAGS += -DDISASM_INIT_STYLED
+
# Allow old libelf to be used:
elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(HOSTCC) $(OBJTOOL_CFLAGS) -x c -E - 2>/dev/null | grep elf_getshdr)
OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
alex.
> In file included from disas.c:12:
> /home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h:10:6: error: redeclaration of ‘enum disassembler_style’
> 10 | enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
> | ^~~~~~~~~~~~~~~~~~
> In file included from /home/jpoimboe/git/linux/tools/objtool/include/objtool/arch.h:10,
> from disas.c:6:
> /usr/include/dis-asm.h:53:6: note: originally defined here
> 53 | enum disassembler_style
> | ^~~~~~~~~~~~~~~~~~
> /home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h: In function ‘init_disassemble_info_compat’:
> /home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h:50:9: error: too few arguments to function ‘init_disassemble_info’
> 50 | init_disassemble_info(info, stream,
> | ^~~~~~~~~~~~~~~~~~~~~
> /usr/include/dis-asm.h:480:13: note: declared here
> 480 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
> | ^~~~~~~~~~~~~~~~~~~~~
> make[4]: *** [/home/jpoimboe/git/linux/tools/build/Makefile.build:86: /home/jpoimboe/git/linux/tools/objtool/disas.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
> make[3]: *** [Makefile:65: /home/jpoimboe/git/linux/tools/objtool/objtool-in.o] Error 2
> make[2]: *** [Makefile:73: objtool] Error 2
> make[1]: *** [/home/jpoimboe/git/linux/Makefile:1448: tools/objtool] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-10 7:07 ` Alexandre Chartre
@ 2025-06-10 13:00 ` Alexandre Chartre
2025-06-10 21:05 ` Josh Poimboeuf
0 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-10 13:00 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: alexandre.chartre, linux-kernel, mingo, peterz
On 6/10/25 09:07, Alexandre Chartre wrote:
>
>
> On 6/9/25 20:31, Josh Poimboeuf wrote:
>> On Fri, Jun 06, 2025 at 05:34:27PM +0200, Alexandre Chartre wrote:
>>> Hi,
>>>
>>> This RFC provides two changes to objtool.
>>>
>>> - Disassemble code with libopcodes instead of running objdump
>>>
>>> objtool executes the objdump command to disassemble code. In particular,
>>> if objtool fails to validate a function then it will use objdump to
>>> disassemble the entire file which is not very helpful when processing
>>> a large file (like vmlinux.o).
>>>
>>> Using libopcodes provides more control about the disassembly scope and
>>> output, and it is possible to disassemble a single instruction or
>>> a single function. Now when objtool fails to validate a function it
>>> will disassemble that single function instead of disassembling the
>>> entire file.
>>>
>>> - Add the --trace <function> option to trace function validation
>>>
>>> Figuring out why a function validation has failed can be difficult because
>>> objtool checks all code flows (including alternatives) and maintains
>>> instructions states (in particular call frame information).
>>>
>>> The trace option allows to follow the function validation done by objtool
>>> instruction per instruction, see what objtool is doing and get function
>>> validation information. An output example is shown below.
>>
>> What do I need for this to build? It wasn't compiling due to missing
>> bfd.h, so I installed binutils-devel (Fedora) and now I get:
>
> That's because of the more recent binutils versions, while I have been using an old
> one (2.30) and some functions have changed. But tools/dis-asm-compat.h handles that
> when the appropriate #define is set.
>
> Below is a quick fix (tested with binutils 2.41), and I will work on a proper fix
> (by using the tools/ features to check the disassembler version).
>
Here is the patch to handle both old and new binutils versions:
8<------------------------------------------------------------------->8
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 00350fc7c662..91a2858fea14 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -7,6 +7,11 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
srctree := $(patsubst %/,%,$(dir $(srctree)))
endif
+FEATURE_USER = .objtool
+FEATURE_TESTS = disassembler-init-styled
+FEATURE_DISPLAY = disassembler-init-styled
+include $(srctree)/tools/build/Makefile.feature
+
LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
ifneq ($(OUTPUT),)
LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
@@ -40,6 +45,10 @@ OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(HOSTCC) $(OBJTOOL_CFLAGS) -x c -E - 2>/dev/null | grep elf_getshdr)
OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
+ifeq ($(feature-disassembler-init-styled), 1)
+OBJTOOL_CFLAGS += -DDISASM_INIT_STYLED
+endif
+
# Always want host compilation.
HOST_OVERRIDES := CC="$(HOSTCC)" LD="$(HOSTLD)" AR="$(HOSTAR)"
8<------------------------------------------------------------------->8
alex.
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index 00350fc7c662..300ea727e454 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -36,6 +36,8 @@ WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -
> OBJTOOL_CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) -lopcodes
>
> +OBJTOOL_CFLAGS += -DDISASM_INIT_STYLED
> +
> # Allow old libelf to be used:
> elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(HOSTCC) $(OBJTOOL_CFLAGS) -x c -E - 2>/dev/null | grep elf_getshdr)
> OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
>
>
> alex.
>
>
>> In file included from disas.c:12:
>> /home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h:10:6: error: redeclaration of ‘enum disassembler_style’
>> 10 | enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
>> | ^~~~~~~~~~~~~~~~~~
>> In file included from /home/jpoimboe/git/linux/tools/objtool/include/objtool/arch.h:10,
>> from disas.c:6:
>> /usr/include/dis-asm.h:53:6: note: originally defined here
>> 53 | enum disassembler_style
>> | ^~~~~~~~~~~~~~~~~~
>> /home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h: In function ‘init_disassemble_info_compat’:
>> /home/jpoimboe/git/linux/tools/include/tools/dis-asm-compat.h:50:9: error: too few arguments to function ‘init_disassemble_info’
>> 50 | init_disassemble_info(info, stream,
>> | ^~~~~~~~~~~~~~~~~~~~~
>> /usr/include/dis-asm.h:480:13: note: declared here
>> 480 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
>> | ^~~~~~~~~~~~~~~~~~~~~
>> make[4]: *** [/home/jpoimboe/git/linux/tools/build/Makefile.build:86: /home/jpoimboe/git/linux/tools/objtool/disas.o] Error 1
>> make[4]: *** Waiting for unfinished jobs....
>> make[3]: *** [Makefile:65: /home/jpoimboe/git/linux/tools/objtool/objtool-in.o] Error 2
>> make[2]: *** [Makefile:73: objtool] Error 2
>> make[1]: *** [/home/jpoimboe/git/linux/Makefile:1448: tools/objtool] Error 2
>> make: *** [Makefile:248: __sub-make] Error 2
>>
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-10 13:00 ` Alexandre Chartre
@ 2025-06-10 21:05 ` Josh Poimboeuf
2025-06-11 6:00 ` Alexandre Chartre
0 siblings, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2025-06-10 21:05 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: linux-kernel, mingo, peterz
On Tue, Jun 10, 2025 at 03:00:50PM +0200, Alexandre Chartre wrote:
> Here is the patch to handle both old and new binutils versions:
>
> 8<------------------------------------------------------------------->8
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index 00350fc7c662..91a2858fea14 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -7,6 +7,11 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> srctree := $(patsubst %/,%,$(dir $(srctree)))
> endif
> +FEATURE_USER = .objtool
> +FEATURE_TESTS = disassembler-init-styled
> +FEATURE_DISPLAY = disassembler-init-styled
> +include $(srctree)/tools/build/Makefile.feature
Thanks, that worked.
That Makefile.feature thing is nice (except it prints an annoying
newline on every build after the first one).
Can we also use that to determine if binutils-devel (or binutils-dev or
whatever) is installed, and then make the build of disas.c optional?
Then if somebody tries to use '--trace', it could tell them to install
the binutils development package and rebuild objtool. That way we don't
disrupt everybody's kernel build for a feature they probably won't use.
That would also mean disas_warned_funcs() would be disabled on missing
binutils-devel. But I think that's probably fine. In fact that will
now have less reason for existing now that we have this tracing. Maybe
we won't need it at all.
--
Josh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-10 21:05 ` Josh Poimboeuf
@ 2025-06-11 6:00 ` Alexandre Chartre
2025-06-11 14:20 ` Peter Zijlstra
0 siblings, 1 reply; 32+ messages in thread
From: Alexandre Chartre @ 2025-06-11 6:00 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: alexandre.chartre, linux-kernel, mingo, peterz
On 6/10/25 23:05, Josh Poimboeuf wrote:
> On Tue, Jun 10, 2025 at 03:00:50PM +0200, Alexandre Chartre wrote:
>> Here is the patch to handle both old and new binutils versions:
>>
>> 8<------------------------------------------------------------------->8
>> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
>> index 00350fc7c662..91a2858fea14 100644
>> --- a/tools/objtool/Makefile
>> +++ b/tools/objtool/Makefile
>> @@ -7,6 +7,11 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
>> srctree := $(patsubst %/,%,$(dir $(srctree)))
>> endif
>> +FEATURE_USER = .objtool
>> +FEATURE_TESTS = disassembler-init-styled
>> +FEATURE_DISPLAY = disassembler-init-styled
>> +include $(srctree)/tools/build/Makefile.feature
>
> Thanks, that worked.
>
> That Makefile.feature thing is nice (except it prints an annoying
> newline on every build after the first one).
>
> Can we also use that to determine if binutils-devel (or binutils-dev or
> whatever) is installed, and then make the build of disas.c optional?
This would probably require a new feature test, I will look at it.
> Then if somebody tries to use '--trace', it could tell them to install
> the binutils development package and rebuild objtool. That way we don't
> disrupt everybody's kernel build for a feature they probably won't use.
>
> That would also mean disas_warned_funcs() would be disabled on missing
> binutils-devel. But I think that's probably fine. In fact that will
> now have less reason for existing now that we have this tracing. Maybe
> we won't need it at all.
>
We can also fall back to using objdump if binutils-devel is not there.
And thanks for all the other comments, I will address them and resubmit. I am
also adding the "disas" option.
alex.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 00/13] objtool: Function validation tracing
2025-06-11 6:00 ` Alexandre Chartre
@ 2025-06-11 14:20 ` Peter Zijlstra
0 siblings, 0 replies; 32+ messages in thread
From: Peter Zijlstra @ 2025-06-11 14:20 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: Josh Poimboeuf, linux-kernel, mingo
On Wed, Jun 11, 2025 at 08:00:32AM +0200, Alexandre Chartre wrote:
> > That would also mean disas_warned_funcs() would be disabled on missing
> > binutils-devel. But I think that's probably fine. In fact that will
> > now have less reason for existing now that we have this tracing. Maybe
> > we won't need it at all.
> >
>
> We can also fall back to using objdump if binutils-devel is not there.
I don't think that is needed; normal builds will never use this code.
Just print "Rebuild with libopcodes for disasm support" or somesuch when
not present. Then developers will know what to do when they need it.
^ permalink raw reply [flat|nested] 32+ messages in thread