| Message ID | 20200624203200.78870-6-samitolvanen@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | add support for Clang LTO | expand |
On Wed, Jun 24, 2020 at 01:31:43PM -0700, Sami Tolvanen wrote: > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 30827f82ad62..12b115152532 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -120,7 +120,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > /* Annotate a C jump table to allow objtool to follow the code flow */ > #define __annotate_jump_table __section(.rodata..c_jump_table) > > -#ifdef CONFIG_DEBUG_ENTRY > +#if defined(CONFIG_DEBUG_ENTRY) || defined(CONFIG_LTO_CLANG) > /* Begin/end of an instrumentation safe region */ > #define instrumentation_begin() ({ \ > asm volatile("%c0:\n\t" \ Why would you be doing noinstr validation for lto builds? That doesn't make sense. > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 9ad9210d70a1..9fdba71c135a 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -399,7 +399,7 @@ config STACK_VALIDATION > > config VMLINUX_VALIDATION > bool > - depends on STACK_VALIDATION && DEBUG_ENTRY && !PARAVIRT > + depends on STACK_VALIDATION && (DEBUG_ENTRY || LTO_CLANG) && !PARAVIRT > default y > For that very same reason you shouldn't be excluding paravirt either. > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal > index d168f0cfe67c..9f1df2f1fab5 100644 > --- a/scripts/Makefile.modfinal > +++ b/scripts/Makefile.modfinal > @@ -48,6 +48,21 @@ endif # CC_USING_PATCHABLE_FUNCTION_ENTRY > endif # CC_USING_RECORD_MCOUNT > endif # CONFIG_FTRACE_MCOUNT_RECORD > > +ifdef CONFIG_STACK_VALIDATION > +ifneq ($(SKIP_STACK_VALIDATION),1) > +cmd_ld_ko_o += \ > + $(objtree)/tools/objtool/objtool \ > + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ > + --module \ > + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ > + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ > + $(if $(CONFIG_RETPOLINE),--retpoline,) \ > + $(if $(CONFIG_X86_SMAP),--uaccess,) \ > + $(@:.ko=$(prelink-ext).o); > + > +endif # SKIP_STACK_VALIDATION > +endif # CONFIG_STACK_VALIDATION What about the objtool invocation from link-vmlinux.sh ?
On Wed, Jun 24, 2020 at 11:19:08PM +0200, Peter Zijlstra wrote: > On Wed, Jun 24, 2020 at 01:31:43PM -0700, Sami Tolvanen wrote: > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > index 30827f82ad62..12b115152532 100644 > > --- a/include/linux/compiler.h > > +++ b/include/linux/compiler.h > > @@ -120,7 +120,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > > /* Annotate a C jump table to allow objtool to follow the code flow */ > > #define __annotate_jump_table __section(.rodata..c_jump_table) > > > > -#ifdef CONFIG_DEBUG_ENTRY > > +#if defined(CONFIG_DEBUG_ENTRY) || defined(CONFIG_LTO_CLANG) > > /* Begin/end of an instrumentation safe region */ > > #define instrumentation_begin() ({ \ > > asm volatile("%c0:\n\t" \ > > Why would you be doing noinstr validation for lto builds? That doesn't > make sense. This is just to avoid a ton of noinstr warnings when we run objtool on vmlinux.o, but I'm also fine with skipping noinstr validation with LTO. > > +ifdef CONFIG_STACK_VALIDATION > > +ifneq ($(SKIP_STACK_VALIDATION),1) > > +cmd_ld_ko_o += \ > > + $(objtree)/tools/objtool/objtool \ > > + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ > > + --module \ > > + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ > > + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ > > + $(if $(CONFIG_RETPOLINE),--retpoline,) \ > > + $(if $(CONFIG_X86_SMAP),--uaccess,) \ > > + $(@:.ko=$(prelink-ext).o); > > + > > +endif # SKIP_STACK_VALIDATION > > +endif # CONFIG_STACK_VALIDATION > > What about the objtool invocation from link-vmlinux.sh ? What about it? The existing objtool_link invocation in link-vmlinux.sh works fine for our purposes as well. Sami
On Wed, Jun 24, 2020 at 02:49:25PM -0700, Sami Tolvanen wrote: > On Wed, Jun 24, 2020 at 11:19:08PM +0200, Peter Zijlstra wrote: > > On Wed, Jun 24, 2020 at 01:31:43PM -0700, Sami Tolvanen wrote: > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > > index 30827f82ad62..12b115152532 100644 > > > --- a/include/linux/compiler.h > > > +++ b/include/linux/compiler.h > > > @@ -120,7 +120,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > > > /* Annotate a C jump table to allow objtool to follow the code flow */ > > > #define __annotate_jump_table __section(.rodata..c_jump_table) > > > > > > -#ifdef CONFIG_DEBUG_ENTRY > > > +#if defined(CONFIG_DEBUG_ENTRY) || defined(CONFIG_LTO_CLANG) > > > /* Begin/end of an instrumentation safe region */ > > > #define instrumentation_begin() ({ \ > > > asm volatile("%c0:\n\t" \ > > > > Why would you be doing noinstr validation for lto builds? That doesn't > > make sense. > > This is just to avoid a ton of noinstr warnings when we run objtool on > vmlinux.o, but I'm also fine with skipping noinstr validation with LTO. Right, then we need to make --no-vmlinux work properly when !DEBUG_ENTRY, which I think might be buggered due to us overriding the argument when the objname ends with "vmlinux.o". > > > +ifdef CONFIG_STACK_VALIDATION > > > +ifneq ($(SKIP_STACK_VALIDATION),1) > > > +cmd_ld_ko_o += \ > > > + $(objtree)/tools/objtool/objtool \ > > > + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ > > > + --module \ > > > + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ > > > + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ > > > + $(if $(CONFIG_RETPOLINE),--retpoline,) \ > > > + $(if $(CONFIG_X86_SMAP),--uaccess,) \ > > > + $(@:.ko=$(prelink-ext).o); > > > + > > > +endif # SKIP_STACK_VALIDATION > > > +endif # CONFIG_STACK_VALIDATION > > > > What about the objtool invocation from link-vmlinux.sh ? > > What about it? The existing objtool_link invocation in link-vmlinux.sh > works fine for our purposes as well. Well, I was wondering why you're adding yet another objtool invocation while we already have one.
On Thu, Jun 25, 2020 at 09:47:16AM +0200, Peter Zijlstra wrote: > On Wed, Jun 24, 2020 at 02:49:25PM -0700, Sami Tolvanen wrote: > > On Wed, Jun 24, 2020 at 11:19:08PM +0200, Peter Zijlstra wrote: > > > On Wed, Jun 24, 2020 at 01:31:43PM -0700, Sami Tolvanen wrote: > > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > > > index 30827f82ad62..12b115152532 100644 > > > > --- a/include/linux/compiler.h > > > > +++ b/include/linux/compiler.h > > > > @@ -120,7 +120,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > > > > /* Annotate a C jump table to allow objtool to follow the code flow */ > > > > #define __annotate_jump_table __section(.rodata..c_jump_table) > > > > > > > > -#ifdef CONFIG_DEBUG_ENTRY > > > > +#if defined(CONFIG_DEBUG_ENTRY) || defined(CONFIG_LTO_CLANG) > > > > /* Begin/end of an instrumentation safe region */ > > > > #define instrumentation_begin() ({ \ > > > > asm volatile("%c0:\n\t" \ > > > > > > Why would you be doing noinstr validation for lto builds? That doesn't > > > make sense. > > > > This is just to avoid a ton of noinstr warnings when we run objtool on > > vmlinux.o, but I'm also fine with skipping noinstr validation with LTO. > > Right, then we need to make --no-vmlinux work properly when > !DEBUG_ENTRY, which I think might be buggered due to us overriding the > argument when the objname ends with "vmlinux.o". Right. Can we just remove that and pass --vmlinux to objtool in link-vmlinux.sh, or is the override necessary somewhere else? > > > > +ifdef CONFIG_STACK_VALIDATION > > > > +ifneq ($(SKIP_STACK_VALIDATION),1) > > > > +cmd_ld_ko_o += \ > > > > + $(objtree)/tools/objtool/objtool \ > > > > + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ > > > > + --module \ > > > > + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ > > > > + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ > > > > + $(if $(CONFIG_RETPOLINE),--retpoline,) \ > > > > + $(if $(CONFIG_X86_SMAP),--uaccess,) \ > > > > + $(@:.ko=$(prelink-ext).o); > > > > + > > > > +endif # SKIP_STACK_VALIDATION > > > > +endif # CONFIG_STACK_VALIDATION > > > > > > What about the objtool invocation from link-vmlinux.sh ? > > > > What about it? The existing objtool_link invocation in link-vmlinux.sh > > works fine for our purposes as well. > > Well, I was wondering why you're adding yet another objtool invocation > while we already have one. Because we can't run objtool until we have compiled bitcode to native code, so for modules, we're need another invocation after everything has been compiled. Sami
On Thu, Jun 25, 2020 at 09:22:26AM -0700, Sami Tolvanen wrote: > On Thu, Jun 25, 2020 at 09:47:16AM +0200, Peter Zijlstra wrote: > > Right, then we need to make --no-vmlinux work properly when > > !DEBUG_ENTRY, which I think might be buggered due to us overriding the > > argument when the objname ends with "vmlinux.o". > > Right. Can we just remove that and pass --vmlinux to objtool in > link-vmlinux.sh, or is the override necessary somewhere else? Think we can remove it; it was just convenient when running manually. > > > > > +ifdef CONFIG_STACK_VALIDATION > > > > > +ifneq ($(SKIP_STACK_VALIDATION),1) > > > > > +cmd_ld_ko_o += \ > > > > > + $(objtree)/tools/objtool/objtool \ > > > > > + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ > > > > > + --module \ > > > > > + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ > > > > > + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ > > > > > + $(if $(CONFIG_RETPOLINE),--retpoline,) \ > > > > > + $(if $(CONFIG_X86_SMAP),--uaccess,) \ > > > > > + $(@:.ko=$(prelink-ext).o); > > > > > + > > > > > +endif # SKIP_STACK_VALIDATION > > > > > +endif # CONFIG_STACK_VALIDATION > > > > > > > > What about the objtool invocation from link-vmlinux.sh ? > > > > > > What about it? The existing objtool_link invocation in link-vmlinux.sh > > > works fine for our purposes as well. > > > > Well, I was wondering why you're adding yet another objtool invocation > > while we already have one. > > Because we can't run objtool until we have compiled bitcode to native > code, so for modules, we're need another invocation after everything has > been compiled. Well, that I understand, my question was why we need one in scripts/link-vmlinux.sh and an additional one. I think we're just talking past one another and agree we only need one.
On Thu, Jun 25, 2020 at 08:33:51PM +0200, Peter Zijlstra wrote: > On Thu, Jun 25, 2020 at 09:22:26AM -0700, Sami Tolvanen wrote: > > On Thu, Jun 25, 2020 at 09:47:16AM +0200, Peter Zijlstra wrote: > > > > Right, then we need to make --no-vmlinux work properly when > > > !DEBUG_ENTRY, which I think might be buggered due to us overriding the > > > argument when the objname ends with "vmlinux.o". > > > > Right. Can we just remove that and pass --vmlinux to objtool in > > link-vmlinux.sh, or is the override necessary somewhere else? > > Think we can remove it; it was just convenient when running manually. Great, I'll change this in v2. > > > > > > +ifdef CONFIG_STACK_VALIDATION > > > > > > +ifneq ($(SKIP_STACK_VALIDATION),1) > > > > > > +cmd_ld_ko_o += \ > > > > > > + $(objtree)/tools/objtool/objtool \ > > > > > > + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ > > > > > > + --module \ > > > > > > + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ > > > > > > + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ > > > > > > + $(if $(CONFIG_RETPOLINE),--retpoline,) \ > > > > > > + $(if $(CONFIG_X86_SMAP),--uaccess,) \ > > > > > > + $(@:.ko=$(prelink-ext).o); > > > > > > + > > > > > > +endif # SKIP_STACK_VALIDATION > > > > > > +endif # CONFIG_STACK_VALIDATION > > > > > > > > > > What about the objtool invocation from link-vmlinux.sh ? > > > > > > > > What about it? The existing objtool_link invocation in link-vmlinux.sh > > > > works fine for our purposes as well. > > > > > > Well, I was wondering why you're adding yet another objtool invocation > > > while we already have one. > > > > Because we can't run objtool until we have compiled bitcode to native > > code, so for modules, we're need another invocation after everything has > > been compiled. > > Well, that I understand, my question was why we need one in > scripts/link-vmlinux.sh and an additional one. I think we're just > talking past one another and agree we only need one. We need just one for vmlinux.o, but this rule adds an objtool invocation for kernel modules, which we also couldn't check earlier. We link all the bitcode for modules into <module>.lto.o and run modpost and objtool on that before building the final .ko. Sami
diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 30827f82ad62..12b115152532 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -120,7 +120,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, /* Annotate a C jump table to allow objtool to follow the code flow */ #define __annotate_jump_table __section(.rodata..c_jump_table) -#ifdef CONFIG_DEBUG_ENTRY +#if defined(CONFIG_DEBUG_ENTRY) || defined(CONFIG_LTO_CLANG) /* Begin/end of an instrumentation safe region */ #define instrumentation_begin() ({ \ asm volatile("%c0:\n\t" \ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 9ad9210d70a1..9fdba71c135a 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -399,7 +399,7 @@ config STACK_VALIDATION config VMLINUX_VALIDATION bool - depends on STACK_VALIDATION && DEBUG_ENTRY && !PARAVIRT + depends on STACK_VALIDATION && (DEBUG_ENTRY || LTO_CLANG) && !PARAVIRT default y config DEBUG_FORCE_WEAK_PER_CPU diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 64e99f4baa5b..82977350f5a6 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -226,6 +226,7 @@ endif # CC_USING_PATCHABLE_FUNCTION_ENTRY endif # CONFIG_FTRACE_MCOUNT_RECORD ifdef CONFIG_STACK_VALIDATION +ifndef CONFIG_LTO_CLANG ifneq ($(SKIP_STACK_VALIDATION),1) __objtool_obj := $(objtree)/tools/objtool/objtool @@ -258,6 +259,7 @@ objtool_obj = $(if $(patsubst y%,, \ $(__objtool_obj)) endif # SKIP_STACK_VALIDATION +endif # CONFIG_LTO_CLANG endif # CONFIG_STACK_VALIDATION # Rebuild all objects when objtool changes, or is enabled/disabled. diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal index d168f0cfe67c..9f1df2f1fab5 100644 --- a/scripts/Makefile.modfinal +++ b/scripts/Makefile.modfinal @@ -48,6 +48,21 @@ endif # CC_USING_PATCHABLE_FUNCTION_ENTRY endif # CC_USING_RECORD_MCOUNT endif # CONFIG_FTRACE_MCOUNT_RECORD +ifdef CONFIG_STACK_VALIDATION +ifneq ($(SKIP_STACK_VALIDATION),1) +cmd_ld_ko_o += \ + $(objtree)/tools/objtool/objtool \ + $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \ + --module \ + $(if $(CONFIG_FRAME_POINTER),,--no-fp) \ + $(if $(CONFIG_GCOV_KERNEL),--no-unreachable,) \ + $(if $(CONFIG_RETPOLINE),--retpoline,) \ + $(if $(CONFIG_X86_SMAP),--uaccess,) \ + $(@:.ko=$(prelink-ext).o); + +endif # SKIP_STACK_VALIDATION +endif # CONFIG_STACK_VALIDATION + endif # CONFIG_LTO_CLANG quiet_cmd_ld_ko_o = LD [M] $@
With LTO, LLVM bitcode won't be compiled into native code until modpost_link, or modfinal for modules. This change postpones calls to objtool until after these steps. Signed-off-by: Sami Tolvanen <samitolvanen@google.com> --- include/linux/compiler.h | 2 +- lib/Kconfig.debug | 2 +- scripts/Makefile.build | 2 ++ scripts/Makefile.modfinal | 15 +++++++++++++++ 4 files changed, 19 insertions(+), 2 deletions(-)