[PATCH] D106206: Lazy initialized the NotUnderValgrind flag
Duncan P. N. Exon Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 19 16:46:48 PDT 2021
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:
> > 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.
================
Comment at: llvm/lib/Support/Valgrind.cpp:41-42
void llvm::sys::ValgrindDiscardTranslations(const void *Addr, size_t Len) {
- if (NotUnderValgrind)
+ if (*NotUnderValgrind)
return;
----------------
dexonsmith wrote:
> Seems to me like this could use `RunningOnValgrind`, unless there's a case where `RUNNING_ON_VALGRIND` returns different answers at load-time vs later...
If the caching is removed it's probably worth following @vtjnash's suggestion to remove the explicit check here (since it's apparently redundant).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106206/new/
https://reviews.llvm.org/D106206
More information about the llvm-commits
mailing list