With LTO and LTO cache, .incbin in assembly is not handled properly during incremental builds

[jacky8hyf]

Bug description

To reproduce the error:

• Checkout Linux kernel source tree from Linus Torvald's tree. I am at: 00dcf5d ("Merge tag 'acpi-6.9-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm")
• Get clang prebuilts. I am at: clang-r522817.

Then do the following:

PATH=$PATH:/path/to/clang-r522817/bin/ make ARCH=arm64 LLVM=1 tinyconfig
PATH=$PATH:/path/to/clang-r522817/bin/ make ARCH=arm64 LLVM=1 menuconfig

In menuconfig, enable the following:

• LTO_CLANG_THIN=y: Enables LTO
• IKCONFIG=y: Enables /proc/config.gz
• HEADERS_INSTALL=y: Any config will do. I use this one as an example.

Then:

PATH=$PATH:/path/to/clang-r522817/bin/ make ARCH=arm64 LLVM=1 -j64
zcat kernel/config_data.gz | grep HEADER
scripts/extract-ikconfig vmlinux | grep HEADER

The last two commands will both say CONFIG_HEADERS_INSTALL=y.

Now, run PATH=$PATH:/path/to/clang-r522817/bin/ make ARCH=arm64 LLVM=1 menuconfig again, and disable HEADERS_INSTALL, then build again:

PATH=$PATH:/path/to/clang-r522817/bin/ make ARCH=arm64 LLVM=1 -j64
zcat kernel/config_data.gz | grep HEADER
scripts/extract-ikconfig vmlinux | grep HEADER

The first command will emit # CONFIG_HEADERS_INSTALL is not set correctly, but the last command will still emit CONFIG_HEADERS_INSTALL=y.

Analysis

The config_data.gz file is embedded in vmlinux using a .incbin instruction: https://github.com/torvalds/linux/blob/96fca68c4fbf77a8185eb10f7557e23352732ea2/kernel/configs.c#L28

From the build log and from the output of the above commands, Kbuild correctly updates kernel/config_data.gz, but the change is not reflected in vmlinux, even though it is rebuilt.

LTO (specifically caching) may play a part in this.

• If I disable LTO in the above make menuconfig commands, vmlinux will correctly have # CONFIG_HEADERS_INSTALL is not set.
• In addition, if I delete .thinlto-cache directory (--thinlto-cache-dir ¹) before rebuilding, vmlinux will also correctly have # CONFIG_HEADERS_INSTALL is not set.
• If I apply this workaround, this will link configs.o with -fno-lto, and vmlinux will also correctly have # CONFIG_HEADERS_INSTALL is not set.

From 10eacc104cb39c26a753796c981e46d0cba233c2 Mon Sep 17 00:00:00 2001
From: Yifan Hong <[email protected]>
Date: Mon, 8 Apr 2024 23:49:38 -0700
Subject: [PATCH] config: Disable LTO on kernel/configs.o

Signed-off-by: Yifan Hong <[email protected]>
---
 kernel/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/Makefile b/kernel/Makefile
index 2e0dac3da346..0db1e9357668 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -140,6 +140,10 @@ obj-$(CONFIG_SCF_TORTURE_TEST) += scftorture.o

 $(obj)/configs.o: $(obj)/config_data.gz

+# Disable LTO on configs.o so changes in .config is reflected
+# in vmlinux.
+CFLAGS_REMOVE_configs.o += $(CC_FLAGS_LTO)
+
 targets += config_data config_data.gz
 $(obj)/config_data.gz: $(obj)/config_data FORCE
 	$(call if_changed,gzip)
--
2.44.0.769.g3c40516874-goog

Hence, I believe this is a bug in LTO caching. Please help, thanks!

I am going to upload this workaround patch to Linux upstream, but the root issue should be fixed.

Footnotes

1. https://github.com/torvalds/linux/blob/96fca68c4fbf77a8185eb10f7557e23352732ea2/Makefile#L945 ↩

[nathanchance]

Sounds like #1618?

I wonder if it would be better to drop the ThinLTO caching flags instead? If it impacts correctness, it seems like it is worth avoiding. Thoughts @samitolvanen? Did caching make a huge difference for incremental compiles?

[samitolvanen]

I'm not aware of anyone depending on ThinLTO caching for incremental builds. Android quickly switched back to FullLTO and most incremental builds during development happen with LTO disabled, so I don't have any performance numbers either. I'm fine with dropping the caching.

[nathanchance]

I think I would prefer to go that route. I'll do some benchmarking to see what kind of performance hit we will be taking by removing it. This should obviously still be fixed in LLVM but the rate at which it is fixed will no longer impact the kernel and if people want the cache brought back, it can be made conditional on having a fixed version of LLVM.

[jacky8hyf]

I have sent this patch upstream at https://lore.kernel.org/lkml/[email protected]/T/#u.

For Android, we have disabled LTO by default since 6.1, so this is not an issue for us now. I will do some mitigation on 5.15 by backporting https://lore.kernel.org/lkml/[email protected]/T/#u .

@samitolvanen and @nathanchance , do you mean that the --thinlto-cache-dir option should be dropped from the build command? https://github.com/torvalds/linux/blob/d03d4188908883e1705987795a09aeed31424f66/Makefile#L945

[samitolvanen]

Yes, I think Nathan proposed dropping --thinlto-cache-dir. Just to clarify, do you see this problem also with LTO_CLANG_FULL or only with ThinLTO? Android uses LTO_CLANG_FULL on 5.15, but this flag is only for ThinLTO.

[nathanchance]

@samitolvanen and @nathanchance , do you mean that the --thinlto-cache-dir option should be dropped from the build command? https://github.com/torvalds/linux/blob/d03d4188908883e1705987795a09aeed31424f66/Makefile#L945

Yes, I think that line should just be deleted to resolve this issue. If people notice the removal and want it brought back, we can always revert it and add the workarounds like the change you sent but if the impact to compile time for incremental builds is not noticeable with or without the cache, I would rather just avoid this problem and any other ones that may surface as a result of caching.

[nathanchance]

I did some benchmarking on my test machine that has an i7-11700 (8c / 16t) in it and came to the conclusion that removing the ThinLTO cache would result in about a 20% performance hit (measured with building a full kernel first then just touching init/main.c).

ARCH=x86_64 defconfig + CONFIG_LTO_CLANG_THIN=y:

Benchmark 1: With ThinLTO cache
  Time (mean ± σ):     85.772 s ±  0.252 s    [User: 91.505 s, System: 8.408 s]
  Range (min … max):   85.447 s … 86.244 s    10 runs

Benchmark 2: Without ThinLTO cache
  Time (mean ± σ):     103.833 s ±  0.288 s    [User: 232.058 s, System: 8.569 s]
  Range (min … max):   103.286 s … 104.124 s    10 runs

Summary
  With ThinLTO cache ran
    1.21 ± 0.00 times faster than Without ThinLTO cache

ARCH=x86_64 allmodconfig + CONFIG_LTO_CLANG_THIN=y:

Benchmark 1: With ThinLTO cache
  Time (mean ± σ):     119.537 s ±  0.243 s    [User: 215.822 s, System: 34.958 s]
  Range (min … max):   118.970 s … 119.780 s    10 runs

Benchmark 2: Without ThinLTO cache
  Time (mean ± σ):     145.668 s ±  0.658 s    [User: 429.416 s, System: 35.013 s]
  Range (min … max):   144.787 s … 146.666 s    10 runs

Summary
  With ThinLTO cache ran
    1.22 ± 0.01 times faster than Without ThinLTO cache

I suspect the difference may be less noticeable on more powerful machines. I think this is an acceptable tradeoff for correctness, especially since I would not expect a developer to require ThinLTO for incremental development.

[nathanchance]

I've submitted my suggested workaround: https://lore.kernel.org/20240501-kbuild-llvm-drop-thinlto-cache-v1-1-c117cc50a24b@kernel.org/

[nathanchance]

Workaround patch has been accepted: https://git.kernel.org/masahiroy/linux-kbuild/c/aba091547ef6159d52471f42a3ef531b7b660ed8