LinuxLists.cc - [PATCH v3 0/7] kbuild: various cleanups

2022-09-24 18:21:53

Hi Masahiro,

I love your patch! Perhaps something to improve:

[auto build test WARNING on masahiroy-kbuild/for-next]
[cannot apply to arm64/for-next/core gerg-m68knommu/for-next geert-m68k/for-next deller-parisc/for-next powerpc/next s390/features tip/x86/core linus/master v6.0-rc6 next-20220923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Masahiro-Yamada/kbuild-various-cleanups/20220925-022150
base: https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
config: um-i386_defconfig
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/d721cc5614aaa17b2965db04e9319d4ef5f7eaf7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Masahiro-Yamada/kbuild-various-cleanups/20220925-022150
git checkout d721cc5614aaa17b2965db04e9319d4ef5f7eaf7
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> /usr/bin/ld: warning: ./arch/x86/um/checksum_32.o: missing .note.GNU-stack section implies executable stack
/usr/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
/usr/bin/ld: warning: .tmp_vmlinux.kallsyms1 has a LOAD segment with RWX permissions
/usr/bin/ld: warning: .tmp_vmlinux.kallsyms1.o: missing .note.GNU-stack section implies executable stack
/usr/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
/usr/bin/ld: warning: .tmp_vmlinux.kallsyms2 has a LOAD segment with RWX permissions
/usr/bin/ld: warning: .tmp_vmlinux.kallsyms2.o: missing .note.GNU-stack section implies executable stack
/usr/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
/usr/bin/ld: warning: vmlinux has a LOAD segment with RWX permissions

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (2.38 kB)
config (42.02 kB)
Download all attachments

2022-09-24 20:39:28

On Thu, Sep 29, 2022 at 4:59 AM Nicolas Schier <[email protected]> wrote:
>
> On Sun, 25 Sep 2022 03:19:13 +0900 Masahiro Yamada wrote:
> > Currently, modpost is executed twice; first for vmlinux, second
> > for modules.
> >
> > This commit merges them.
> >
> > Current build flow
> > ==================
> >
> > 1) build obj-y and obj-m objects
> > 2) link vmlinux.o
> > 3) modpost for vmlinux
> > 4) link vmlinux
> > 5) modpost for modules
> > 6) link modules (*.ko)
> >
> > The build steps 1) through 6) are serialized, that is, modules are
> > built after vmlinux. You do not get benefits of parallel builds when
> > scripts/link-vmlinux.sh is being run.
> >
> > New build flow
> > ==============
> >
> > 1) build obj-y and obj-m objects
> > 2) link vmlinux.o
> > 3) modpost for vmlinux and modules
> > 4a) link vmlinux
> > 4b) link modules (*.ko)
> >
> > In the new build flow, modpost is invoked just once.
> >
> > vmlinux and modules are built in parallel. One exception is
> > CONFIG_DEBUG_INFO_BTF_MODULES=y, where modules depend on vmlinux.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > (no changes since v1)
> >
> > Makefile | 30 ++++++++++---
> > scripts/Makefile.modfinal | 2 +-
> > scripts/Makefile.modpost | 93 ++++++++++++---------------------------
> > scripts/link-vmlinux.sh | 3 --
> > 4 files changed, 53 insertions(+), 75 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index b5dfb54b1993..cf9d7b1d8c14 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1152,7 +1152,7 @@ cmd_link-vmlinux = \
> > $(CONFIG_SHELL) $< "$(LD)" "$(KBUILD_LDFLAGS)" "$(LDFLAGS_vmlinux)"; \
> > $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
> >
> > -vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
> > +vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) modpost FORCE
> > +$(call if_changed_dep,link-vmlinux)
> >
> > targets := vmlinux
> > @@ -1428,7 +1428,13 @@ endif
> > # Build modules
> > #
> >
> > -modules: $(if $(KBUILD_BUILTIN),vmlinux) modules_prepare
> > +# *.ko are usually independent of vmlinux, but CONFIG_DEBUG_INFOBTF_MODULES
> > +# is an exception.
> > +ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> > +modules: vmlinux
> > +endif
> > +
> > +modules: modules_prepare
> >
> > # Target to prepare building external modules
> > modules_prepare: prepare
> > @@ -1741,8 +1747,12 @@ ifdef CONFIG_MODULES
> > $(MODORDER): $(build-dir)
> > @:
> >
> > -modules: modules_check
> > - $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
> > +# KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
> > +# This is solely useful to speed up test compiles.
> > +modules: modpost
> > +ifneq ($(KBUILD_MODPOST_NOFINAL),1)
> > + $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modfinal
> > +endif
> >
> > PHONY += modules_check
> > modules_check: $(MODORDER)
> > @@ -1773,6 +1783,11 @@ KBUILD_MODULES :=
> >
> > endif # CONFIG_MODULES
> >
> > +PHONY += modpost
> > +modpost: $(if $(single-build),, $(if $(KBUILD_BUILTIN), vmlinux.o)) \
> > + $(if $(KBUILD_MODULES), modules_check)
> > + $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
> > +
> > # Single targets
> > # ---------------------------------------------------------------------------
> > # To build individual files in subdirectories, you can do like this:
> > @@ -1792,16 +1807,19 @@ single-ko := $(sort $(filter %.ko, $(MAKECMDGOALS)))
> > single-no-ko := $(filter-out $(single-ko), $(MAKECMDGOALS)) \
> > $(foreach x, o mod, $(patsubst %.ko, %.$x, $(single-ko)))
> >
> > -$(single-ko): single_modpost
> > +$(single-ko): single_modules
> > @:
> > $(single-no-ko): $(build-dir)
> > @:
> >
> > # Remove MODORDER when done because it is not the real one.
> > PHONY += single_modpost
>
> PHONY += single_modules

Thank you for your close review!
I will fix it locally.

>
> Reviewed-by: Nicolas Schier <[email protected]>
>

--
Best Regards
Masahiro Yamada

2022-09-29 15:24:34

Hi,

following an IRC discussion with many parties...

On 24. 09. 22, 20:19, Masahiro Yamada wrote:
> The objects placed at the head of vmlinux need special treatments:
>
> - arch/$(SRCARCH)/Makefile adds them to head-y in order to place
> them before other archives in the linker command line.
>
> - arch/$(SRCARCH)/kernel/Makefile adds them to extra-y instead of
> obj-y to avoid them going into built-in.a.
>
> This commit gets rid of the latter.
>
> Create vmlinux.a to collect all the objects that are unconditionally
> linked to vmlinux. The objects listed in head-y are moved to the head
> of vmlinux.a by using 'ar m'.
>
> With this, arch/$(SRCARCH)/kernel/Makefile can consistently use obj-y
> for builtin objects.
>
> There is no *.o that is directly linked to vmlinux. Drop unneeded code
> in scripts/clang-tools/gen_compile_commands.py.
>
> $(AR) mPi needs 'T' to workaround the llvm-ar bug. The fix was suggested
> by Nathan Chancellor [1].
>
> [1]: https://lore.kernel.org/llvm/[email protected]/
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
...
> --- a/scripts/Makefile.vmlinux_o
> +++ b/scripts/Makefile.vmlinux_o
> @@ -18,7 +18,7 @@ quiet_cmd_gen_initcalls_lds = GEN $@
> $(PERL) $(real-prereqs) > $@
>
> .tmp_initcalls.lds: $(srctree)/scripts/generate_initcall_order.pl \
> - $(KBUILD_VMLINUX_OBJS) $(KBUILD_VMLINUX_LIBS) FORCE
> + vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE

There is a slight problem with this. The kernel built with gcc-LTO does
not boot. But as I understand it, it's not limited to gcc-LTO only.

On x86, startup_64() is supposed to be at offset >zero< of the image
(see .Lrelocated()). It was ensured by putting head64.o to the beginning
of vmlinux (by KBUILD_VMLINUX_OBJS on the LD command-line above). The
patch above instead packs head64.o into vmlinux.a and then moves it
using "ar -m" to the beginning (it's in 7/7 of the series IIRC).

The problem is that .o files listed on the LD command line explicitly
are taken as spelled. But unpacking .a inside LD gives no guarantees on
the order of packed objects. To quote: "that it happens to work
sometimes is pure luck." (Correct me guys, if I misunderstood you.)

For x86, the most ideal fix seems to be to fix it in the linker script.
By putting startup_64() to a different section and handle it in the ld
script specially -- see the attachment. It should always have been put
this way, the command line order is only a workaround. But this might
need more fixes on other archs too -- I haven't take a look.

Ideas, comments? I'll send the attachment as a PATCH later (if there are
no better suggestions).

thanks,
--
js
suse labs

Attachments:

0001-head-fix.patch (1.69 kB)

2022-10-25 13:07:58

by Michael Matz

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] kbuild: use obj-y instead extra-y for objects placed at the head

Hello,

On Mon, 24 Oct 2022, Jiri Slaby wrote:

> > Create vmlinux.a to collect all the objects that are unconditionally
> > linked to vmlinux. The objects listed in head-y are moved to the head
> > of vmlinux.a by using 'ar m'.
...
> > --- a/scripts/Makefile.vmlinux_o
> > +++ b/scripts/Makefile.vmlinux_o
> > @@ -18,7 +18,7 @@ quiet_cmd_gen_initcalls_lds = GEN $@
> > $(PERL) $(real-prereqs) > $@
> > .tmp_initcalls.lds: $(srctree)/scripts/generate_initcall_order.pl \
> > - $(KBUILD_VMLINUX_OBJS) $(KBUILD_VMLINUX_LIBS) FORCE
> > + vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
>
> There is a slight problem with this. The kernel built with gcc-LTO does not
> boot. But as I understand it, it's not limited to gcc-LTO only.
>
> On x86, startup_64() is supposed to be at offset >zero< of the image (see
> .Lrelocated()). It was ensured by putting head64.o to the beginning of vmlinux
> (by KBUILD_VMLINUX_OBJS on the LD command-line above). The patch above instead
> packs head64.o into vmlinux.a and then moves it using "ar -m" to the beginning
> (it's in 7/7 of the series IIRC).
>
> The problem is that .o files listed on the LD command line explicitly are
> taken as spelled. But unpacking .a inside LD gives no guarantees on the order
> of packed objects. To quote: "that it happens to work sometimes is pure luck."
> (Correct me guys, if I misunderstood you.)

To be precise: I know of no linker (outside LTO-like modes) that processes
archives in a different order than first-to-last-member (under
whole-archive), but that's not guaranteed anywhere. So relying on
member-order within archives is always brittle.

It will completely break down with LTO modes: the granularity for that is
functions, and they are placed in some unknown (from the outside, but
usually related to call-graph locality) order into several partitions,
with non-LTO-able parts (like asm code) being placed randomly between
them. The order of these blobs can not be defined in relation to the
input order of object files: with cross-file dependencies such order might
not even exist. Those whole sequence of blobs then takes the place of the
input archive (which, as there was only one, has no particular order from
the linker command lines perspective).

There are only two ways of guaranteeing an ordering: put non-LTO-.o files
at certain places of the link command, or, better, use a linker script to
specify an order.

> For x86, the most ideal fix seems to be to fix it in the linker script. By
> putting startup_64() to a different section and handle it in the ld script
> specially -- see the attachment. It should always have been put this way, the
> command line order is only a workaround. But this might need more fixes on
> other archs too -- I haven't take a look.
>
> Ideas, comments? I'll send the attachment as a PATCH later (if there are
> no better suggestions).

This will work. An alternative way would be to explicitely name the input
file in the section commands, without renaming the section:

@@ -126,6 +126,7 @@ SECTIONS
_text = .;
_stext = .;
/* bootstrapping code */
+ KEEP(vmlinux.a:head64.o(.head.text))
HEAD_TEXT
TEXT_TEXT

But I guess not all arch's name their must-be-first file head64.o (or even
have such requirement), so that's probably still arch-dependend and hence
not inherently better than your way.

(syntax for the section selector in linkerscripts is:

{archive-glob:}filename-glob (sectionname-glob)

Ciao,
Michael.

2022-10-26 08:42:37

by Jiri Slaby

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] kbuild: use obj-y instead extra-y for objects placed at the head

On 25. 10. 22, 14:26, Michael Matz wrote:
>> Ideas, comments? I'll send the attachment as a PATCH later (if there are
>> no better suggestions).
>
> This will work. An alternative way would be to explicitly name the input
> file in the section commands, without renaming the section:
>
> @@ -126,6 +126,7 @@ SECTIONS
> _text = .;
> _stext = .;
> /* bootstrapping code */
> + KEEP(vmlinux.a:head64.o(.head.text))
> HEAD_TEXT
> TEXT_TEXT
>
> But I guess not all arch's name their must-be-first file head64.o (or even
> have such requirement), so that's probably still arch-dependend and hence
> not inherently better than your way.

The downside of this is that it doesn't make sure the function
(startup_64()) is the first one. When someone sticks something before
it, it breaks again. But leaving the decision up to the x86 maintainers ;).

Re. other archs, I have absolutely no idea (haven't looked into that at
all).

thanks,
--
js

2022-10-26 11:40:20

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] kbuild: use obj-y instead extra-y for objects placed at the head

On Wed, 26 Oct 2022 at 10:35, Jiri Slaby <[email protected]> wrote:
>
> On 25. 10. 22, 14:26, Michael Matz wrote:
> >> Ideas, comments? I'll send the attachment as a PATCH later (if there are
> >> no better suggestions).
> >
> > This will work. An alternative way would be to explicitly name the input
> > file in the section commands, without renaming the section:
> >
> > @@ -126,6 +126,7 @@ SECTIONS
> > _text = .;
> > _stext = .;
> > /* bootstrapping code */
> > + KEEP(vmlinux.a:head64.o(.head.text))
> > HEAD_TEXT
> > TEXT_TEXT
> >
> > But I guess not all arch's name their must-be-first file head64.o (or even
> > have such requirement), so that's probably still arch-dependend and hence
> > not inherently better than your way.
>
> The downside of this is that it doesn't make sure the function
> (startup_64()) is the first one. When someone sticks something before
> it, it breaks again. But leaving the decision up to the x86 maintainers ;).
>
> Re. other archs, I have absolutely no idea (haven't looked into that at
> all).
>

I seriously doubt that those __head routines need to be in .head.text.
At the time, there seems to have been some regression related to
5-level paging, but whether and how that change actually fixed
anything at all is undocumented.

So the fix here is to move those __head routines into __init, so that
only startup_64 remains in .head.text, and the existing linker script
will put it where it belongs.

commit 26179670a68b7b365fbfe38afb043dcd2e1a4678
Author: Kirill A. Shutemov <[email protected]>
Date: Fri Jun 16 14:30:24 2017 +0300

x86/boot/64: Put __startup_64() into .head.text

Put __startup_64() and fixup_pointer() into .head.text section to make
sure it's always near startup_64() and always callable.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: kernel test robot <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>

2022-10-26 17:19:39

by Masahiro Yamada

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] kbuild: use obj-y instead extra-y for objects placed at the head

On Tue, Oct 25, 2022 at 2:26 PM Michael Matz <[email protected]> wrote:
>
> Hello,
>
> On Mon, 24 Oct 2022, Jiri Slaby wrote:
>
> > > Create vmlinux.a to collect all the objects that are unconditionally
> > > linked to vmlinux. The objects listed in head-y are moved to the head
> > > of vmlinux.a by using 'ar m'.
> ...
> > > --- a/scripts/Makefile.vmlinux_o
> > > +++ b/scripts/Makefile.vmlinux_o
> > > @@ -18,7 +18,7 @@ quiet_cmd_gen_initcalls_lds = GEN $@
> > > $(PERL) $(real-prereqs) > $@
> > > .tmp_initcalls.lds: $(srctree)/scripts/generate_initcall_order.pl \
> > > - $(KBUILD_VMLINUX_OBJS) $(KBUILD_VMLINUX_LIBS) FORCE
> > > + vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
> >
> > There is a slight problem with this. The kernel built with gcc-LTO does not
> > boot. But as I understand it, it's not limited to gcc-LTO only.
> >
> > On x86, startup_64() is supposed to be at offset >zero< of the image (see
> > .Lrelocated()). It was ensured by putting head64.o to the beginning of vmlinux
> > (by KBUILD_VMLINUX_OBJS on the LD command-line above). The patch above instead
> > packs head64.o into vmlinux.a and then moves it using "ar -m" to the beginning
> > (it's in 7/7 of the series IIRC).
> >
> > The problem is that .o files listed on the LD command line explicitly are
> > taken as spelled. But unpacking .a inside LD gives no guarantees on the order
> > of packed objects. To quote: "that it happens to work sometimes is pure luck."
> > (Correct me guys, if I misunderstood you.)
>
> To be precise: I know of no linker (outside LTO-like modes) that processes
> archives in a different order than first-to-last-member (under
> whole-archive), but that's not guaranteed anywhere. So relying on
> member-order within archives is always brittle.

The objects in an archive are linked first-to-last-member for a long time.
This is the assumption which we have relied on for a long time.

We assume the initcall order is preserved.
The call order within each of core_initcall, arch_initcall,
device_initcall, etc.
is the order of objects in built-in.a, in other words,
the order they appear in Makefiles.

If this assumption were broken, the initcall order would be randomised.
(Somebody would have screamed earlier, if so.)

Clang LTO came up with its own workaround.
See commit a8cccdd954732a558d481407ab7c3106b89c34ae

So, this is happening on (not-upstreamed-yet) GCC LTO only?

> It will completely break down with LTO modes: the granularity for that is
> functions, and they are placed in some unknown (from the outside, but
> usually related to call-graph locality) order into several partitions,
> with non-LTO-able parts (like asm code) being placed randomly between
> them. The order of these blobs can not be defined in relation to the
> input order of object files: with cross-file dependencies such order might
> not even exist. Those whole sequence of blobs then takes the place of the
> input archive (which, as there was only one, has no particular order from
> the linker command lines perspective).
>
> There are only two ways of guaranteeing an ordering: put non-LTO-.o files
> at certain places of the link command, or, better, use a linker script to
> specify an order.

The objects directly given in the command line are linked in the same order,
even under LTO mode. Is this what you mean?

Any documentation about that?

> > For x86, the most ideal fix seems to be to fix it in the linker script. By
> > putting startup_64() to a different section and handle it in the ld script
> > specially -- see the attachment. It should always have been put this way, the
> > command line order is only a workaround. But this might need more fixes on
> > other archs too -- I haven't take a look.
> >
> > Ideas, comments? I'll send the attachment as a PATCH later (if there are
> > no better suggestions).
>
> This will work. An alternative way would be to explicitely name the input
> file in the section commands, without renaming the section:
>
> @@ -126,6 +126,7 @@ SECTIONS
> _text = .;
> _stext = .;
> /* bootstrapping code */
> + KEEP(vmlinux.a:head64.o(.head.text))
> HEAD_TEXT
> TEXT_TEXT
>
> But I guess not all arch's name their must-be-first file head64.o (or even
> have such requirement), so that's probably still arch-dependend and hence
> not inherently better than your way.
>
> (syntax for the section selector in linkerscripts is:
>
> {archive-glob:}filename-glob (sectionname-glob)
>
>
> Ciao,
> Michael.

--
Best Regards

Masahiro Yamada

2022-10-26 17:46:41

by Michael Matz

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] kbuild: use obj-y instead extra-y for objects placed at the head

Hello,

On Thu, 27 Oct 2022, Masahiro Yamada wrote:

> > To be precise: I know of no linker (outside LTO-like modes) that processes
> > archives in a different order than first-to-last-member (under
> > whole-archive), but that's not guaranteed anywhere. So relying on
> > member-order within archives is always brittle.
>
> The objects in an archive are linked first-to-last-member for a long time.
> This is the assumption which we have relied on for a long time.

Sure, that doesn't mean it's guaranteed, for this I'm just devils
advocate. As I said, it's the mode of operation of all currently existing
linkers I know, so for the forseeable future you can continue to rely on
it. But as soon as LTO enters the picture that all breaks down, as you
see here.

Consider how LTO works, the granularity of shuffling stuff around is the
functions of all inputs, not the object files. So, even if you say
obj1.o obj2.o
on the link command line, and supposed there are two functions in each of
obj1 and obj2, then it may just so happen that LTO partitions the program
such that it ends up with partitions
part1 : obj1:foo obj2:bar
part2 : obj1:bar obj2:foo
now, suddenly there is no order between part1 and part2 anymore that
would faithfully represent the original order between obj1 and obj2
functions. Of course the partitioning algorithm could be changed, but
that would limit the effectiveness of LTO.

> We assume the initcall order is preserved.
> The call order within each of core_initcall, arch_initcall,
> device_initcall, etc.
> is the order of objects in built-in.a, in other words,
> the order they appear in Makefiles.

If you rely on relative order of these, then you will probably see
interesting effects in LTO mode eventually.

> So, this is happening on (not-upstreamed-yet) GCC LTO only?

I don't know. As any kind of whole-program transformations is in
principle cross-file on per-function basis (that's the whole purpose) I
would imagine that you can see these effects in all compilers, if you try
hard enough.

> > There are only two ways of guaranteeing an ordering: put non-LTO-.o files
> > at certain places of the link command, or, better, use a linker script to
> > specify an order.
>
> The objects directly given in the command line are linked in the same order,
> even under LTO mode. Is this what you mean?

If they don't contain LTO code then yes, they are linked
in exactly the given order. If they do, they become part of the LTO blob
whose ordering isn't precisely influenced by cmdline order.

> Any documentation about that?

I think so, but I can't point my finger to anything. Several parts of the
toolchain rely on that (so the kernel is not alone), but those are either
carefully avoiding LTO or using other ordering means like linker script to
achieve what they need.

Ciao,
Michael.