[PATCH] D20352: Add XRay flags to Clang

[Dean Michael Berris via cfe-commits]

dberris added inline comments.

================
Comment at: include/clang/Basic/Attr.td:436
@@ +435,3 @@
+                   CXX11<"clang", "xray_never_instrument">];
+  let Subjects = SubjectList<[CXXMethod, ObjCMethod, Function], WarnDiag,
+                              "ExpectedFunctionOrMethod">;
----------------
aaron.ballman wrote:
> You can drop the WarnDiag and string literal; the defaults already do the right thing.
I tried, but I'm getting this error:

.../Basic/Attr.td:436:18: error: Could not deduce diagnostic argument for Attr subjects
  let Subjects = SubjectList<[CXXMethod, ObjCMethod, Function]>;


================
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.
+
----------------
aaron.ballman wrote:
> Let's make sure that this patch and D19908 do not conflict, because they seem to be implementing competing semantics.
Good catch. Let me comment on D19908 to see if it competes with how we define XRay instrumentation. As far as I can tell from the LLVM side, those should not interfere with each other.

================
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(),
----------------
aaron.ballman wrote:
> Can't you rangify this loop?
Probably in a different change. :) For now I've reverted the formatting changes.

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:402
@@ +401,3 @@
+/// instrumented with XRay nop sleds.
+bool CodeGenFunction::ShouldXRayInstrumentFunction() {
+  return CGM.getCodeGenOpts().XRayInstrumentFunctions;
----------------
aaron.ballman wrote:
> Any particular reason this isn't a `const` function?
No good reason. :) Good catch, fixed.

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:691
@@ +690,3 @@
+    if (D->hasAttr<XRayInstrumentAttr>()) {
+      switch (D->getAttr<XRayInstrumentAttr>()->getSpellingListIndex()) {
+        case 0:
----------------
aaron.ballman wrote:
> 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.
I thought this was a little weird, thanks for pointing this out!

I've used accessors instead, maybe it looks better now?


http://reviews.llvm.org/D20352