Re: [RFC][PATCH v4 01/32] objtool: Prepare to merge recordmcount
From: Julien Thierry
Date: Tue Jun 09 2020 - 15:31:16 EST
On 6/9/20 4:42 PM, Matt Helsley wrote:
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 <mhelsley@xxxxxxxxxx>
<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?
I wasn't completely sure I understood before I saw your reply on the
next patch. I don't think it's too far, it's cleaner! The current way
was good enough to deal with only two x86 specific objtool subcommands.
Since you're adding another one and it is likely that more will be added
in the future, I think it's worth having something a bit more structured
:) .
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
Good point. But since those are errors, you don't need the "else" :)
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..
Yes but $(SKIP_STACK_VALIDATION) == 1 is actually just
CONFIG_STACK_VALIDATION && ($(has_libelf) != 1). And since you are
already in the ifneq ($(has_libelf),1) branch, checking
$(SKIP_STACK_VALIDATION) == 1 is the same as CONFIG_STACK_VALIDATION
being defined.
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.
To me this is minor, we could also imagine another command CONFIG_
performing another action than warning or error when libelf is not
available.
Anyway, this was just a suggestion, I don't want to insist to much on this.
Cheers,
--
Julien Thierry