| Message ID | 1539681053-24388-3-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: add scripts/Makefile.toolcheck to check necessary host tools | expand |
On Tue, Oct 16, 2018 at 06:10:52PM +0900, Masahiro Yamada wrote: > Another behavioral change is, missing libelf for CONFIG_STACK_VALIDATION > was previously a warning, but now a error. This behavioral change should be fine. It was already an error for UNWINDER_ORC, so this would only upgrade a warning to an error for people using STACK_VALIDATION without ORC, which should be a small number of people by this point. > diff --git a/scripts/Makefile.toolcheck b/scripts/Makefile.toolcheck > index f3c165d..bc26fc0 100644 > --- a/scripts/Makefile.toolcheck > +++ b/scripts/Makefile.toolcheck > @@ -12,6 +12,11 @@ include include/config/auto.conf > __toolcheck: > @: > > +chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > +msg_stack_validation = "libelf is necessary for building the objtool." \ > + "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > +toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation > + > PHONY += $(toolcheck-y) > __toolcheck: $(toolcheck-y) This is a nice improvement. It would probably be a good idea to clarify to the user which config option(s) are the cause for the error, by putting "CONFIG_STACK_VALIDATION" in the error string, for example. Though, for this particular case, it would be clearer to have a different error, based on which option is enabled, like we had before. Like: ifdef CONFIG_UNWINDER_ORC chk_unwinder_orc = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - msg_unwinder_orc = "Cannot build objtool to generate ORC metadata for CONFIG_UNWINDER_ORC=y. " \ "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." toolcheck-$(CONFIG_UNWINDER_ORC) += unwinder_orc else chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - msg_stack_validation = "Cannot build objtool for CONFIG_STACK_VALIDATION=y. " \ "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation endif What do you think? -- Josh
On Tue, Oct 16, 2018 at 11:25 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Tue, Oct 16, 2018 at 06:10:52PM +0900, Masahiro Yamada wrote: > > Another behavioral change is, missing libelf for CONFIG_STACK_VALIDATION > > was previously a warning, but now a error. > > This behavioral change should be fine. It was already an error for > UNWINDER_ORC, so this would only upgrade a warning to an error for > people using STACK_VALIDATION without ORC, which should be a small > number of people by this point. > > > diff --git a/scripts/Makefile.toolcheck b/scripts/Makefile.toolcheck > > index f3c165d..bc26fc0 100644 > > --- a/scripts/Makefile.toolcheck > > +++ b/scripts/Makefile.toolcheck > > @@ -12,6 +12,11 @@ include include/config/auto.conf > > __toolcheck: > > @: > > > > +chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > +msg_stack_validation = "libelf is necessary for building the objtool." \ > > + "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > +toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation > > + > > PHONY += $(toolcheck-y) > > __toolcheck: $(toolcheck-y) > > This is a nice improvement. > > It would probably be a good idea to clarify to the user which config > option(s) are the cause for the error, by putting > "CONFIG_STACK_VALIDATION" in the error string, for example. > > Though, for this particular case, it would be clearer to have a > different error, based on which option is enabled, like we had before. > > Like: > > > ifdef CONFIG_UNWINDER_ORC > > chk_unwinder_orc = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > msg_unwinder_orc = "Cannot build objtool to generate ORC metadata for CONFIG_UNWINDER_ORC=y. " \ > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > toolcheck-$(CONFIG_UNWINDER_ORC) += unwinder_orc > > else > > chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > msg_stack_validation = "Cannot build objtool for CONFIG_STACK_VALIDATION=y. " \ > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation > > endif > > > What do you think? It is ugly. Do you need such detailed information like ORC metadata stuff here? This Makefile aims to error out, showing why the build failed. That's it. -- Best Regards Masahiro Yamada
On Wed, Oct 17, 2018 at 12:51:40AM +0900, Masahiro Yamada wrote: > > ifdef CONFIG_UNWINDER_ORC > > > > chk_unwinder_orc = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > msg_unwinder_orc = "Cannot build objtool to generate ORC metadata for CONFIG_UNWINDER_ORC=y. " \ > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > toolcheck-$(CONFIG_UNWINDER_ORC) += unwinder_orc > > > > else > > > > chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > msg_stack_validation = "Cannot build objtool for CONFIG_STACK_VALIDATION=y. " \ > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation > > > > endif > > > > > > What do you think? > > > It is ugly. > > Do you need such detailed information like ORC metadata stuff here? > > This Makefile aims to error out, showing why the build failed. > That's it. Yeah, it is kind of ugly. But the "showing why the build failed" part is important. I was trying to give the user a clear error message, similar to what we have today. Without context, the user won't know what objtool is, or why it needs to be built. If we have just a single error message for all cases, it should at least mention the config option. Like "Cannot build objtool for CONFIG_STACK_VALIDATION." But then, most users will only have that enabled because of ORC. So an ORC-specific message would be more appropriate in most cases. So maybe it can just be something more vague: msg_stack_validation = "Cannot build objtool for CONFIG_UNWINDER_ORC and/or CONFIG_STACK_VALIDATION. " \ "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." That would probably be good enough. Then we could drop the ugly ifdef. -- Josh
Hi Josh, On Wed, Oct 17, 2018 at 1:16 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Wed, Oct 17, 2018 at 12:51:40AM +0900, Masahiro Yamada wrote: > > > ifdef CONFIG_UNWINDER_ORC > > > > > > chk_unwinder_orc = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > > msg_unwinder_orc = "Cannot build objtool to generate ORC metadata for CONFIG_UNWINDER_ORC=y. " \ > > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > > toolcheck-$(CONFIG_UNWINDER_ORC) += unwinder_orc > > > > > > else > > > > > > chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > > msg_stack_validation = "Cannot build objtool for CONFIG_STACK_VALIDATION=y. " \ > > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > > toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation > > > > > > endif > > > > > > > > > What do you think? > > > > > > It is ugly. > > > > Do you need such detailed information like ORC metadata stuff here? > > > > This Makefile aims to error out, showing why the build failed. > > That's it. > > Yeah, it is kind of ugly. But the "showing why the build failed" part > is important. I was trying to give the user a clear error message, > similar to what we have today. > > Without context, the user won't know what objtool is, or why it needs to > be built. > > If we have just a single error message for all cases, it should at least > mention the config option. Like > > "Cannot build objtool for CONFIG_STACK_VALIDATION." > > But then, most users will only have that enabled because of ORC. So an > ORC-specific message would be more appropriate in most cases. > > So maybe it can just be something more vague: > > msg_stack_validation = "Cannot build objtool for CONFIG_UNWINDER_ORC and/or CONFIG_STACK_VALIDATION. " \ > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > That would probably be good enough. Then we could drop the ugly ifdef. Fair point, but I am confused by the current STACK_VALIDATION / UNWINDER_ORC logic. In my understanding, objtool is an all-in-one object check/manipulation tool. STACK_VALIDATION and UNWINDER_ORC is a selection of a sub-command, 'check' or 'orc generate'. (Correct me if am wrong.) However, STACK_VALIDATION is still used to decide whether or not to compile the objtool. Adding a new symbol OBJTOOL would clarify the logic. config OBJTOOL bool config STACK_VALIDATION bool "Compile-time stack metadata validation" depends on HAVE_STACK_VALIDATION select OBJTOOL ... config UNWINDER_ORC bool "ORC unwinder" depends on X86_64 select OBJTOOL ... -- Best Regards Masahiro Yamada
On Fri, Oct 19, 2018 at 03:04:22PM +0900, Masahiro Yamada wrote: > Hi Josh, > > > On Wed, Oct 17, 2018 at 1:16 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > On Wed, Oct 17, 2018 at 12:51:40AM +0900, Masahiro Yamada wrote: > > > > ifdef CONFIG_UNWINDER_ORC > > > > > > > > chk_unwinder_orc = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > > > msg_unwinder_orc = "Cannot build objtool to generate ORC metadata for CONFIG_UNWINDER_ORC=y. " \ > > > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > > > toolcheck-$(CONFIG_UNWINDER_ORC) += unwinder_orc > > > > > > > > else > > > > > > > > chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - > > > > msg_stack_validation = "Cannot build objtool for CONFIG_STACK_VALIDATION=y. " \ > > > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > > > toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation > > > > > > > > endif > > > > > > > > > > > > What do you think? > > > > > > > > > It is ugly. > > > > > > Do you need such detailed information like ORC metadata stuff here? > > > > > > This Makefile aims to error out, showing why the build failed. > > > That's it. > > > > Yeah, it is kind of ugly. But the "showing why the build failed" part > > is important. I was trying to give the user a clear error message, > > similar to what we have today. > > > > Without context, the user won't know what objtool is, or why it needs to > > be built. > > > > If we have just a single error message for all cases, it should at least > > mention the config option. Like > > > > "Cannot build objtool for CONFIG_STACK_VALIDATION." > > > > But then, most users will only have that enabled because of ORC. So an > > ORC-specific message would be more appropriate in most cases. > > > > So maybe it can just be something more vague: > > > > msg_stack_validation = "Cannot build objtool for CONFIG_UNWINDER_ORC and/or CONFIG_STACK_VALIDATION. " \ > > "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." > > > > That would probably be good enough. Then we could drop the ugly ifdef. > > > Fair point, but I am confused by the current > STACK_VALIDATION / UNWINDER_ORC logic. > > In my understanding, objtool is > an all-in-one object check/manipulation tool. > > STACK_VALIDATION and UNWINDER_ORC > is a selection of a sub-command, 'check' or 'orc generate'. > > (Correct me if am wrong.) > > > However, STACK_VALIDATION is still used to > decide whether or not to compile the objtool. > > > Adding a new symbol OBJTOOL would clarify the logic. > > > > config OBJTOOL > bool > > config STACK_VALIDATION > bool "Compile-time stack metadata validation" > depends on HAVE_STACK_VALIDATION > select OBJTOOL > ... > > > config UNWINDER_ORC > bool "ORC unwinder" > depends on X86_64 > select OBJTOOL > ... While 'orc generate' and 'check' are indeed separate subcommands of objtool, the functionality of 'orc generate' is actually a superset of the functionality of 'check'. In other words, ORC generation relies on the stack validation feature, which is consistent with the current config logic. -- Josh
diff --git a/Makefile b/Makefile index 23a204a..71940b7 100644 --- a/Makefile +++ b/Makefile @@ -949,23 +949,6 @@ mod_sign_cmd = true endif export mod_sign_cmd -ifdef CONFIG_STACK_VALIDATION - has_libelf := $(call try-run,\ - echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf -,1,0) - ifeq ($(has_libelf),1) - objtool_target := tools/objtool FORCE - else - ifdef CONFIG_UNWINDER_ORC - $(error "Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel") - else - $(warning "Cannot use CONFIG_STACK_VALIDATION=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel") - endif - SKIP_STACK_VALIDATION := 1 - export SKIP_STACK_VALIDATION - endif -endif - - ifeq ($(KBUILD_EXTMOD),) core-y += kernel/ certs/ mm/ fs/ ipc/ security/ crypto/ block/ @@ -1115,7 +1098,9 @@ uapi-asm-generic: src=uapi/asm obj=arch/$(SRCARCH)/include/generated/uapi/asm PHONY += prepare-objtool -prepare-objtool: $(objtool_target) +ifdef CONFIG_STACK_VALIDATION +prepare-objtool: tools/objtool +endif # Generate some files # --------------------------------------------------------------------------- diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 54da4b0..e9dabe4 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -233,7 +233,6 @@ endif # CC_USING_RECORD_MCOUNT endif # CONFIG_FTRACE_MCOUNT_RECORD ifdef CONFIG_STACK_VALIDATION -ifneq ($(SKIP_STACK_VALIDATION),1) __objtool_obj := $(objtree)/tools/objtool/objtool @@ -270,7 +269,6 @@ objtool_obj = $(if $(patsubst y%,, \ $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \ $(__objtool_obj)) -endif # SKIP_STACK_VALIDATION endif # CONFIG_STACK_VALIDATION # Rebuild all objects when objtool changes, or is enabled/disabled. diff --git a/scripts/Makefile.toolcheck b/scripts/Makefile.toolcheck index f3c165d..bc26fc0 100644 --- a/scripts/Makefile.toolcheck +++ b/scripts/Makefile.toolcheck @@ -12,6 +12,11 @@ include include/config/auto.conf __toolcheck: @: +chk_stack_validation = echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf - +msg_stack_validation = "libelf is necessary for building the objtool." \ + "Please install libelf-dev, libelf-devel or elfutils-libelf-devel." +toolcheck-$(CONFIG_STACK_VALIDATION) += stack_validation + PHONY += $(toolcheck-y) __toolcheck: $(toolcheck-y)