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

Serge Rogatch via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 10 12:21:22 PDT 2016


rSerge added a comment.

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:

1. ```// RUN: not %clang -v -fxray-instrument -c %s

// XFAIL: Linux
// REQUIRES-ANY: amd64-, x86_64, x86_64h, arm
typedef int a;

  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



================
Comment at: lib/Driver/Tools.cpp:4796
+      Feature += " on ";
+      Feature += Triple.getArchName().data();
+      D.Diag(diag::err_drv_clang_unsupported) << Feature;
----------------
dberris wrote:
> rSerge wrote:
> > dberris wrote:
> > > 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`?
> .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.
I see now, changing.


https://reviews.llvm.org/D24799





More information about the cfe-commits mailing list