ClangBuiltLinux / linux

Linux kernel source tree
Other
241 stars 14 forks source link

"'-pcrel' / '-prefixed' is not a recognized feature for this target" when building .S files #1839

Open nathanchance opened 1 year ago

nathanchance commented 1 year ago

After commit 648a1783fe25 ("powerpc/boot: Fix boot wrapper code generation with CONFIG_POWER10_CPU") in the PowerPC tree, I see:

$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- LLVM=1 O=build mrproper powernv_defconfig all
'-prefixed' is not a recognized feature for this target (ignoring feature)
'-pcrel' is not a recognized feature for this target (ignoring feature)
'-prefixed' is not a recognized feature for this target (ignoring feature)
'-pcrel' is not a recognized feature for this target (ignoring feature)
'-prefixed' is not a recognized feature for this target (ignoring feature)
'-pcrel' is not a recognized feature for this target (ignoring feature)
'-prefixed' is not a recognized feature for this target (ignoring feature)
'-pcrel' is not a recognized feature for this target (ignoring feature)
'-prefixed' is not a recognized feature for this target (ignoring feature)
'-pcrel' is not a recognized feature for this target (ignoring feature)
'-prefixed' is not a recognized feature for this target (ignoring feature)
'-pcrel' is not a recognized feature for this target (ignoring feature)

This appears to occur when -mno-pcrel or -mno-prefixed is passed with -x assembler:

$ echo | clang --target=powerpc64le-linux-gnu -mno-pcrel -mno-prefixed -c -o /dev/null -x assembler -
'-pcrel' is not a recognized feature for this target (ignoring feature)
'-prefixed' is not a recognized feature for this target (ignoring feature)

$ echo | clang --target=powerpc64le-linux-gnu -mno-pcrel -mno-prefixed -c -o /dev/null -x c -

This is easy enough to just work around in the kernel (see diff below, it just moves the flags so that they are not added to AFLAGS) but I wonder if LLVM should be warning in this case?

cc @mpe @nemanjai

diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index 85cde5bf04b7..08b4c6dd1706 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -34,8 +34,6 @@ endif

 BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
                 -fno-strict-aliasing -O2 -msoft-float -mno-altivec -mno-vsx \
-                $(call cc-option,-mno-prefixed) $(call cc-option,-mno-pcrel) \
-                $(call cc-option,-mno-mma) \
                 $(call cc-option,-mno-spe) $(call cc-option,-mspe=no) \
                 -pipe -fomit-frame-pointer -fno-builtin -fPIC -nostdinc \
                 $(LINUXINCLUDE)
@@ -71,6 +69,10 @@ BOOTAFLAGS   := -D__ASSEMBLY__ $(BOOTCFLAGS) -nostdinc

 BOOTARFLAGS    := -crD

+BOOTCFLAGS +=  $(call cc-option,-mno-prefixed) \
+               $(call cc-option,-mno-pcrel) \
+               $(call cc-option,-mno-mma)
+
 ifdef CONFIG_CC_IS_CLANG
 BOOTCFLAGS += $(CLANG_FLAGS)
 BOOTAFLAGS += $(CLANG_FLAGS)
npiggin commented 1 year ago

If clang takes -mno-vsx and other code generation options then it seems consistent to not warn with these new options either.

In any case thanks for the fix, that should get it working.

nathanchance commented 1 year ago

If clang takes -mno-vsx and other code generation options then it seems consistent to not warn with these new options either.

I agree.

In any case thanks for the fix, that should get it working.

I will wait a few days to see if the LLVM folks have any comments on this issue. If not, I'll send that patch along as a quick fix, although I see there is some discussion on the linuxppc issue linked above that may end up superseding that fix, so I will keep an eye on it before sending the patch.

npiggin commented 1 year ago

If clang takes -mno-vsx and other code generation options then it seems consistent to not warn with these new options either.

I agree.

In any case thanks for the fix, that should get it working.

I will wait a few days to see if the LLVM folks have any comments on this issue. If not, I'll send that patch along as a quick fix, although I see there is some discussion on the linuxppc issue linked above that may end up superseding that fix, so I will keep an eye on it before sending the patch.

Thanks, I posted something to the list, but mpe might prefer your minimal fix at least to start with, so feel free to post your patch to the linuxppc list as an alternative.

I found a couple more issues with llvm with the pcrel code model when doing that. I've opened llvm issues for them, do you like to open issues to mirror such issues here?

nathanchance commented 1 year ago

Thanks, I posted something to the list, but mpe might prefer your minimal fix at least to start with, so feel free to post your patch to the linuxppc list as an alternative.

Thanks, I assume that is this series?

https://lore.kernel.org/20230426055848.402993-1-npiggin@gmail.com/

I think patches 3-5 could easily be taken as fixes at some point during the 6.4 cycle and even if they have to wait for 6.5, I do not think that it is the end of the world. The warning is a little annoying but it is not breaking the build and your series is a better overall fix than mine would be so I think we can just let that play out. Worst case, I will send my fix in a few weeks if there is no movement on that series :)

I found a couple more issues with llvm with the pcrel code model when doing that. I've opened llvm issues for them, do you like to open issues to mirror such issues here?

Thanks, looks like these ones?

https://github.com/llvm/llvm-project/issues/62372 https://github.com/llvm/llvm-project/issues/62373

I do not think we need to mirror them here, we only tend to do that when the issue was first opened and triaged here but needs to be resolved by others on the LLVM side, unless others have a different opinion :)

Thanks a lot for taking the time to test with LLVM and report those, it is much appreciated!

npiggin commented 1 year ago

Thanks, I posted something to the list, but mpe might prefer your minimal fix at least to start with, so feel free to post your patch to the linuxppc list as an alternative.

Thanks, I assume that is this series?

https://lore.kernel.org/20230426055848.402993-1-npiggin@gmail.com/

Yes.

I think patches 3-5 could easily be taken as fixes at some point during the 6.4 cycle and even if they have to wait for 6.5, I do not think that it is the end of the world. The warning is a little annoying but it is not breaking the build and your series is a better overall fix than mine would be so I think we can just let that play out. Worst case, I will send my fix in a few weeks if there is no movement on that series :)

Thinking about it a bit more, yours is the correct minimal fix for the recent commit 648a1783fe255, which is what that change should have been, so it's actually better for commit history to put in first even if more changes are coming.

Plus mpe won't be happy about making bigger changes here until the next release, so if you could send your patch when you get time would be really good. For the above patch,

Reviewed-by: Nicholas Piggin npiggin@gmail.com

I found a couple more issues with llvm with the pcrel code model when doing that. I've opened llvm issues for them, do you like to open issues to mirror such issues here?

Thanks, looks like these ones?

llvm/llvm-project#62372 llvm/llvm-project#62373

I do not think we need to mirror them here, we only tend to do that when the issue was first opened and triaged here but needs to be resolved by others on the LLVM side, unless others have a different opinion :)

Yeah those ones. Okay I we work around them in Linux for now so no problem.

Thanks a lot for taking the time to test with LLVM and report those, it is much appreciated!

Thank you for helping out with powerpc and making it all work so well. I need to make a point to do more llvm testing especially on build changes..

nathanchance commented 1 year ago

so if you could send your patch when you get time would be really good.

Ack, I have submitted https://lore.kernel.org/20230427-remove-power10-args-from-boot-aflags-clang-v1-1-9107f7c943bc@kernel.org/ with your tag, thanks a lot for the guidance and review!

nathanchance commented 1 year ago

Workaround in mainline: https://git.kernel.org/linus/2b694fc96fe33a7c042e3a142d27d945c8c668b0