On Tue, Jun 09, 2020 at 09:54:33AM +0100, Julien Thierry wrote:
> Hi Matt,
>
> On 6/2/20 8:49 PM, Matt Helsley wrote:
> > Move recordmcount into the objtool directory. We keep this step separate
> > so changes which turn recordmcount into a subcommand of objtool don't
> > get obscured.
> >
> > Signed-off-by: Matt Helsley <mhels...@vmware.com>
<snip>
> > diff --git a/Makefile b/Makefile
> > index 04f5662ae61a..d353a0a65a71 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -844,6 +844,7 @@ ifdef CONFIG_DYNAMIC_FTRACE
> > ifdef CONFIG_HAVE_C_RECORDMCOUNT
> > BUILD_C_RECORDMCOUNT := y
> > export BUILD_C_RECORDMCOUNT
> > + objtool_target := tools/objtool FORCE
> > endif
> > endif
> > endif
> > @@ -1023,10 +1024,10 @@ endif
> > export mod_sign_cmd
> > HOST_LIBELF_LIBS = $(shell pkg-config libelf --libs 2>/dev/null || echo
> > -lelf)
> > +has_libelf := $(call try-run,\
> > + echo "int main() {}" | $(HOSTCC) -xc -o /dev/null
> > $(HOST_LIBELF_LIBS) -,1,0)
>
> Maybe there could be some build dependency, e.g. CONFIG_OBJTOOL_SUBCMDS that
> sets the "objtool_target" and "has_libelf" when selected.
>
> Then the CONFIG_UNWINDER_ORC, RECORD_MCOUNT and STACK_VALIDATION would just
> had to select that config option.
That might save a good amount of control flow in the Makefiles.
We could take it one step further and have specific CONFIG_OBJTOOL_<subcmd>
which might help us remove the per-architecture control-flow in
the multi-arch subcmd support found in tools/objtool/Makefile.
What do folks think of that -- too far?
>
> > ifdef CONFIG_STACK_VALIDATION
> > - has_libelf := $(call try-run,\
> > - echo "int main() {}" | $(HOSTCC) -xc -o /dev/null
> > $(HOST_LIBELF_LIBS) -,1,0)
> > ifeq ($(has_libelf),1)
> > objtool_target := tools/objtool FORCE
> > else
> > @@ -1163,13 +1164,15 @@ uapi-asm-generic:
> > PHONY += prepare-objtool
> > prepare-objtool: $(objtool_target)
> > -ifeq ($(SKIP_STACK_VALIDATION),1)
> > -ifdef CONFIG_UNWINDER_ORC
> > +ifneq ($(has_libelf),1)
> > + ifdef CONFIG_UNWINDER_ORC
> > @echo "error: Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y,
> > please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2
> > @false
> > -else
> > + else
> > + ifeq ($(SKIP_STACK_VALIDATION),1)
> > @echo "warning: Cannot use CONFIG_STACK_VALIDATION=y, please install
> > libelf-dev, libelf-devel or elfutils-libelf-devel" >&2
>
>
> I think this would be more readable without the else branch:
>
> ifneq ($(has_libelf),1)
> ifdef <some objtool command config>
> <warn about unavailability>
Note: error not warn
> endif
> ifdef <another objtool command config>
> <warn ...>
> endif
> <...>
> endif
I think the next patch, which makes recordmcount a subcmd, makes it a
little clearer what the pattern is because it adds another ifdef block
in the way you suggest.
As for the else around the SKIP_STACK_VALIDATION check -- it is special
in a couple ways -- at least as best I can tell.
It's not a CONFIG_* -- it actually breaks the normal pattern with
CONFIG_* in that..
It's about a judgement call that it's OK to merely warn and skip the
stack validation rather than produce an error. The other, CONFIG_*
blocks produce errors.
These two reasons are why I think it makes sense to keep the logic
distinct with the "else".
Cheers,
-Matt Helsley
