Page MenuHomePhabricator

Add support to -Wa,--version in clang
ClosedPublic

Authored by jcai19 on Mon, Mar 29, 6:05 PM.

Details

Summary

Clang currently only supports -Wa,--version when -no-integrated-as is
used. This adds support to -Wa,--version with -integrated-as.

Link:
https://github.com/ClangBuiltLinux/linux/issues/1320

Diff Detail

Event Timeline

jcai19 requested review of this revision.Mon, Mar 29, 6:05 PM
jcai19 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 29, 6:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

gcc does not understand options in -Wa, so -Wa,--version needs to be used this way: gcc -Wa,--version -c -x assembler /dev/null -o /dev/null
The verbose syntax makes it not that useful. If the kernel really wants to use it, I have no issue with it.

-integrated-as => -fintegrated-as. -i* options are generally used for include related features.
Similarly, -no-integrated-as => -fno-integrated-as

MaskRay added inline comments.Mon, Mar 29, 7:25 PM
clang/test/Driver/as-options.s
40 ↗(On Diff #334027)

Drop 2>&1 because you specifically want to test stdout.

gcc does not understand options in -Wa, so -Wa,--version needs to be used this way: gcc -Wa,--version -c -x assembler /dev/null -o /dev/null
The verbose syntax makes it not that useful. If the kernel really wants to use it, I have no issue with it.

I agree; the kernel only uses the compiler as the driver for assembler sources, as the kernel assembler is almost entirely -x assembler-with-cpp, so the C pre processor must be run first over the sources. This is done to share important constants like the PAGE_SIZE between C and assembler. For this reason, the maintainer of the kernel build system (Kbuild) would prefer to detect assembler version via $(CC) -Wa,--version.

@jcai19 would you mind adding a test like clang/test/Driver/version.c? clang/test/Driver/version.s perhaps? Testing that there is some output from -Wa,--version might be helpful.

nickdesaulniers removed a subscriber: MaskRay.
jcai19 updated this revision to Diff 334277.Tue, Mar 30, 3:00 PM
jcai19 marked an inline comment as done.

Update tests to address comments.

clang/test/Driver/as-version.s
10

What happens if we run this on windows (where I suspect that GAS isn't available)? Perhaps a CHECK-NOT: clang would be better?

jcai19 updated this revision to Diff 334315.Tue, Mar 30, 6:02 PM
jcai19 marked an inline comment as done.

Update tests.

clang/test/Driver/as-version.s
10

CHECK-NOT does sound safer in this case. I've updated the test. Thanks.

nickdesaulniers accepted this revision.Tue, Mar 30, 6:06 PM
This revision is now accepted and ready to land.Tue, Mar 30, 6:06 PM
MaskRay accepted this revision.Tue, Mar 30, 7:24 PM
MaskRay added inline comments.
clang/test/Driver/as-version.s
4

Delete excess spaces

This revision was landed with ongoing or failed builds.Wed, Mar 31, 4:30 PM
This revision was automatically updated to reflect the committed changes.
jcai19 marked an inline comment as done.

Looks like the test fails on non-linux:
http://45.33.8.238/macm1/6631/step_6.txt

Please take a look and revert for now if it takes a while to fix.

ah, sorry, I think we're missing a // REQUIRES: linux in the newly added test? I've pushed a revert for now.

dyung added a subscriber: dyung.Wed, Mar 31, 6:32 PM

It also fails with the PS4 triple because we default to an external assembler that isn't present: https://lab.llvm.org/buildbot/#/builders/139/builds/2161/steps/6/logs/FAIL__Clang__as-version_s

We can probably remove the -fno-integraetd-as case.

We can probably remove the -fno-integraetd-as case.

Or split it out into a separate test file with // REQUIRES: linux. Either actually is fine by me.

We can probably remove the -fno-integraetd-as case.

Or split it out into a separate test file with // REQUIRES: linux. Either actually is fine by me.

No, we should reduce reliance on the host system. Printing ....as is fine (with -B), but invoking it to get the output is not something we should do in isolated tests.

In the future, please include Differential Revision: for relands so that the Commits line displays the associated commits :)