Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sat, 26 Feb 2022 11:42:09 -0800
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     x86@kernel.org, joao@overdrivepizza.com, hjl.tools@gmail.com,
        andrew.cooper3@citrix.com, linux-kernel@vger.kernel.org,
        ndesaulniers@google.com, keescook@chromium.org,
        samitolvanen@google.com, mark.rutland@arm.com,
        alyssa.milburn@intel.com
Subject: Re: [PATCH 21/29] objtool: Rename --duplicate to --lto
Message-ID: <20220226194209.bvv3t65hhtnwltmk@treble>
References: <20220218164902.008644515@infradead.org>
 <20220218171409.814392411@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220218171409.814392411@infradead.org>
Precedence: bulk

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) <peterz@infradead.org>
> ---
>  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", &lto, "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