[PATCH] D20352: Add XRay flags to Clang

[Aaron Ballman via cfe-commits]

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.


http://reviews.llvm.org/D20352