@amyk thank you so much!

Could this be merged into main and backported to release/16.x? If this makes 16.0.0 final, I think the kernel can avoid working around this issue altogether, as -mtune was only wired up to do something on PowerPC in during the 16 development cycle; in prior versions, it was ignored so any value was accepted.

Thanks for the fix, it appears to work fine for me against the kernel's powernv_defconfig target.

I applied this change along with the others from https://discourse.llvm.org/t/rfc-syncing-asm-goto-with-outputs-with-gcc/65453/11 and built and boot tested a variety of configurations on the architectures that the Linux kernel cares about and saw no new issues.

I tested this against the kernel and left my feedback above (no issues) so this can be merged.

I’ll have to filter the warnings to see if there are any other instances with other operators that appear problematic.

In D139114#4053166, @nathanchance wrote:

For what it’s worth, this triggers a LOT in the Linux kernel for the pattern that @MaskRay pointed out:

Out of curiosity, has it found any true positives that you can tell?

For what it’s worth, this triggers a LOT in the Linux kernel for the pattern that @MaskRay pointed out:

This does not show any issues with any of the MIPS configurations that I build in the Linux kernel, thanks for fixing the problems I reported!

I bisected a crash I see while building the Linux kernel for arm64 to this change. A reduced C and LLVM IR reproducer:

cvise spits out:

Thanks for the fix! It does not appear to be enough though, I still see some of those errors in my build, although there are far fewer of them:

For what it's worth, this breaks building the Linux kernel's ARCH=mips 32r1_defconfig with clang and GNU as, with thousands of messages along the line of:

In D135402#3913815, @MaskRay wrote:

What's the reproduce instruction? My two-stage build using LLVM_ENABLE_LLD=on works fine: ninja -C /tmp/out/s2-custom lib/libLTO.so.16git lib/libRemarks.so.16git clang

For what's worth, I now see the libLTO failures that @jhuber6 saw above and similar failures in libRemarks when doing a multistage build. I can dig out the cmake commands tomorrow if a standard one does not reproduce it.

I had the same thought as Nick that there should be a test added for the regression. Regardless, I just tested your change against the Linux kernel and everything build successfully, thank you for the quick fix! I do not think I am qualified enough to approve though.

Judging from the time zone of the committer, I suspect this will not get dealt with right away so I have reverted this change in 74bace2dfe57d9cf569addf94af4e01a990d2374 due to the above crash.

Actually, I lied, here is an LLVM IR reproducer that has been passed through llvm-reduce based on whether llc crashed or not.

I bisect a crash while compiling the Linux kernel to this change. A simplified C reproducer (sorry, I do not have time at the moment for llvm-reduce):

I just bisected check-llvm failing to this change. Based on that and the above comment and the fact that it does not look like there is any progress on a forward fix that I can see (presumably due to time zones), I reverted this change in 23c50432fb25775fe0eb958bc8c2a099b0a0c286.

In D134671#3828197, @craig.topper wrote:

In D134671#3824644, @nickdesaulniers wrote:

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

Is there an expectation that we honor -mabi=ms and something that matches gcc? The original bug report I was fixing was that we silently ignored it.

In D134831#3827790, @samitolvanen wrote:

In D134831#3827730, @nickdesaulniers wrote:

Please get the patch disabling this warning for the kernel in flight before landing this.

Agreed. @nathanchance do you want to send the patch above to LKML?

Thanks a lot for fixing this! I took it for a spin against the Linux kernel and all instances of -Wvoid-ptr-dereference disappeared :)

Diff 463063 looks good against the Linux kernel with https://lore.kernel.org/20220925153151.2467884-1-me@inclyc.cn/ applied. I see no additional errors/warnings and the drivers/media/platform/nvidia/tegra-vde/v4l2.c did disappear. Once that patch has been fully chased and accepted, this change should be able to be merged without any problems. Thank you again for taking a look at the kernel ahead of time, it is very much appreciated!

In D133574#3815951, @aaron.ballman wrote:

LGTM, thank you! Please don't land until you have some indication from the kernel folks that this won't be super disruptive for them. CC @nickdesaulniers and @nathanchance for awareness.

In D134461#3815458, @aaron.ballman wrote:

What do folks think of that idea?

This warning is quite noisy for the Linux kernel due to a couple of places where a void * is dereferenced as part of compile time checking.

With D132609, D130127, and D129997 applied, I do not see any crashes when compiling the Linux kernel with a variety of architectures and configurations.

I don't see any issues with my RISC-V Linux kernel builds with latest tip of tree plus this change. Thanks for the fix!

I too saw a crash in the Linux kernel with this change (no KMSAN), my reduction just finished:

In D131270#3743732, @luismarques wrote:

@compnerd is working on a fix for that, D132482. This should probably have been reverted earlier until the fix was ready but I guess it's nearly there now? Do revert if you think that's not the case.

This patch breaks building the Linux kernel for RISC-V. A simplified reproducer:

I can no longer do a two stage build on Fedora after this change.

Thanks a lot for the quick fix. This resolves all of the issues I noticed in my 32-bit ARM Linux kernel build tests. However, I don't think I am qualified enough to approve this.

Sorry I didn't get a chance to report this until after it was merged (my first round of tests forgot to update Linux to fix some unrelated build breakage) but I see a crash in some PowerPC Linux kernel builds as a result of this change in net/ceph/messenger_v2.c. A reduced C reproducer below.

My Linux kernel build and QEMU boot tests (minus i386, due to an unrelated regression in mainline) showed no regressions with this change.

This cleans up the objtool warnings I initially reported and does not introduce any other regressions in my Linux kernel build and QEMU boot tests.

I tested diff 443646 and saw no regressions with my Linux kernel build and QEMU boot tests. I can test the latest revision if that would be useful. I can test bare metal later today.

My set of Linux kernel build and QEMU boot tests did not notice any issues with this change.

I did a set of builds with this patch on mainline and saw no issues.

I bisected an assertion failure while building the Linux kernel for PowerPC to this patch:

I ended up reducing something down anyways. At the parent commit of dc969061c68e62328607d68215ed8b9ef4a1e4b1, there is no crash.

Should dc969061c68e62328607d68215ed8b9ef4a1e4b1 be reverted as well? I just bisected an assertion failure while building the Linux kernel for arm64 to that change:

@amyk Thank you a lot for the fix! I'll pull it down and verify everything is fixed tonight.

I bisected a crash when compiling the Linux kernel to this change.

I see an instance of this warning in the Linux kernel due to the "Now, for unknown directives inside a skipped conditional block, we diagnose the unknown directive as a warning if it is sufficiently similar to a directive specific to preprocessor conditional blocks" part of this change:

I am seeing a failure during a two stage build of LLVM on an AArch64 host that I bisected to this change:

In rGf6dff93641b2259623e686eb13a1884b8b9f4a00#1083750, @aaron.ballman wrote:.

Ooh, thank you for that offer! Out of curiosity, do you know of any pain stemming from the strengthening of diagnostics around implicit function declarations and implicit int, or the changes we made to diagnosing K&R C functions?

In rGf6dff93641b2259623e686eb13a1884b8b9f4a00#1083605, @aaron.ballman wrote:

In rGf6dff93641b2259623e686eb13a1884b8b9f4a00#1083577, @nathanchance wrote:

It will be more disruptive than I initially realized, as the Linux kernel has SPDX markings at the top of every source file, which are required to be prefaced with // for .c files:

Yeah, that's far too disruptive for me to feel comfortable with, so I've reverted this in 1c50909f6f8ac183b82e973457522439a8856e96. Thank you for bringing this to my attention so quickly!

In rGf6dff93641b2259623e686eb13a1884b8b9f4a00#1083567, @aaron.ballman wrote:

In rGf6dff93641b2259623e686eb13a1884b8b9f4a00#1083536, @nathanchance wrote:

This triggers even without -pedantic, is that intentional?

Yes, it is also triggered by -Wcomment in addition to -pedantic

$ echo "// comment" | clang -fsyntax-only -std=gnu89 -Wall -Wextra -x c -
<stdin>:1:1: warning: // comments are not allowed in this language [-Wcomment]
// comment
^
1 warning generated.

$ echo "// comment" | clang -fsyntax-only -std=gnu89 -Wall -Wextra -pedantic -x c -
<stdin>:1:1: warning: // comments are not allowed in this language [-Wcomment]
// comment
^
<stdin>:1:11: warning: ISO C requires a translation unit to contain at least one declaration [-Wempty-translation-unit]
// comment
          ^
2 warnings generated.

vs.

$ echo "// comment" | gcc -fsyntax-only -std=gnu89 -Wall -Wextra -x c -

$ echo "// comment" | gcc -fsyntax-only -std=gnu89 -Wall -Wextra -pedantic -x c -
<stdin>:1:1: warning: C++ style comments are not allowed in ISO C90
<stdin>:1:1: note: (this will be reported only once per input file)
<stdin>:2: warning: ISO C forbids an empty translation unit [-Wpedantic]

This is going to be quite noisy for older versions of the Linux kernel, which still build with -std=gnu89.

-Wcomment is enabled by -Wmost and -Wmost is enabled by -Wall, so from that, yes, I would expect this to trigger.

This triggers even without -pedantic, is that intentional?

I bisected a crash while building the Linux kernel for RISC-V to this change:

I bisected a new crash while building the Linux kernel for arm64 to this change:

It looks like Fangrui already fixed this with f4a3569d0ad61907692fe10b1ffe1fa7763e9c26, thanks a lot! I can confirm that ARCH=arm64 defconfig and allmodconfig build fine now. I will test other architectures tonight and report back if there are any other issues I uncover.

This assertion triggers when building the Linux kernel for arm64:

I have reverted this in 4e0008dcbe9fce99b9727e8bbeb129efc7bf2d80 to hopefully keep our CI from going completely red. I am happy to test a new revision as needed.

This patch causes a crash while building the Linux kernel for arm64. A simplifier C reproducer:

This passes through all my ARCH=arm and ARCH=arm64 build and boot tests on next-20220321 with the instances of -Wdeclaration-after-statement resolved and no new instances of any other warnings.

I can build malta_defconfig on next-20220303 with this patch. I enabled CONFIG_DEBUG_STACKOVERFLOW and CONFIG_HARDENED_USERCOPY, which both use current_stack_pointer, and did not see any warnings/errors.

In D120192#3348461, @RKSimon wrote:

@nathanchance Please can you confirm if rGfadd20f80d69 fixes your build?

This patch causes a failed assertion when building the arm64 Linux kernel.

In D118840#3331575, @ardb wrote:

If the section name '.plt' is special in some way, I'd be happy to prepare a kernel patch that renames these into something more suitable.

I have not done a bisect but it seems likely that this change breaks linking modules for the arm64 Linux kernel, is this expected?

In D119978#3327553, @abrachet wrote:

@nathanchance could you take a look if this fixes the problem from https://github.com/llvm/llvm-project/issues/53391 for you? I was not able to reproduce this error after the workaround

The latest revision passes all of my AArch64 Linux kernel builds with assertions enabled and boots on bare metal, thank you for fixing all the issues!

Thank you for the quick fix! Unfortunately, I still an assertion failure with this patch while building at least allnoconfig and allmodconfig + ThinLTO kernels, albeit a different one. See below and let me know if there are any problems with reproducing:

I have reverted this in 22eb1dae3fb20ca8ada865de1d95baab0e08a060, as it causes assertion failures when building the Linux kernel, which has caused our CI to go red:

In D110869#3302625, @nickdesaulniers wrote:

All built+boot. I know @nathanchance pointed out an issue with some already complex code, but I'm of the opinion that fn should be attributed in kernel sources.

It looks like _paravirt_ident_64() is the problematic function. This diff on top of v5.17-rc2 allows me to boot:

In D110869#3295012, @void wrote:

In D110869#3294696, @nathanchance wrote:

This diff allows me to boot on bare metal as of v5.17-rc2:

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 6aef9ee28a39..8ee176dac669 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_DEBUG_NMI_SELFTEST) += nmi_selftest.o

 obj-$(CONFIG_KVM_GUEST)                += kvm.o kvmclock.o
 obj-$(CONFIG_PARAVIRT)         += paravirt.o
+CFLAGS_paravirt.o += -fzero-call-used-regs=skip
 obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= paravirt-spinlocks.o
 obj-$(CONFIG_PARAVIRT_CLOCK)   += pvclock.o
 obj-$(CONFIG_X86_PMEM_LEGACY_DEVICE) += pmem.o

I have uploaded the config used, the preprocessed file, and the "good" object (with the following diff) and the "bad" object (without the above diff) here: https://github.com/nathanchance/bug-files/tree/052a31e6d94c1b349cf6f3128087944444dace24/D110869

If there is any more information I can give, please let me know!

Thanks for the information!

Could you test this patch for me? (Applied over the patch in this review.)

diff --git a/llvm/lib/CodeGen/PrologEpilogInserter.cpp b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
index 968a14548813..46ae48bd6a3c 100644
--- a/llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -1217,7 +1217,8 @@ void PEI::insertZeroCallUsedRegs(MachineFunction &MF) {
             continue;

           MCRegister Reg = MO.getReg();
-          if ((MO.isDef() || MO.isUse()) && AllocatableSet[Reg])
+          if (AllocatableSet[Reg] && !MO.isImplicit() &&
+              (MO.isDef() || MO.isUse()))
             UsedRegs.set(Reg);
         }

This diff allows me to boot on bare metal as of v5.17-rc2:

The latest revision allows me to boot a kernel in QEMU now but that same kernel does not boot on bare metal. I'll see if I can narrow down the problematic translation unit with Nick's subdir-ccflags-y trick.

As discussed, this is not the right fix for the issue. I have just gone ahead and told the original reporter to use scan-build with the --use-cc=clang flag to avoid these issues.

In D117717#3256293, @MaskRay wrote:

Assuming scan-build is clang/tools/scan-build, which is in tree and can be fixed instead.

In D117717#3256180, @MaskRay wrote:

Clang has a long list of ignored options but most were added during the catch-up phase when Clang strove to be a competitor compiler.
I think for new ignored options there needs to be some higher standard.
From Debian Code Search https://codesearch.debian.net/search?q=fconserve-stack&literal=0&page=2 and internal code search, I believe this is a fairly obscure option.
Shall we fix the projects instead?

In D105169#3246378, @nlopes wrote:

In D105169#3244624, @nathanchance wrote:

I can reduce all of these down for you and/or I can start an email thread with the objtool maintainers to see if there is a way to fix or avoid these warnings on the objtool side and include you in that discussion, if LLVM is not really doing anything wrong here. I am by no means an expert in this area and I don't want to delay this anymore but I want to avoid regressing our builds, as objtool regularly helps us spot compiler bugs. Perhaps @nickdesaulniers has some other thoughts?

LLVM is not doing anything wrong here. The issue is that once we have UB, LLVM is not required to lay out the functions nicely. For example, issue #53118 is just a "nice to fix", it's not a bug.
On the other hand, I understand that fixing objtool is likely impossible if we consider this UB thing as assembly doesn't have enough information to distinguish between a compiler bug and a UB case. I just don't know how frequent and/or useful these warnings are.

I don't think we should hold this patch anymore as it's not wrong and the side-effects are understood. We can and should try to reduce those kernel warnings to zero, but we cannot put all that burden on this patch's author.

In D105169#3242949, @hyeongyukim wrote:

@nathanchance Hi, I analyzed all four warnings.

In D105169#3236484, @hyeongyukim wrote:

@nathanchance

I tried to reproduce the last warning (intelfbhw_validate_mode), but I failed to produce it.
I think my reproducer is correct, but it does not make any warning.
Can you tell me which part was wrong?

clang -O2 -flto=thin -fsanitize=integer-divide-by-zero -c -o intelfb.o intelfb.i
ld.lld -m elf_x86_64 -r -o intelfb.lto.o --whole-archive intelfb.o
objtool orc generate --module --no-fp --no-unreachable --uaccess --mcount intelfb.lto.o

I use these commands, and I attached the intelfb.i file.

intelfb.i2 KBDownload

f62f47f5e1f641b41d3b7d593c058ebec2883534 hides the assertion failure for me.

Just in case it is something different than the Chromium report, I see an assertion failure while building the Linux kernel. Reduced reproducer:

The two false positive warnings in the kernel are fixed with this patch and there were no new warnings introduced.

@hyeongyukim I hand reduced a couple of the translation units that I see issues in so apologies if they are a little verbose.

@hyeongyukim I am currently offline for the evening but it seems like my reduction might have been too aggressive. It looks like this code comes from ravb_set_gti() (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/renesas/ravb_main.c?h=v5.16-rc7#n2480), which checks that rate is not zero before using it as a divisor. I will see if I can get a reproducer without any undefined behavior such as this.