Page MenuHomePhabricator

[XRay] Check in Clang whether XRay supports the target when -fxray-instrument is passed
ClosedPublic

Authored by rSerge on Sep 21 2016, 7:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

rSerge updated this revision to Diff 72039.Sep 21 2016, 7:26 AM
rSerge retitled this revision from to [XRay] Check in Clang whether XRay supports the target when -fxray-instrument is passed.
rSerge updated this object.
rSerge added reviewers: rsmith, dberris.
rSerge added subscribers: cfe-commits, iid_iunknown.
dberris edited edge metadata.Sep 21 2016, 7:17 PM

What does the error actually look like? Can you add a test for it? It's unclear to me how this would read... for example does it say "XRay for arm is unsupported"?

What does the error actually look like? Can you add a test for it? It's unclear to me how this would read... for example does it say "XRay for arm is unsupported"?

In the attached picture you can see how the error looks when cross-compiling with Clang from x86_64-Windows host to Thumb-Linux target.


It says

clang++.exe: error: the clang compiler does not support 'XRay for armv6kz--linux-gnueabihf'

I'll try to add tests.

What does the error actually look like? Can you add a test for it? It's unclear to me how this would read... for example does it say "XRay for arm is unsupported"?

In the attached picture you can see how the error looks when cross-compiling with Clang from x86_64-Windows host to Thumb-Linux target.


It says

clang++.exe: error: the clang compiler does not support 'XRay for armv6kz--linux-gnueabihf'

Thanks -- I that's really helpful.

A clearer message I think might read something like:

the clang compiler does not support '-fxray-instrument' on '-mtriple=armv6kz-linux-gnueabihf'

Even if you drop the '-mtriple=' prefix, I think that might work better than "XRay for".

I'll try to add tests.

Thanks -- an XFAIL for a platform you know it should fail for should be sufficient, without spelling out the actual error message in the test.

rSerge updated this revision to Diff 72312.EditedSep 23 2016, 11:28 AM
rSerge edited edge metadata.

Added a test.
Changed the error message to:

clang++.exe: error: the clang compiler does not support '-fxray-instrument on armv6kz--linux-gnueabihf'

I think it's better not to say -mtriple in the error message, because there are AFAIK different possibilities for specifying the target architecture, so an error message with -mtriple may confuse the user if he used a different option to specify the target.

dberris added inline comments.Sep 26 2016, 1:56 AM
lib/Driver/Tools.cpp
4777–4780 ↗(On Diff #72312)

Wouldn't something like this work better:

D.Diag(...) << XRayInstrumentOption << " on " << Triple.getArchName();
rSerge added inline comments.Sep 26 2016, 6:58 AM
lib/Driver/Tools.cpp
4777–4780 ↗(On Diff #72312)

No, it doesn't work this way. Although D.Diag looks like a C++ stream because of overloaded << operator, it works like C printf and diag::err_drv_clang_unsupported is like a format specifier. The latter specifies that only the first argument added via operator<< is included in the final message. So we have to pass in one string everything we want to say. And LLVM coding guidelines forbid C++ streams.

rSerge updated this revision to Diff 72554.Sep 26 2016, 1:50 PM

Implemented a workaround for XFAIL not differentiating between x86 and x86_64 because it searches for a substring in the triple string, thus x86 matches both x86-X-Y-Z and x86_64-X-Y-Z.

dberris accepted this revision.Sep 28 2016, 8:19 PM
dberris edited edge metadata.
This revision is now accepted and ready to land.Sep 28 2016, 8:19 PM
rSerge added a comment.Oct 3 2016, 4:19 AM

@dberris , could you deliver this patch to mainline? Or do we need approval from more reviewers?

@dberris , could you deliver this patch to mainline? Or do we need approval from more reviewers?

Yes, I can do this -- sorry it was the long weekend for me down under. I'll get to this today. :)

This revision was automatically updated to reflect the committed changes.
dberris reopened this revision.Oct 4 2016, 2:06 AM

This broke the build on i686.

******************** TEST 'Clang :: Driver/xray-instrument.c' FAILED ********************
Script:
--
C:/bb-win/ninja-clang-i686-msc19-R/build/./bin/clang.EXE  -v -fxray-instrument -c C:\bb-win\ninja-clang-i686-msc19-R\llvm-project\clang\test\Driver\xray-instrument.c
--
Exit Code: 1

Command Output (stdout):
--
$ "C:/bb-win/ninja-clang-i686-msc19-R/build/./bin/clang.EXE" "-v" "-fxray-instrument" "-c" "C:\bb-win\ninja-clang-i686-msc19-R\llvm-project\clang\test\Driver\xray-instrument.c"
# command stderr:
clang version 4.0.0 

Target: i686-pc-windows-msvc

Thread model: posix

InstalledDir: C:\bb-win\ninja-clang-i686-msc19-R\build\bin

clang.EXE: error: the clang compiler does not support '-fxray-instrument on i686-pc-windows-msvc19.0.23506'


error: command failed with exit status: 1

Reverted in rL283199.

This revision is now accepted and ready to land.Oct 4 2016, 2:06 AM
dberris requested changes to this revision.Oct 4 2016, 2:12 AM
dberris edited edge metadata.
dberris added a subscriber: rengolin.

I'm not sure whether the exhaustive list scales though... is there a better way of doing this? Maybe @rengolin has better ideas here?

This revision now requires changes to proceed.Oct 4 2016, 2:12 AM
rSerge updated this revision to Diff 73509.Oct 4 2016, 10:56 AM
rSerge edited edge metadata.
rSerge removed rL LLVM as the repository for this revision.

My mistake was that initially I only enumerated the unsupported targets from llvm\include\llvm\ADT\Triple.h . Now I've added also the cases from llvm\lib\Support\Triple.cpp .
XFAIL requires a list of all unsupported cases, which is currently much larger than the list of supported cases. However, AFAIK there is nothing like XPASS in LIT.

My mistake was that initially I only enumerated the unsupported targets from llvm\include\llvm\ADT\Triple.h . Now I've added also the cases from llvm\lib\Support\Triple.cpp .
XFAIL requires a list of all unsupported cases, which is currently much larger than the list of supported cases. However, AFAIK there is nothing like XPASS in LIT.

I just thought about reversing this. How about if you do something like:

// RUN: not %clang -v -fxray-instrument -c %s
// XFAIL: x86_64-, arm7-

I suspect that would be sufficient to work as an XPASS of sorts?

rSerge updated this revision to Diff 73673.EditedOct 5 2016, 11:02 AM
rSerge edited edge metadata.

I just thought about reversing this. How about if you do something like:

// RUN: not %clang -v -fxray-instrument -c %s
// XFAIL: x86_64-, arm7-

I suspect that would be sufficient to work as an XPASS of sorts?

Good idea, thanks. I've used it.

dberris requested changes to this revision.Oct 5 2016, 11:29 PM
dberris edited edge metadata.

Are we sure this will not fail on Windows? i.e. have you built/run the tests on Windws ARM or X86_64?

lib/Driver/Tools.cpp
4787 ↗(On Diff #73673)

Why can't this just be a const string, or a const StringRef?

4796 ↗(On Diff #73673)

I'm not a fan of calling .data() here -- if this returns a StringRef I'd much rather turn it into a string directly. Either that or you can even string these together. Say something like:

default:
  D.Diag(...) << (XRayInstrumentOption + " on " + Triple.getArchName().str());
  break;
4799 ↗(On Diff #73673)

As part of my submitting this change upstream, I had to format it accordingly with clang-format -- you might want to do the same so that the formatting is in-line with the LLVM developers style guidelines.

This revision now requires changes to proceed.Oct 5 2016, 11:29 PM

Are we sure this will not fail on Windows? i.e. have you built/run the tests on Windws ARM or X86_64?

The test itself passes for arm-pc-win32 and x86_64-pc-win32 on my machine. So in this sense it doesn't fail.
However, the feature of this patch doesn't cover the OS part of the triple: currently because of mprotect and other Linux-only code, XRay is only supported on Linux. However, clang (also with this patch) doesn't check for Linux target, and on non-Linux machines either LLVM backend will emit an error later (saying that XRay is not supported on Thumb - this happens because Windows is Thumb-only for ARM target), or I expect linker errors because code generation on Windows references some API of compiler-rt, which doesn't get compiled on Windows, even if it's x86_64.

lib/Driver/Tools.cpp
4787 ↗(On Diff #73673)

Is there any advantage over const char* const here?

4796 ↗(On Diff #73673)

It returns StringRef. .str() would construct a std::string, which seems unnecessary. Of course this piece is not performance-critical, but why not to just use operator+= taking as the argument const char* const?

4799 ↗(On Diff #73673)

Done.

rSerge updated this revision to Diff 73824.Oct 6 2016, 11:04 AM
rSerge edited edge metadata.
dberris added inline comments.Oct 6 2016, 9:46 PM
lib/Driver/Tools.cpp
4787 ↗(On Diff #73673)

This same value is turned into a string later on anyway. You can make it a std::string and std::move(...) it at the call to CmdArgs.push_back(...).

4796 ↗(On Diff #73673)

.data() doesn't necessarily have a null terminator.

Constructing a string out of successive +'s invoke move constructors on std::string, which makes it as efficient if not more efficient than growing a single string this way. At any rate it's much more readable if you did it in a single line.

dberris requested changes to this revision.Oct 10 2016, 3:54 AM
dberris edited edge metadata.

Sorry, I forgot to update the status. I made a few more comments to make this part a little more readable.

This revision now requires changes to proceed.Oct 10 2016, 3:54 AM
rSerge added a comment.EditedOct 10 2016, 12:21 PM

I have extended this feature to check for OS support too (currently Linux only). I can't commit it so far because I don't know how to implement a test. XFAIL cannot check for both CPU and OS: it can only check for one of them. I tried to implement 2 tests instead like these:
Test 1:

// RUN: not %clang -v -fxray-instrument -c %s
// XFAIL: Linux
// REQUIRES-ANY: amd64-, x86_64, x86_64h, arm
typedef int a;

Test 2:

// RUN: not %clang -v -fxray-instrument -c %s
// XFAIL: amd64-, x86_64, x86_64h, arm
// REQUIRES: Linux
typedef int a;

However the problem with REQUIRES / REQUIRES-ANY is that they only check in LIT features, but not in the target triple. So everything becomes unsupported.

Does anyone have any ideas on how to implement the tests for Clang checking for both OS and CPU? I have 2 options in mind:

  1. extend LIT, putting OS and CPU into the feature list
  2. implement the test via GTest, rather than LIT
lib/Driver/Tools.cpp
4796 ↗(On Diff #73673)

I see now, changing.

rSerge updated this revision to Diff 74829.Oct 17 2016, 5:33 AM
rSerge edited edge metadata.

Ping? (please, see the question about the test)

Sorry for the delay, I had thought I pointed to some potentially helpful documentation. :/

BTW, did the test get removed from the latest change? I don't see it being added anymore.

I have extended this feature to check for OS support too (currently Linux only). I can't commit it so far because I don't know how to implement a test. XFAIL cannot check for both CPU and OS: it can only check for one of them. I tried to implement 2 tests instead like these:
Test 1:

// RUN: not %clang -v -fxray-instrument -c %s
 // XFAIL: Linux
 // REQUIRES-ANY: amd64-, x86_64, x86_64h, arm
 typedef int a;

Test 2:

// RUN: not %clang -v -fxray-instrument -c %s
 // XFAIL: amd64-, x86_64, x86_64h, arm
 // REQUIRES: Linux
 typedef int a;

However the problem with REQUIRES / REQUIRES-ANY is that they only check in LIT features, but not in the target triple. So everything becomes unsupported.

Does anyone have any ideas on how to implement the tests for Clang checking for both OS and CPU? I have 2 options in mind:

  1. extend LIT, putting OS and CPU into the feature list

This should be configurable in the various lit.site.cfg files for Clang. I suspect though that just relying on the REQUIRES exclusively should work -- i.e. define a list of features that you actually require.

http://llvm.org/docs/TestingGuide.html#requires-and-requires-any-directive

  1. implement the test via GTest, rather than LIT

Probably won't work since this is a test of the compiler, unless you had something more specific?

@dberris , below is the list of features supported by REQUIRES and REQUIRES-ANY for me:

set(['system-windows', 'nozlib', u'amdgpu-registered-target', 'case-insensitive-filesystem', u'msp430-registered-target', u'sparc-registered-target', 'libgcc', 'staticanalyzer', 'native', u'powerpc-registered-target', 'non-ps4-sdk', 'clang-driver', 'backtrace', 'crash-recovery', u'bpf-registered-target', u'hexagon-registered-target', 'not_ubsan', u'x86-registered-target', u'aarch64-registered-target', u'arm-registered-target', 'not_asan', u'nvptx-registered-target', u'mips-registered-target', u'systemz-registered-target', u'lanai-registered-target', u'xcore-registered-target'])

So there is system-windows, however, there is no target CPU among the features. I understand that the features with -registered-target suffixes mean the targets supported with the current Clang build.
I am not submitting the tests because they do not work for the above reason. I also had to remove the old test because the new code checks the OS, but not only the CPU.

rSerge updated this revision to Diff 75452.Oct 21 2016, 11:22 AM
rSerge edited edge metadata.

I had to add the root directories a and b manually, as I couldn't find an svn diff argument for that.
The code file Tools.cpp was run via clang-format, then just my changes were copy-pasted.
2 tests have been added to enable both OS and CPU checking.
const string approach was leading to undefined behavior, because the lifetime of the string is shorter than the lifetime of the const char * array using its data. So I had to return to const char* const.

dberris accepted this revision.Oct 26 2016, 9:55 PM
dberris edited edge metadata.

Thanks @rSerge -- I'll land this now.

This revision is now accepted and ready to land.Oct 26 2016, 9:55 PM

Actually I tried again, but still the patch doesn't apply cleanly through git (and it still complains of whitespace issues). Let me go through this manually again, and see whether there are still failures that come up.

This revision was automatically updated to reflect the committed changes.

@dberris , thanks! I ran clang-format on Tools.cpp , then just copy-pasted the changed piece. Do you remember, was the whitespace problem in this file? Or was it in the tests?

It was in the .cpp file, and was mostly trailing whitespace. This is what I did:

curl https://reviews.llvm.org/file/data/d72xn56ybspvx3ovex36/PHID-FILE-vzd63mvia62eq7e32iho/D24799.diff | git apply - -p0 --whitespace=fix

And I got the following:

<stdin>:6: trailing whitespace.

<stdin>:7: trailing whitespace.
  if (Args.hasFlag(options::OPT_fxray_instrument,
<stdin>:8: trailing whitespace.
                   options::OPT_fnoxray_instrument, false)) {
<stdin>:10: trailing whitespace.
    const char *const XRayInstrumentOption = "-fxray-instrument";
<stdin>:11: trailing whitespace.
    if (Triple.getOS() == llvm::Triple::Linux &&

@dberris , I've checked the .diff file which I was uploading to Phabricator, and it doesn't seem to contain trailing whitespace. Please, see the screenshot:


So it seems that Phabricator or your downloading tool adds the whitespace.
The only potential problem is in "CR,LF" line terminator, while you may be using Linux "LF" only.