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

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: lib/xray/xray_fdr_logging.cc:202
+  if (!Guard) {
+    assert(Running == true && "RecursionGuard is buggy!");
+    return;
----------------
Running &&


================
Comment at: lib/xray/xray_fdr_logging.cc:206
+  if (EventSize > std::numeric_limits<int32_t>::max()) {
+    bool Once = [&] {
+      Report("Event size too large = %zu ; > max = %d\n", EventSize,
----------------
static.

Also, it doesn't have to be a bool. An empty struct or tuple<> will do.

Please also check other places.


================
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,
----------------
s/sizeof(uint64_t)/64/? By definition. :)


================
Comment at: lib/xray/xray_fdr_logging.cc:257
+
+  FDROptions.reset(new FDRLoggingOptions());
+  memcpy(FDROptions.get(), Options, OptionsSize);
----------------
Does FDROptions really need to be a unique_ptr? How about just using the value type?


================
Comment at: lib/xray/xray_fdr_logging_impl.h:464
 
-static inline void processFunctionHook(
-    int32_t FuncId, XRayEntryType Entry, uint64_t TSC, unsigned char CPU,
-    int (*wall_clock_reader)(clockid_t, struct timespec *),
-    __sanitizer::atomic_sint32_t &LoggingStatus,
-    const std::shared_ptr<BufferQueue> &BQ) XRAY_NEVER_INSTRUMENT {
+static inline bool prepareBuffer(int (*wall_clock_reader)(clockid_t,
+                                                          struct timespec *),
----------------
I don't usually see "static inline" around. Usually static defeats the purpose of inline. I suggest to remove static.


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


https://reviews.llvm.org/D30630