In D88834#2323565, @RKSimon wrote:

@nathanchance Please can you confirm rG2c3e4a21f93d fixes the issue?

This patch breaks building the PowerPC Linux kernel with clang.

Please ensure that this patch gets picked up into release/11.x as the patch that introduced this failure is in there.

With this diff applied on top of f3731d34faa7432462c877714af235e9787c9b30, this looks good to me.

I will throw this through my build tests for the Linux kernel and make sure that it passes (will probably take my machine four hours or so). I do not feel qualified enough to approve this once I get those results though.

This commits causes a failed assertion when building the Linux kernel for arm32:

This patch breaks compiling the Linux kernel:

@nemanjai thanks for the fix, I can confirm all of my PowerPC builds are successful with LLVM at 4772b99dffec4f87bb7bc9273495066058ac0186!

Gentle ping. This is still an issue at 4c6548222b3c41d024581d28f42b3f02510bcfe3 and I have not heard anything about a fix. I know I sent that message on Friday night before the weekend so I am hoping that it did not get lost. If I need to file a bug report, I am more than happy to do so.

@void that diff on top of this revision resolves the issue I was seeing, thanks!

In D79794#2104506, @nickdesaulniers wrote:

@void 's additional diff fixes the arm64 mainline boot issue I was observing. Maybe @nathanchance can help test with that additional diff applied atop this patch that we're in good shape?

Hi! This patch causes a crash when compiling the Linux kernel with certain distribution configs:

I do not know enough about KASAN enough to review this patch but I can say that against mainline at https://git.kernel.org/linus/af7b4801030c07637840191c69eb666917e4135d, there appear to be no visible regressions with this version of the patch on top of caa2fddce72f2e8ca3d6346cc2c8fe85252b91d8.

This patch causes a crash in the Linux kernel when building s390 allyesconfig:

Thanks for the fix! I can confirm that a multi_v5_defconfig kernel looks normal size wise and boots in QEMU. I will do a set of kernel builds overnight to make sure that nothing else regresses.

Thanks! I can confirm that I do not see a crash anymore with the config I used when initially reporting. I will try to run this patch through all of my kernel build tests this afternoon to make sure nothing else has regressed.

In D77330#1994737, @psmith wrote:

In D77330#1994680, @nickdesaulniers wrote:

Hmm...seems to have regressed ARMv5 Linux kernel boots according to a bisection from @nathanchance (https://groups.google.com/g/clang-built-linux/c/aKDK-N6JN4g)

I don't think that URL is publicly visible.

Just as an FYI, this fixes the issue I reported here: https://bugs.llvm.org/show_bug.cgi?id=45580

In D73664#1975685, @nemanjai wrote:

In D73664#1975228, @nathanchance wrote:

Awesome. Thank you. I'll work on getting that fixed. To verify everything, I just replace modules in the invocation with all?

Correct!

Hi Nathan, I pushed 04eae396176ccebe359bbb47ca1458a6377bbc4f after building all successfully. When you get a chance, could you please confirm that it is back to normal? Thank you for your patience and for reporting this.

In D73664#1975208, @nemanjai wrote:

In D73664#1975159, @nathanchance wrote:

In D73664#1975045, @nemanjai wrote:

This is also a regression after this patch or something that was broken before?

Yes. If I checkout to f3bf25eb663c49e821120fd311677764d2bad79e, modules builds successfully but with 6c4b40def77622a5cf62a219ef4af63dc876e144, I get that error, with or without 5b18b6e9a84d985c0a907009fb71de7c1943bc88.

Awesome. Thank you. I'll work on getting that fixed. To verify everything, I just replace modules in the invocation with all?

In D73664#1975045, @nemanjai wrote:

This is also a regression after this patch or something that was broken before?

@nemanjai thank you for that fix! Unfortunately, I am still seeing some errors with the modules target:

For what it's worth, this patch introduces a regression when building the Linux kernel's powernv_defconfig with Clang and linking with ld.bfd (for some reason, ld.lld is fine):

These are new tests how do you get the generic lld driver to work?

Weird -- the baseline didn't take CLANG_DEFAULT_LINKER into account either, though, right?

This does not take into account CLANG_DEFAULT_LINKER, resulting in a check-clang failure:

I can confirm this fixes my issue, thank you for the quick fix! I can approve if need be but might be better for someone else more familiar with this area to do that :)

This change breaks -DLLVM_INCLUDE_TESTS=OFF:

Seems sane to me if I am understanding the string arithmetic correctly. I can confirm this fixes the Debian use case that we came across.

Thanks for the review and accepting. I do not have commit rights, would you mind doing that on my behalf?

In D72231#1881855, @rjmccall wrote:

In D72231#1881797, @nickdesaulniers wrote:

In D72231#1881784, @rjmccall wrote:

In D72231#1881760, @nickdesaulniers wrote:

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Maybe we should only warn if the size of the void* is smaller than the size of the enum? (32b void*, 64b enum)? https://godbolt.org/z/oAts-u

Otherwise this warning creates a massive mess for us to clean up, and I suspect Linux kernel developers will just end up disabling the warning.

If deployment is easier if we split out a subgroup that we can turn off, that seems fine. But I don't see any good abstract justification for warning about a cast to int and not a cast to an int-sized enum. What would the reasoning be, just that the latter "couldn't possibly" be intended to preserve the original pointer value, so it must be an opaque value being represented as a void*? That seems pretty weak to me.

Less about enums, more about casts to/from void*, since you might use that in place of a union that would be too large to describe. Specifically, this struct is used throughout the kernel for most drivers: https://elixir.bootlin.com/linux/v5.5.4/source/include/linux/mod_devicetable.h#L260 It is exceedingly common to stuff whatever data in there: https://elixir.bootlin.com/linux/v5.5.4/source/drivers/ata/ahci_brcm.c#L428 so long as the driver is careful not to reinterpret the data as the incorrect type. Describing such a union for ever possible enum packed in there would not be fun.

No, I understand the pattern, but they must have already done some sort of pass over the code to make it warning-clean when they're working with a smaller integer type. Or do they just in practice never store smaller integers in there, whereas it's hard to control size with an enum?

In D72231#1879347, @rjmccall wrote:

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

The Linux kernel hits the assert in markConstant on arm32 defconfig in net/mac80211/sta_info.o, as reported by Linaro's CI:

Please revert; I am happy to test a new revision to make sure there are no warnings but I don’t want this shipped in clang-10 and a revert is something that we can easily backport unless you can come up with a fix rather quickly.

This fixes the error for me and looks better from a documentation perspective.

I am seeing the same failure that @leonardchan reported above. This is related to -DCLANG_DEFAULT_LINKER=lld:

In D71627#1793142, @jonpa wrote:

In D71627#1793006, @nickdesaulniers wrote:

This is regressing linux kernel builds for multiple architectures. The linux kernel uses feature detection for compiler flags, basically invoking:

$ clang -mrecord-mcount -target aarch64-linux-gnu -c -x c /dev/null

Because the target info isn't validated until code gen, the above returns 0, which seems like the flag is supported, so it is used for further compilations which then do fail.

I am not sure how this should be handled... Maybe you could open an issue on Bugzilla and add the right people on the CC list (I am not sure exactly who they would be...)?

As a follow up to my previous post, I have sent patches to clean up all of the warnings that I see in the Linux kernel. However, I found one that I do believe is a false positive:

IMO that is misleading indentation. A single space followed by a tab? WTF

As an FYI, this appears to cause several false positive warnings with the Linux kernel:

Yes, it does! I can approve this if you need it but I am not an expect in cmake so I cannot comment on how proper this is.

Just as an FYI, this patch breaks LLVM_INCLUDE_TESTS=OFF for me:

@NoQ thank you for the fix, I can confirm it works!

This commit causes some warnings in the Linux kernel that appear to be false positives. For example:

LGTM.

The test case passes for me after this, thanks for the fix!

My instrumented stage 2 build fails after this commit with this script.

Could it be an issue with python? What is the version you are using?

Done, thanks for looking into this!

This test case has never passed for me with check-clang, is there something environment related that is needed with this or are others experiencing this too?

Yes, it does look like that is the case, I will abandon this. Thanks for the reply!

This causes a compiler hang (I assume infinite loop or deadlock somewhere) when compiling the Linux kernel.

I tested this patch on top of r366728 and saw no regressions in my set of kernel builds.

This change breaks building the Linux kernel for arm32 (at least):

Thanks for the quick fix, looks good to me!

Is there a point to adding this if it isn't a user selectable option? LLVM_ENABLE_PLUGINS doesn't have an option in any cmake files, it is only ever set.