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

[rSerge]

Summary

Added the code which explicitly emits an error in Clang in case -fxray-instrument is passed, but XRay is not supported for the selected target.

[dberris]

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"?

[rSerge]

In D24799#549442, @dberris wrote:

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.

[dberris]

In D24799#549634, @rSerge wrote:

In D24799#549442, @dberris wrote:

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]

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]

lib/Driver/Tools.cpp

4777–4780

Wouldn't something like this work better:

D.Diag(...) << XRayInstrumentOption << " on " << Triple.getArchName();

[rSerge]

lib/Driver/Tools.cpp

4777–4780

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]

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.

[rSerge]

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

[dberris]

In D24799#558888, @rSerge wrote:

@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. :)

[dberris]

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.

[dberris]

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

[rSerge]

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.

[dberris]

In D24799#561106, @rSerge wrote:

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]

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]

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

4770–4783

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

4779

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;

4782

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.

[rSerge]

In D24799#563058, @dberris wrote:

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

4770–4783

Is there any advantage over const char* const here?

4779

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?

4782

Done.

[dberris]

lib/Driver/Tools.cpp

4770–4783

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

4779

.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]

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

[rSerge]

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

4779

I see now, changing.

[rSerge]

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

[dberris]

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.

[dberris]

In D24799#566507, @rSerge wrote:

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

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

[rSerge]

@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]

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]

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

[dberris]

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.

[rSerge]

@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?

[dberris]

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 &&

[rSerge]

@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.

lib/Driver/Tools.cpp

#endif
   if (!Args.hasFlag(options::OPT_funique_section_names,
                     options::OPT_fno_unique_section_names, true))
     CmdArgs.push_back("-fno-unique-section-names");
 
   Args.AddAllArgs(CmdArgs, options::OPT_finstrument_functions);
 
   if (Args.hasFlag(options::OPT_fxray_instrument,
                    options::OPT_fnoxray_instrument, false)) {
-    CmdArgs.push_back("-fxray-instrument");
+    const char* const XRayInstrumentOption = "-fxray-instrument";
+    switch(getToolChain().getArch()) {
+    case llvm::Triple::arm:
+    case llvm::Triple::x86_64:
+      break;
+    default:
+    {
+      std::string Feature(XRayInstrumentOption);
+      Feature += " on ";
+      Feature += Triple.getArchName().data();
+      D.Diag(diag::err_drv_clang_unsupported) << Feature;
+      break;
+    } }
+    CmdArgs.push_back(XRayInstrumentOption);
     if (const Arg *A =
             Args.getLastArg(options::OPT_fxray_instruction_threshold_,
                             options::OPT_fxray_instruction_threshold_EQ)) {
       CmdArgs.push_back("-fxray-instruction-threshold");
       CmdArgs.push_back(A->getValue());
     }
   }
 

test/Driver/xray-instrument.c

+// RUN: %clang -fxray-instrument %s
+// XFAIL: armeb,aarch64,aarch64_be,avr,bpfel,bpfeb,hexagon,mips,mipsel,mips64,mips64el,msp430,ppc,ppc64,ppc64le,r600,amdgcn,sparc,sparcv9,sparcel,systemz,tce,thumb,thumbeb,x86,xcore,nvptx,nvptx64,le32,le64,amdil,amdil64,hsail,hsail64,spir,spir64,kalimba,shave,lanai,wasm32,wasm64,renderscript32,renderscript64