[PATCH] D116139: [ORC-RT] Add debug logging macros.

[Ben Langmuir via Phabricator via llvm-commits]

benlangmuir added inline comments.


================
Comment at: compiler-rt/lib/orc/debug.cpp:28
+    DebugTypes = DT;
+  if (getenv("ORC_RT_DEBUG"))
+    DebugState = DebugStates::Enabled;
----------------
lhames wrote:
> benlangmuir wrote:
> > I'm not familiar with the history of `-debug` vs. `-debug-only`.  What's the reason for not just basing this on a single value -- is `ORC_RT_DEBUG_TYPES` empty or not?
> Using the empty-string feels counter-intuitive  -- I think people are going to write:
> `ORC_RT_DEBUG=1` and get confused when it doesn't print anything.
> 
> What if we reserve '1', 'true', 'on', or 'all' for "log everything"? 
> I think people are going to write: ORC_RT_DEBUG=1 and get confused when it doesn't print anything.

This the status quo for both this patch and the `-debug` flag in llvm though.  It's already confusing that you need to set two independent things to get any output.  That's why I was wondering if we should just check `ORC_RT_DEBUG_TYPES` (the _TYPES one), which I think makes it clearer that it's not just a boolean flag.  But if there's some good reason to keep them separate, that's fine with me too, I was mostly curious why we have this two-part configuration.


================
Comment at: compiler-rt/lib/orc/debug.h:33
+    if (::__orc_rt::DebugState == DebugStates::Uninitialized)                  \
+      initializeDebug();                                                       \
+    if (::__orc_rt::DebugState == DebugStates::Enabled &&                      \
----------------
lhames wrote:
> benlangmuir wrote:
> > If we really need to initialize it here, we could skip the second atomic load in the common case that it's already initialized
> > 
> > ```
> > auto State = ::__orc_rt::DebugState.load(memory_order_acquire);
> > if (State == DebugStates::Uninitialized) {
> >   initializeDebug();
> >   State = ::__orc_rt::DebugState.load(memory_order_relaxed);
> > }
> > if (State == DebugStates::Enabled && ...
> > ```
> > 
> > Or to improve code size, initialize `DebugState` to `Enabled` and put a `std::call_once` in `isCurrentDebugType` to actually initialize it the first time.
> > 
> > We could also get away with `load(memory_order_acquire)` here instead of the default sequentially consistent.  Or perhaps even `load(memory_order_relaxed)` if we're willing to accept a stale value of DebugTypes if there's contention during initialization.
> It's a debug macro and the ORC runtime isn't likely to contain any too-hot code. I guess one think that we need to watch out for is the possibility that relying on sequential consistency for logging checks might obscure ordering bugs? 
Okay, I'm not going to try to hold this patch up over this, but I would suggest at least refactoring to only do a single atomic load in the common case that you've already done the initialization, and preferably using acquire or relaxed ordering.

> I guess one think that we need to watch out for is the possibility that relying on sequential consistency for logging checks might obscure ordering bugs?

I suspect the slowness of the logging itself would be a bigger issue for reproducibility of bugs unless you're running under tsan.  Or are you thinking of when logging is disabled? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116139/new/

https://reviews.llvm.org/D116139