| Message ID | 20180918212847.199085-3-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 2018-09-18 23:28, Nadav Amit wrote: > diff --git a/arch/x86/Makefile b/arch/x86/Makefile > index 8f6e7eb8ae9f..944fa3bc9376 100644 > --- a/arch/x86/Makefile > +++ b/arch/x86/Makefile > @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 > KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) > endif > > -# Speed up the build > -KBUILD_CFLAGS += -pipe > +# We cannot use -pipe flag since we give an additional .s file to the compiler > +#KBUILD_CFLAGS += -pipe Is this really necessary? The gas manual says that one can use -- to name stdin, though that's probably a typo and should just be - . Doing gcc -pipe -Wa,foo.s -Wa,- does seem to work as expected (and would also make it possible to append some .s file should that ever be required). > > +archmacros: > + $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s > + > +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s > +export ASM_MACRO_FLAGS > +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) How does this affect what gets rebuilt when one of the asm/foo.h files going into macros.s changes? Does that cause a global rebuild because everything depends on macros.s, or do we still only rebuild the files that actually include asm/foo.h? Rasmus
at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 2018-09-18 23:28, Nadav Amit wrote: > >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile >> index 8f6e7eb8ae9f..944fa3bc9376 100644 >> --- a/arch/x86/Makefile >> +++ b/arch/x86/Makefile >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) >> endif >> >> -# Speed up the build >> -KBUILD_CFLAGS += -pipe >> +# We cannot use -pipe flag since we give an additional .s file to the compiler >> +#KBUILD_CFLAGS += -pipe > > Is this really necessary? The gas manual says that one can use -- to > name stdin, though that's probably a typo and should just be - . Doing > > gcc -pipe -Wa,foo.s -Wa,- Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in v9. > does seem to work as expected (and would also make it possible to append > some .s file should that ever be required). >> +archmacros: >> + $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s >> + >> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s >> +export ASM_MACRO_FLAGS >> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) > How does this affect what gets rebuilt when one of the asm/foo.h files > going into macros.s changes? Does that cause a global rebuild because > everything depends on macros.s, or do we still only rebuild the files > that actually include asm/foo.h? There will not be a global rebuild. Any source file that uses the header files that are included in macros.S will be rebuilt. But your question actually raises two issues: 1. If macros.S changes, there *should* be a global rebuild, and currently there wouldn’t be one. Do you happen to know what would be the appropriate way to trigger one? 2. Someone might mistakenly use the macros through inline assembly without including the header file. It would be better to detect such cases and fail the build. I may be able to create another asm macro in the C part of the header that would cause the build to fail. But let me know if you any better idea. Regards, Nadav
On 2018-09-26 19:56, Nadav Amit wrote: > at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > >>> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s >>> +export ASM_MACRO_FLAGS >>> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) >> How does this affect what gets rebuilt when one of the asm/foo.h files >> going into macros.s changes? Does that cause a global rebuild because >> everything depends on macros.s, or do we still only rebuild the files >> that actually include asm/foo.h? > > There will not be a global rebuild. Any source file that uses the header > files that are included in macros.S will be rebuilt. > > But your question actually raises two issues: > > 1. If macros.S changes, there *should* be a global rebuild, and currently > there wouldn’t be one. Do you happen to know what would be the appropriate > way to trigger one? Hm, that's a question for Kbuild folks, I think. Maybe one could just ensure macros.s gets written into the dependency file for each translation unit (maybe fixdep already has support for that). But if we do that, we probably want macros.s to be generated with some if_changed makefile magic, so that changes in one of the asm/ headers that do not actually change macros.s does not cause a global rebuild (it would still rebuild the files that actually inclue that asm/ header, of course). > 2. Someone might mistakenly use the macros through inline assembly without > including the header file. That's rather unlikely, I think. An open-coded asm("SOME_MACRO ...") would hopefully be caught in review. But yeah... > It would be better to detect such cases and fail > the build. I may be able to create another asm macro in the C part of the > header that would cause the build to fail. But let me know if you any > better idea. The best I can think of (and I'm not suggesting to do this, or that it would actually work): In asm/foo.h, inside its cpp include guard, and inside a guard that excludes this piece when building macros.s itself, have a 'asm("___asm_foo_included__ = 1\n")'. Then inside each asm macro defined in asm/foo.h, have a ".ifndef ___asm_foo_included___\n.error ...\n.endif". That latter part might be macrofied itself so that it's just one line, "CHECK_INCLUDE ___asm_foo_included___\n", with CHECK_INCLUDE being defined in macros.S. Rasmus
Hi. 2018年9月27日(木) 2:57 Nadav Amit <namit@vmware.com>: > > at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > > On 2018-09-18 23:28, Nadav Amit wrote: > > > >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile > >> index 8f6e7eb8ae9f..944fa3bc9376 100644 > >> --- a/arch/x86/Makefile > >> +++ b/arch/x86/Makefile > >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 > >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) > >> endif > >> > >> -# Speed up the build > >> -KBUILD_CFLAGS += -pipe > >> +# We cannot use -pipe flag since we give an additional .s file to the compiler > >> +#KBUILD_CFLAGS += -pipe > > > > Is this really necessary? The gas manual says that one can use -- to > > name stdin, though that's probably a typo and should just be - . Doing > > > > gcc -pipe -Wa,foo.s -Wa,- > > Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in > v9. > > > does seem to work as expected (and would also make it possible to append > > some .s file should that ever be required). > >> +archmacros: > >> + $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s > >> + > >> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s > >> +export ASM_MACRO_FLAGS > >> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) > > How does this affect what gets rebuilt when one of the asm/foo.h files > > going into macros.s changes? Does that cause a global rebuild because > > everything depends on macros.s, or do we still only rebuild the files > > that actually include asm/foo.h? > > There will not be a global rebuild. Any source file that uses the header > files that are included in macros.S will be rebuilt. > > But your question actually raises two issues: > > 1. If macros.S changes, there *should* be a global rebuild, Looking at this series, I can see this rule: "macros and inline functions that calls them are placed in the same header". For example, REFCOUNT_CHECK_LE_ZERO is defined in <asm/refcount.h>, and REFCOUNT_CHECK_LE_ZERO is invoked via refcount_sub_and_test() etc. in the same header. Therefore, all users of REFCOUNT_CHECK_LE_ZERO must have included <asm/refcount.h> This means all C files using REFCOUNT_CHECK_LE_ZERO will be appropriately recompiled as long as the rule above is observed. > and currently > there wouldn’t be one. Do you happen to know what would be the appropriate > way to trigger one? > > 2. Someone might mistakenly use the macros through inline assembly without > including the header file. Right, it is possible to use REFCOUNT_CHECK_LE_ZERO directly. If this happens, my assumption breaks. It would be unlikely to happen, though... > It would be better to detect such cases and fail > the build. I may be able to create another asm macro in the C part of the > header that would cause the build to fail. But let me know if you any > better idea. > > Regards, > Nadav >
* Nadav Amit <namit@vmware.com> wrote: > at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > > On 2018-09-18 23:28, Nadav Amit wrote: > > > >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile > >> index 8f6e7eb8ae9f..944fa3bc9376 100644 > >> --- a/arch/x86/Makefile > >> +++ b/arch/x86/Makefile > >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 > >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) > >> endif > >> > >> -# Speed up the build > >> -KBUILD_CFLAGS += -pipe > >> +# We cannot use -pipe flag since we give an additional .s file to the compiler > >> +#KBUILD_CFLAGS += -pipe > > > > Is this really necessary? The gas manual says that one can use -- to > > name stdin, though that's probably a typo and should just be - . Doing > > > > gcc -pipe -Wa,foo.s -Wa,- > > Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in > v9. Ok, will wait for v9. Thanks, Ingo
at 3:01 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi. Thanks for your reply. Your advices are always valuable! > > > > 2018年9月27日(木) 2:57 Nadav Amit <namit@vmware.com>: >> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: >> >>> On 2018-09-18 23:28, Nadav Amit wrote: >>> >>>> diff --git a/arch/x86/Makefile b/arch/x86/Makefile >>>> index 8f6e7eb8ae9f..944fa3bc9376 100644 >>>> --- a/arch/x86/Makefile >>>> +++ b/arch/x86/Makefile >>>> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 >>>> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) >>>> endif >>>> >>>> -# Speed up the build >>>> -KBUILD_CFLAGS += -pipe >>>> +# We cannot use -pipe flag since we give an additional .s file to the compiler >>>> +#KBUILD_CFLAGS += -pipe >>> >>> Is this really necessary? The gas manual says that one can use -- to >>> name stdin, though that's probably a typo and should just be - . Doing >>> >>> gcc -pipe -Wa,foo.s -Wa,- >> >> Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in >> v9. >> >>> does seem to work as expected (and would also make it possible to append >>> some .s file should that ever be required). >>>> +archmacros: >>>> + $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s >>>> + >>>> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s >>>> +export ASM_MACRO_FLAGS >>>> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) >>> How does this affect what gets rebuilt when one of the asm/foo.h files >>> going into macros.s changes? Does that cause a global rebuild because >>> everything depends on macros.s, or do we still only rebuild the files >>> that actually include asm/foo.h? >> >> There will not be a global rebuild. Any source file that uses the header >> files that are included in macros.S will be rebuilt. >> >> But your question actually raises two issues: >> >> 1. If macros.S changes, there *should* be a global rebuild, > > > Looking at this series, I can see this rule: > "macros and inline functions that calls them are placed in the same header". > > > For example, > REFCOUNT_CHECK_LE_ZERO is defined in <asm/refcount.h>, > and REFCOUNT_CHECK_LE_ZERO is invoked via refcount_sub_and_test() etc. > in the same header. > > Therefore, all users of REFCOUNT_CHECK_LE_ZERO must have included > <asm/refcount.h> > > This means all C files using REFCOUNT_CHECK_LE_ZERO > will be appropriately recompiled as long as the rule above is observed. Yes. My concern is not what happens if any of the files included from macros.S changes, but what happens if macros.S changes (e.g., to include an additional header). Maybe I can create some artificial dependency based on __ASSEMBLY__ . I’ll give it another thought. >> and currently >> there wouldn’t be one. Do you happen to know what would be the appropriate >> way to trigger one? >> >> 2. Someone might mistakenly use the macros through inline assembly without >> including the header file. > > Right, it is possible to use REFCOUNT_CHECK_LE_ZERO directly. > If this happens, my assumption breaks. > > It would be unlikely to happen, though... Yes. I’ll let it go for now. Thanks, Nadav
On October 2, 2018 3:59:52 AM PDT, Ingo Molnar <mingo@kernel.org> wrote: > >* Nadav Amit <namit@vmware.com> wrote: > >> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: >> >> > On 2018-09-18 23:28, Nadav Amit wrote: >> > >> >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile >> >> index 8f6e7eb8ae9f..944fa3bc9376 100644 >> >> --- a/arch/x86/Makefile >> >> +++ b/arch/x86/Makefile >> >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 >> >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) >> >> endif >> >> >> >> -# Speed up the build >> >> -KBUILD_CFLAGS += -pipe >> >> +# We cannot use -pipe flag since we give an additional .s file to >the compiler >> >> +#KBUILD_CFLAGS += -pipe >> > >> > Is this really necessary? The gas manual says that one can use -- >to >> > name stdin, though that's probably a typo and should just be - . >Doing >> > >> > gcc -pipe -Wa,foo.s -Wa,- >> >> Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do >it in >> v9. > >Ok, will wait for v9. > >Thanks, > > Ingo Does -pipe actually win anything these days?
at 12:43 PM, hpa@zytor.com wrote: > > Does -pipe actually win anything these days? Sorry, I don’t have the time to check it right now. Anyhow, I presume it is best that such a change will be in a separate patch.
diff --git a/Makefile b/Makefile index f03a1e062503..af5dba613533 100644 --- a/Makefile +++ b/Makefile @@ -1071,7 +1071,7 @@ scripts: scripts_basic asm-generic gcc-plugins $(autoksyms_h) # version.h and scripts_basic is processed / created. # Listed in dependency order -PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3 +PHONY += prepare archprepare macroprepare prepare0 prepare1 prepare2 prepare3 # prepare3 is used to check if we are building in a separate output directory, # and if so do: @@ -1094,7 +1094,9 @@ prepare2: prepare3 outputmakefile asm-generic prepare1: prepare2 $(version_h) $(autoksyms_h) include/generated/utsrelease.h $(cmd_crmodverdir) -archprepare: archheaders archscripts prepare1 scripts_basic +macroprepare: prepare1 archmacros + +archprepare: archheaders archscripts macroprepare scripts_basic prepare0: archprepare gcc-plugins $(Q)$(MAKE) $(build)=. @@ -1162,6 +1164,9 @@ archheaders: PHONY += archscripts archscripts: +PHONY += archmacros +archmacros: + PHONY += __headers __headers: $(version_h) scripts_basic uapi-asm-generic archheaders archscripts $(Q)$(MAKE) $(build)=scripts build_unifdef diff --git a/arch/x86/Makefile b/arch/x86/Makefile index 8f6e7eb8ae9f..944fa3bc9376 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64 KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000) endif -# Speed up the build -KBUILD_CFLAGS += -pipe +# We cannot use -pipe flag since we give an additional .s file to the compiler +#KBUILD_CFLAGS += -pipe # Workaround for a gcc prelease that unfortunately was shipped in a suse release KBUILD_CFLAGS += -Wno-sign-compare # @@ -237,6 +237,13 @@ archscripts: scripts_basic archheaders: $(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all +archmacros: + $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s + +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s +export ASM_MACRO_FLAGS +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) + ### # Kernel objects diff --git a/arch/x86/kernel/macros.S b/arch/x86/kernel/macros.S new file mode 100644 index 000000000000..cfc1c7d1a6eb --- /dev/null +++ b/arch/x86/kernel/macros.S @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* + * This file includes headers whose assembly part includes macros which are + * commonly used. The macros are precompiled into assmebly file which is later + * assembled together with each compiled file. + */ diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index ce53639a864a..8aeb60eb6ee3 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -115,7 +115,9 @@ __cc-option = $(call try-run,\ # Do not attempt to build with gcc plugins during cc-option tests. # (And this uses delayed resolution so the flags will be up to date.) -CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS)) +# In addition, do not include the asm macros which are built later. +CC_OPTION_FILTERED = $(GCC_PLUGINS_CFLAGS) $(ASM_MACRO_FLAGS) +CC_OPTION_CFLAGS = $(filter-out $(CC_OPTION_FILTERED),$(KBUILD_CFLAGS)) # cc-option # Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586) diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile index 42c5d50f2bcc..a5b4af47987a 100644 --- a/scripts/mod/Makefile +++ b/scripts/mod/Makefile @@ -4,6 +4,8 @@ OBJECT_FILES_NON_STANDARD := y hostprogs-y := modpost mk_elfconfig always := $(hostprogs-y) empty.o +CFLAGS_REMOVE_empty.o := $(ASM_MACRO_FLAGS) + modpost-objs := modpost.o file2alias.o sumversion.o devicetable-offsets-file := devicetable-offsets.h