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

[Serge Rogatch via cfe-commits]

rSerge added a comment.

In https://reviews.llvm.org/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.



> dberris wrote in Tools.cpp:4787
> Why can't this just be a `const string`, or a `const StringRef`?

Is there any advantage over `const char* const` here?

> dberris wrote in Tools.cpp:4796
> 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;

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

> dberris wrote in Tools.cpp:4799
> 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.

Done.

https://reviews.llvm.org/D24799