[PATCH] D30630: [XRay][compiler-rt] Runtime changes to support custom event logging

[Tim Shen via Phabricator via llvm-commits]

timshen accepted this revision.
timshen added a comment.
This revision is now accepted and ready to land.

Other than a typo, LGTM. I believe that the atomics are correct as well, especially in the assembly - x86 load-acquire is indeed a no-op.



================
Comment at: lib/xray/xray_fdr_logging_impl.h:483
+
+inline bool isLogInitializedAndRedy(
+    std::shared_ptr<BufferQueue> &LocalBQ, uint64_t TSC, unsigned char CPU,
----------------
s/Redy/Ready/ :)


================
Comment at: lib/xray/xray_trampoline_x86_64.S:205
+  .cfi_startproc
+	pushq %rbp
+	.cfi_def_cfa_offset 16
----------------
dberris wrote:
> timshen wrote:
> > dberris wrote:
> > > timshen wrote:
> > > > Why stash rbp, if we don't clobber it?
> > > I can't fully explain it, but if I don't stash this (seems to be the frame pointer) then I've run into issues in the past. I can try removing it, and see how that goes.
> > Do you have any clue about the rbp? I believe that rbp is caller-saved, therefore if __xray_CustomEvent doesn't use it, it doesn't have to save it.
> So I looked this up, and it seems that the frame pointer in x86_64 is usually stored in %rbp -- and if I don't put this on the stack, we run into issues locating the frame pointer when unwinding and debugging. If the frame pointer is potentially omitted, we can't know so we're being safe anyway in this case by stashing it regardless.
> 
> Does this make sense?
Yes! This reminds me of -fomit-frame-pointer.

I think it's good save rbp.


https://reviews.llvm.org/D30630