Page MenuHomePhabricator

[Clang FE, SystemZ] Recognize -mrecord-mcount CL option.
ClosedPublic

Authored by jonpa on Dec 17 2019, 1:54 PM.

Details

Reviewers
uweigand
Summary

Recognize -mrecord-mcount from the command line and add a function attribute "mrecord-mcount when passed.

Only valid on SystemZ when used with -mfentry.

Diff Detail

Event Timeline

jonpa created this revision.Dec 17 2019, 1:54 PM
jonpa updated this revision to Diff 234607.Dec 18 2019, 1:33 PM

Probably better to only have one hard-coded space character in mrecord-mcount.c CHECK line.

uweigand accepted this revision.Dec 18 2019, 2:17 PM

LGTM.

This revision is now accepted and ready to land.Dec 18 2019, 2:17 PM

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.

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...)?

Can we simply move all the validity checks (arg only valid on SystemZ, or arg only valid in combination with another arg) into ParseCodeGenArgs in CompilerInvocation.cpp, where the args are initially looked at?

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...)?

While that is discussed, I think this commit should be reverted. All working Linux kernel allyesconfig builds with Clang are now broken with this commit because of what Nick pointed out (-mrecord-mcount gets added to the list of CFLAGS then the build fails when actual code generation starts).

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...)?

While that is discussed, I think this commit should be reverted. All working Linux kernel allyesconfig builds with Clang are now broken with this commit because of what Nick pointed out (-mrecord-mcount gets added to the list of CFLAGS then the build fails when actual code generation starts).

I noticed this problem as well. I will try fixing it..

Fixed by 0792ef72564071f21b727d0de3a14d565950290f. My Linux powerpc64le build is good now.

make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- CC=~/llvm/Release/bin/clang LD=~/llvm/Release/bin/ld.lld O=out/powerpc64le powernv_defconfig zImage.epapr -j 30

Fixed by 0792ef72564071f21b727d0de3a14d565950290f. My Linux powerpc64le build is good now.

make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- CC=~/llvm/Release/bin/clang LD=~/llvm/Release/bin/ld.lld O=out/powerpc64le powernv_defconfig zImage.epapr -j 30

Thank for you the fix, and sorry for the disruption!

There is another limitation of the current implementation: -mrecord-mcount / -mnop-mcount are only supported with -mfentry, not with the "regular" mcount profiling. This is just a limitation of the current implementation (but in any case, the Linux kernel build will only use it in this combination anyway).

We currently have checks that this combination is specified, but those are also in the code generator. Should they be moved to the driver as well?

Fixed by 0792ef72564071f21b727d0de3a14d565950290f. My Linux powerpc64le build is good now.

make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- CC=~/llvm/Release/bin/clang LD=~/llvm/Release/bin/ld.lld O=out/powerpc64le powernv_defconfig zImage.epapr -j 30

Thank for you the fix, and sorry for the disruption!

There is another limitation of the current implementation: -mrecord-mcount / -mnop-mcount are only supported with -mfentry, not with the "regular" mcount profiling. This is just a limitation of the current implementation (but in any case, the Linux kernel build will only use it in this combination anyway).

We currently have checks that this combination is specified, but those are also in the code generator. Should they be moved to the driver as well?

gcc/config/s390/s390.c requires -mnop-mcount to be used together with -mfentry, but gcc/config/i386/i386-options.c doesn't.

% clang -target s390x -pg -mnop-mcount -c a.c      
error: option '-mnop-mcount' cannot be specified without '-mfentry'
1 error generated.
% gcc -pg -mnop-mcount -fno-pic -c a.c

(GCC performs some other checks. I noted some in commit 527b0f8c7448707aa6b8159e6e4707a49f1dcb87, but I am not sure whether we should document all... These options are only used by Linux kernel, and more specifically, ftrace.)