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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: lib/xray/xray_fdr_logging.cc:235
+      uint8_t(MetadataRecord::RecordKinds::CustomEventMarker);
+  std::memcpy(&CustomEvent.Data, &std::get<0>(TSC_CPU), sizeof(uint64_t));
+  std::memcpy(&CustomEvent.Data[sizeof(uint64_t)], &ReducedEventSize,
----------------
timshen wrote:
> s/sizeof(uint64_t)/64/? By definition. :)
Actually, sizeof(uint64_t) is 8. However, we're doing this to try and match what the type of the element is. I've made it a local constant instead, to give it a name.


================
Comment at: lib/xray/xray_fdr_logging.cc:257
+
+  FDROptions.reset(new FDRLoggingOptions());
+  memcpy(FDROptions.get(), Options, OptionsSize);
----------------
timshen wrote:
> Does FDROptions really need to be a unique_ptr? How about just using the value type?
Good point. Doesn't need to be a unique_ptr. Turned it into a global instead, protected with a spinlock.


================
Comment at: lib/xray/xray_trampoline_x86_64.S:205
+  .cfi_startproc
+	pushq %rbp
+	.cfi_def_cfa_offset 16
----------------
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?


https://reviews.llvm.org/D30630