[PATCH] D106206: Lazy initialized the NotUnderValgrind flag

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/lib/Support/Valgrind.cpp:27-38
+namespace {
+struct CreateNotUnderValgrind {
+  static void *call() { return new bool{!RUNNING_ON_VALGRIND}; }
+};
+} // namespace
+static const llvm::ManagedStatic<bool, CreateNotUnderValgrind> NotUnderValgrind;
 
----------------
mehdi_amini wrote:
> dexonsmith wrote:
> > mehdi_amini wrote:
> > > dexonsmith wrote:
> > > > dexonsmith wrote:
> > > > > Can `NotUnderValgrind` be merged into `RunningOnValgrind`, skipping the managed static?
> > > > > 
> > > > > The only reason not to is if it `RUNNING_ON_VALGRIND` could return "true" on the first use, but "false" some time later in the same execution. I don't imagine that's possible...?
> > > > > 
> > > > > ... but if it //is// possible, maybe switching to ManagedStatic isn't safe either, since there would be a reason to cache the value at load-time instead of at first use.
> > > > (Based on https://www.valgrind.org/docs/manual/manual-core-adv.html I strongly doubt load-time is special in any way...)
> > > Yeah I'm confused by all this logic, and I didn't get the comment either. I don't get why we don't just `return RUNNING_ON_VALGRIND` and forget about caching entirely?
> > Maybe there was a time when it wasn't fast? Might be worth a quick look at git-blame to see if this was in response to a specific performance problem. Might be better to remove the caching entirely.
> No I had looked and it was added as part of the original revision: https://github.com/llvm/llvm-project/commit/3ddd88f523defacb922ea53e64fec2bdf25f2896#diff-d5d0a3cfb6f89c8c0d2367f426b48e438acce79ff3ddaff17c3ed1ada2ae5693
Based on the commit message, I'd bet the author just assumed it was slow and needed caching. Maybe it even did at the time, but the current valgrind docs imply that it's fast (six instructions IIUC). I'd be in favour of calling the macros directly without caching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106206