[PATCH] D46998: [XRay][compiler-rt] Remove reliance on C++ ABI features

Dean Michael Berris via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 31 07:21:30 PDT 2018


dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_basic_logging.cc:85
+  pthread_once(&DetectOnce, +[] {
+    if (atomic_load(&UseRealTSC, memory_order_relaxed))
+      atomic_store(&CycleFrequency, getTSCFrequency(), memory_order_release);
----------------
eizan wrote:
> Why do we need to use atomic_load with a custom memory order rather than just using the default provided by operator=?
`UseRealTSC` isn't a std::atomic<...> which will have the `operator=` overload. We're using the types in sanitizer_common/sanitizer_atomic.h here, which require that loads and stores have an explicit memory order.


================
Comment at: compiler-rt/lib/xray/xray_basic_logging.cc:412
     GlobalOptions.MaxStackDepth = F.max_stack_depth;
+    *basicFlags() = F;
   } else if (OptionsSize != sizeof(BasicLoggingOptions)) {
----------------
eizan wrote:
> Is this a bugfix?
You could call it that, though we don't have failing tests that indicate we've hit this. :)


================
Comment at: compiler-rt/lib/xray/xray_basic_logging.cc:428-431
+  __xray_set_handler_arg1(atomic_load(&UseRealTSC, memory_order_relaxed)
+                              ? basicLoggingHandleArg1RealTSC
+                              : basicLoggingHandleArg1EmulateTSC);
+  __xray_set_handler(atomic_load(&UseRealTSC, memory_order_relaxed)
----------------
kpw wrote:
> The pthread once block that initialized UseRealTSC doesn't have to be this thread. Don't we need synchronization (memory_order_acquire) to make sure it's visible?
I think a stronger memory order here doesn't hurt -- but thinking about this a little, the global synchronisation on `BasicInitialized` just allows us to have one of these functions running at the same time. These loads will only happen in either:

- The first thread that gets to run this, in which case we get the once initialisation done before any of these loads.

- Another thread that will be able to run this function will always see the release stores commit.

Note that this assumes x86, which is a bad assumption to make. So you're right, I should be using stronger loads here.


https://reviews.llvm.org/D46998





More information about the llvm-commits mailing list