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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
