[PATCH] D20352: Add XRay flags to Clang

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 27 07:37:52 PDT 2016

aaron.ballman added a comment.

It looks like you ran clang-format over the entire file in some instances, causing a lot of unrelated changes to creep in. Can you back those changes out and only run clang-format over just your patch instead (http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting)?

Comment at: include/clang/Basic/Attr.td:436
@@ +435,3 @@
+                   CXX11<"clang", "xray_never_instrument">];
+  let Subjects = SubjectList<[CXXMethod, ObjCMethod, Function], WarnDiag,
+                              "ExpectedFunctionOrMethod">;
You can drop the WarnDiag and string literal; the defaults already do the right thing.

Comment at: include/clang/Basic/AttrDocs.td:2455
@@ +2454,3 @@
+  let Content = [{
+``__attribute__((xray_always_instrument))`` or ``[[clang:xray_always_instrument]]`` is used to mark member functions (in C++), methods (in Objective C), and free functions (in C, C++, and Objective C) to be instrumented with XRay. This will cause the function to always have space at the beginning and exit points to allow for runtime patching.
Let's make sure that this patch and D19908 do not conflict, because they seem to be implementing competing semantics.

Comment at: lib/CodeGen/CodeGenFunction.cpp:381
@@ -380,3 +380,3 @@
-  for (SmallVectorImpl<std::pair<llvm::Instruction *, llvm::Value *> >::iterator
+  for (SmallVectorImpl<std::pair<llvm::Instruction *, llvm::Value *>>::iterator
            I = DeferredReplacements.begin(),
Can't you rangify this loop?

Comment at: lib/CodeGen/CodeGenFunction.cpp:402
@@ +401,3 @@
+/// instrumented with XRay nop sleds.
+bool CodeGenFunction::ShouldXRayInstrumentFunction() {
+  return CGM.getCodeGenOpts().XRayInstrumentFunctions;
Any particular reason this isn't a `const` function?

Comment at: lib/CodeGen/CodeGenFunction.cpp:691
@@ +690,3 @@
+    if (D->hasAttr<XRayInstrumentAttr>()) {
+      switch (D->getAttr<XRayInstrumentAttr>()->getSpellingListIndex()) {
+        case 0:
This is not the right way to do this. ;-)

You should check the *semantic* spelling instead. e.g.,
if (const auto *A = D->getAttr<XRayInstrumentAttr>()) {
  switch (A->getSemanticSpelling()) {
  case XRayInstrumentAttr::GNU_blahblah:
  case XRayInstrumentAttr::CXX11_clang_blahblah:
Alternatively (and I think this may be cleaner), you could add an additional member in Attr.td that exposes this as two Boolean getters `bool alwaysInstrument() const` and `bool neverInstrument() const` that do the semantic spelling switch.


More information about the cfe-commits mailing list