In order to prepare for LTO like objtool runs for modules, rename the
duplicate argument to lto.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
scripts/link-vmlinux.sh | 2 +-
tools/objtool/builtin-check.c | 4 ++--
tools/objtool/check.c | 7 ++++++-
tools/objtool/include/objtool/builtin.h | 2 +-
4 files changed, 10 insertions(+), 5 deletions(-)
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -115,7 +115,7 @@ objtool_link()
objtoolcmd="orc generate"
fi
- objtoolopt="${objtoolopt} --duplicate"
+ objtoolopt="${objtoolopt} --lto"
if is_enabled CONFIG_FTRACE_MCOUNT_USE_OBJTOOL; then
objtoolopt="${objtoolopt} --mcount"
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -20,7 +20,7 @@
#include <objtool/objtool.h>
bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
- validate_dup, vmlinux, mcount, noinstr, backup, sls, dryrun;
+ lto, vmlinux, mcount, noinstr, backup, sls, dryrun;
static const char * const check_usage[] = {
"objtool check [<options>] file.o",
@@ -40,7 +40,7 @@ const struct option check_options[] = {
OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
OPT_BOOLEAN('s', "stats", &stats, "print statistics"),
- OPT_BOOLEAN('d', "duplicate", &validate_dup, "duplicate validation for vmlinux.o"),
+ OPT_BOOLEAN(0, "lto", <o, "whole-archive like runs"),
OPT_BOOLEAN('n', "noinstr", &noinstr, "noinstr validation for vmlinux.o"),
OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"),
OPT_BOOLEAN('M', "mcount", &mcount, "generate __mcount_loc"),
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3501,6 +3501,11 @@ int check(struct objtool_file *file)
{
int ret, warnings = 0;
+ if (lto && !(vmlinux || module)) {
+ fprintf(stderr, "--lto requires: --vmlinux or --module\n");
+ return 1;
+ }
+
arch_initial_func_cfi_state(&initial_func_cfi);
init_cfi_state(&init_cfi);
init_cfi_state(&func_cfi);
@@ -3521,7 +3526,7 @@ int check(struct objtool_file *file)
if (list_empty(&file->insn_list))
goto out;
- if (vmlinux && !validate_dup) {
+ if (vmlinux && !lto) {
ret = validate_vmlinux_functions(file);
if (ret < 0)
goto out;
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -9,7 +9,7 @@
extern const struct option check_options[];
extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
- validate_dup, vmlinux, mcount, noinstr, backup, sls, dryrun;
+ lto, vmlinux, mcount, noinstr, backup, sls, dryrun;
extern int cmd_parse_options(int argc, const char **argv, const char * const usage[]);
On Fri, Feb 18, 2022 at 05:49:23PM +0100, Peter Zijlstra wrote:
> In order to prepare for LTO like objtool runs for modules, rename the
> duplicate argument to lto.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> scripts/link-vmlinux.sh | 2 +-
> tools/objtool/builtin-check.c | 4 ++--
> tools/objtool/check.c | 7 ++++++-
> tools/objtool/include/objtool/builtin.h | 2 +-
> 4 files changed, 10 insertions(+), 5 deletions(-)
>
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -115,7 +115,7 @@ objtool_link()
> objtoolcmd="orc generate"
> fi
>
> - objtoolopt="${objtoolopt} --duplicate"
> + objtoolopt="${objtoolopt} --lto"
>
> if is_enabled CONFIG_FTRACE_MCOUNT_USE_OBJTOOL; then
> objtoolopt="${objtoolopt} --mcount"
> --- a/tools/objtool/builtin-check.c
> +++ b/tools/objtool/builtin-check.c
> @@ -20,7 +20,7 @@
> #include <objtool/objtool.h>
>
> bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
> - validate_dup, vmlinux, mcount, noinstr, backup, sls, dryrun;
> + lto, vmlinux, mcount, noinstr, backup, sls, dryrun;
>
> static const char * const check_usage[] = {
> "objtool check [<options>] file.o",
> @@ -40,7 +40,7 @@ const struct option check_options[] = {
> OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
> OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
> OPT_BOOLEAN('s', "stats", &stats, "print statistics"),
> - OPT_BOOLEAN('d', "duplicate", &validate_dup, "duplicate validation for vmlinux.o"),
> + OPT_BOOLEAN(0, "lto", <o, "whole-archive like runs"),
"--lto" is a confusing name, since this "feature" isn't specific to LTO.
Also, it gives no indication of what it actually does.
What it does is, run objtool on vmlinux or module just like it's a
normal object, and *don't* do noinstr validation. Right?
It's weird for the noinstr-only-mode to be the default.
BTW "--duplicate" had similar problems...
So how about:
- Default to normal mode on vmlinux/module, i.e. validate and/or
generate ORC like any other object. This default is more logically
consistent and makes sense for the future once we get around to
parallelizing objtool.
- Have "--noinstr", which does noinstr validation, in addition to all
the other objtool validation/generation. So it's additive, like any
other cmdline option. (Maybe this option isn't necessarily needed for
now.)
- Have "--noinstr-only" which only does noinstr validation and nothing
else. (Alternatively, "--noinstr --dry-run")
?
--
Josh
On Sat, Feb 26, 2022 at 11:42:13AM -0800, Josh Poimboeuf wrote:
> > + OPT_BOOLEAN(0, "lto", <o, "whole-archive like runs"),
>
> "--lto" is a confusing name, since this "feature" isn't specific to LTO.
>
> Also, it gives no indication of what it actually does.
>
> What it does is, run objtool on vmlinux or module just like it's a
> normal object, and *don't* do noinstr validation. Right?
>
> It's weird for the noinstr-only-mode to be the default.
>
> BTW "--duplicate" had similar problems...
>
> So how about:
>
> - Default to normal mode on vmlinux/module, i.e. validate and/or
> generate ORC like any other object. This default is more logically
> consistent and makes sense for the future once we get around to
> parallelizing objtool.
>
> - Have "--noinstr", which does noinstr validation, in addition to all
> the other objtool validation/generation. So it's additive, like any
> other cmdline option. (Maybe this option isn't necessarily needed for
> now.)
It just dawned on me that "--noinstr" already exists. But I'm
scratching my head trying to figure out the difference between
"--noinstr" and omitting "--lto".
> - Have "--noinstr-only" which only does noinstr validation and nothing
> else. (Alternatively, "--noinstr --dry-run")
>
> ?
--
Josh
On Sat, Feb 26, 2022 at 01:48:02PM -0800, Josh Poimboeuf wrote:
> On Sat, Feb 26, 2022 at 11:42:13AM -0800, Josh Poimboeuf wrote:
> > > + OPT_BOOLEAN(0, "lto", <o, "whole-archive like runs"),
> >
> > "--lto" is a confusing name, since this "feature" isn't specific to LTO.
> >
> > Also, it gives no indication of what it actually does.
> >
> > What it does is, run objtool on vmlinux or module just like it's a
> > normal object, and *don't* do noinstr validation. Right?
How about --whole-archive, much like the linker then?
The distinction is that we run objtool *only* on vmlinux and modules and
not also on the individual .o files.
There's 3 models:
A) every translation unit
(module parts get --module)
B) every translation unit + shallow vmlinux
(module parts get --module, vmlinux.o gets --vmlinux)
C) vmlinux + modules
(modules get --module, vmlinux.o gets --vmlinux
--duplicate/lto/whole-archive, pick your poison).
objtool started out with (A); then for noinstr validation I added a
shallow vmlinux pass that *only* checks .noinstr.text and .entry.text
for escapes (B). This is to not unduly add time to the slowest (single
threaded) part of the kernel build, linking vmlinux.
Then CLANG_LTO added (C), due to LTO there simply isn't asm to poke at
until the whole-archive thing. But this means that the vmlinux run needs
to do all validation, not only the shallow noinstr validation.
--duplicate was added there, a bad name because it really doesn't do
duplicate work, it's the first and only objtool run (it's only duplicate
if you also run on each TU, but we don't do that).
Now with these patches I need whole-archive objtool passes and instead
of making a 4th mode, or extend (B), I chose to just bite the bullet and
go full LTO style (C).
Now, I figured it would be good to have a flag to indicate we're running
LTO style and --duplicate is more or less that, except for the terrible
name.
> > It's weird for the noinstr-only-mode to be the default.
> >
> > BTW "--duplicate" had similar problems...
> >
> > So how about:
> >
> > - Default to normal mode on vmlinux/module, i.e. validate and/or
> > generate ORC like any other object. This default is more logically
> > consistent and makes sense for the future once we get around to
> > parallelizing objtool.
> >
> > - Have "--noinstr", which does noinstr validation, in addition to all
> > the other objtool validation/generation. So it's additive, like any
> > other cmdline option. (Maybe this option isn't necessarily needed for
> > now.)
>
> It just dawned on me that "--noinstr" already exists. But I'm
> scratching my head trying to figure out the difference between
> "--noinstr" and omitting "--lto".
If you run: "objtool check --vmlinux --noinstr vmlinux.o", it'll only do
the shallow .noinstr.text/.entry.text checks. If OTOH you do: "objtool
check --vmlinux --noinstr --lto vmlinux.o" it'll do everything
(including noinstr).
Similarlt, "--module --lto" will come to mean whole module (which is
currently not distinguishable from a regular module part run).
(barring the possible 's/--lto/--whole-archive/' rename proposed up top)
On Mon, Feb 28, 2022 at 12:05:06PM +0100, Peter Zijlstra wrote:
> > It just dawned on me that "--noinstr" already exists. But I'm
> > scratching my head trying to figure out the difference between
> > "--noinstr" and omitting "--lto".
>
> If you run: "objtool check --vmlinux --noinstr vmlinux.o", it'll only do
> the shallow .noinstr.text/.entry.text checks. If OTOH you do: "objtool
> check --vmlinux --noinstr --lto vmlinux.o" it'll do everything
> (including noinstr).
I think I got all that. But what does "--vmlinux" do by itself?
> Similarlt, "--module --lto" will come to mean whole module (which is
> currently not distinguishable from a regular module part run).
>
> (barring the possible 's/--lto/--whole-archive/' rename proposed up top)
Thanks for the explanations. To summarize, we have:
A) legacy mode:
translation unit: objtool check [--module]
vmlinux.o: N/A
module: N/A
B) CONFIG_VMLINUX_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y)
translation unit: objtool check [--module]
vmlinux: objtool check --vmlinux --noinstr
module: objtool check --module --noinstr
C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
translation unit: N/A
vmlinux: objtool check --vmlinux --noinstr --lto
module: objtool check --module --noinstr --lto
Right?
I think I get it, but it's mental gymnastics for me to remember how the
options interact. It still seems counterintuitive, because whatever
"objtool check" does to a translation unit, I'd expect "objtool check
--vmlinux" to do the same things.
I suppose it makes sense if I can remember that --vmlinux is a magical
option which disables all that other stuff. And it's counteracted by
--lto, which removes the magic. But that's all hard to remember and
just seems weird.
There are a variety of ways to run objtool against vmlinux. The "lto"
approach is going to be less of an exception and may end up being the
default someday. So making --vmlinux do weird stuff is going to be even
less intuitive as we go forward. Let's make the default sane and
consistent with other file types.
So how about we just get rid of the magical --vmlinux and --lto options
altogether, and make --noinstr additive, like all the other options?
A) legacy mode:
.o files: objtool check [--module]
vmlinux: N/A
module: N/A
B) CONFIG_NOINSTR_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y):
.o files: objtool check [--module]
vmlinux: objtool check --noinstr-only
module: objtool check --module --noinstr-only
C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
.o files: N/A
vmlinux: objtool check --noinstr
module: objtool check --module --noinstr
(notice I renamed VMLINUX_VALIDATION to NOINSTR_VALIDATION)
Isn't that much more logical and intuitive?
a) objtool has sane defaults, regardless of object type
b) no magic options, other than --noinstr-only, but that's
communicated in its name
c) --vmlinux is no longer needed -- fewer options to juggle
--
Josh
On Mon, Feb 28, 2022 at 09:09:34PM +0100, Peter Zijlstra wrote:
> > So how about we just get rid of the magical --vmlinux and --lto options
> > altogether, and make --noinstr additive, like all the other options?
> >
> > A) legacy mode:
> > .o files: objtool check [--module]
> > vmlinux: N/A
> > module: N/A
> >
> > B) CONFIG_NOINSTR_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y):
> > .o files: objtool check [--module]
> > vmlinux: objtool check --noinstr-only
> > module: objtool check --module --noinstr-only
> >
> > C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
> > .o files: N/A
> > vmlinux: objtool check --noinstr
> > module: objtool check --module --noinstr
>
> I like the --noinstr-only thing. But I think I still like a flag to
> differentiate between TU/.o file and vmlinux/whole-module invocation.
I'm missing why that would still be useful.
> Anyway, you ok with me cleaning this up later, in a separate series?
Sure. It's already less than ideal today anyway, with '--vmlinux' and
'--duplicate'.
--
Josh
On Mon, Feb 28, 2022 at 10:32:28AM -0800, Josh Poimboeuf wrote:
> Thanks for the explanations. To summarize, we have:
>
> A) legacy mode:
>
> translation unit: objtool check [--module]
> vmlinux.o: N/A
> module: N/A
>
> B) CONFIG_VMLINUX_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y)
>
> translation unit: objtool check [--module]
> vmlinux: objtool check --vmlinux --noinstr
> module: objtool check --module --noinstr
Not the module thing here; noinstr never leaves the core kernel (for
now; I need me a few compiler features before I can tackle the idle path
issues).
> C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
>
> translation unit: N/A
> vmlinux: objtool check --vmlinux --noinstr --lto
> module: objtool check --module --noinstr --lto
>
> Right?
More or less, with the one caveat above.
> I think I get it, but it's mental gymnastics for me to remember how the
> options interact. It still seems counterintuitive, because whatever
> "objtool check" does to a translation unit, I'd expect "objtool check
> --vmlinux" to do the same things.
I think I agree. It is a bit weird.
> So how about we just get rid of the magical --vmlinux and --lto options
> altogether, and make --noinstr additive, like all the other options?
>
> A) legacy mode:
> .o files: objtool check [--module]
> vmlinux: N/A
> module: N/A
>
> B) CONFIG_NOINSTR_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y):
> .o files: objtool check [--module]
> vmlinux: objtool check --noinstr-only
> module: objtool check --module --noinstr-only
>
> C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
> .o files: N/A
> vmlinux: objtool check --noinstr
> module: objtool check --module --noinstr
I like the --noinstr-only thing. But I think I still like a flag to
differentiate between TU/.o file and vmlinux/whole-module invocation.
Anyway, you ok with me cleaning this up later, in a separate series?
On Mon, 28 Feb 2022, Josh Poimboeuf wrote:
> On Mon, Feb 28, 2022 at 09:09:34PM +0100, Peter Zijlstra wrote:
> > > So how about we just get rid of the magical --vmlinux and --lto options
> > > altogether, and make --noinstr additive, like all the other options?
> > >
> > > A) legacy mode:
> > > .o files: objtool check [--module]
> > > vmlinux: N/A
> > > module: N/A
> > >
> > > B) CONFIG_NOINSTR_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y):
> > > .o files: objtool check [--module]
> > > vmlinux: objtool check --noinstr-only
> > > module: objtool check --module --noinstr-only
> > >
> > > C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
> > > .o files: N/A
> > > vmlinux: objtool check --noinstr
> > > module: objtool check --module --noinstr
> >
> > I like the --noinstr-only thing. But I think I still like a flag to
> > differentiate between TU/.o file and vmlinux/whole-module invocation.
>
> I'm missing why that would still be useful.
>
> > Anyway, you ok with me cleaning this up later, in a separate series?
>
> Sure. It's already less than ideal today anyway, with '--vmlinux' and
> '--duplicate'.
Since I always have hard times to figure out different passes and options
of objtool, could you add the above description (its final version) to
tools/objtool/Documentation/ as a part of the cleanup series, please?
Miroslav