Remove the NotUnderValgrind caching flag

[mehdi_amini]

Summary

The motivation for this caching wasn't clear, remove it in an effort to
simplify the code and make libSupport free of global dynamic constructor.

[MaskRay]

Perhaps we should just get rid of the file. It (2010) seems to be related to the old JIT lib/Target/X86/X86JITInfo.cpp which was deleted in 2014. The file has another usage in bugpoint.cpp to tune a parameter which appears not to useful.

[mehdi_amini]

I have the impression that it can be called transitively from Orc as well, through calls to protectMappedMemory

[lhames]

This code predates my involvement in the JIT by half a decade or more. ;)

It's definitely reachable, but I'm not sure whether anyone actually relies on it any more. My first impression is that running Valgrind on JIT'd code would be pretty rare these days. Maybe we just remove it after we branch for LLVM 13 in a week or so?

[vchuravy]

@loladiro does this impact us? Julia does support running under valgrind, see https://docs.julialang.org/en/v1/devdocs/valgrind/,
and IIRC that extends to JIT'ed code.

[vtjnash]

There doesn't seem to be a reason to make the VALGRIND_DISCARD_TRANSLATIONS call conditional. The macro is already a specific sequence of bytes (e.g. rol instructions) that are a no-op if valgrind is not running the particular segment of code inside its own JIT

[dexonsmith]

llvm/lib/Support/Valgrind.cpp

21–24

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.

26–27

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...

[dexonsmith]

llvm/lib/Support/Valgrind.cpp

21–24

(Based on https://www.valgrind.org/docs/manual/manual-core-adv.html I strongly doubt load-time is special in any way...)

[mehdi_amini]

llvm/lib/Support/Valgrind.cpp

21–24

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?

[mehdi_amini]

In D106206#2887844, @vtjnash wrote:

There doesn't seem to be a reason to make the VALGRIND_DISCARD_TRANSLATIONS call conditional. The macro is already a specific sequence of bytes (e.g. rol instructions) that are a no-op if valgrind is not running the particular segment of code inside its own JIT

Right, I'm happy to simplify all of this code and just rely on the Valgrind macros. It's a case of Chesterton's fence really: the code was added as-is >10 years ago and it isn't clear what behavior is relied on.

[dexonsmith]

llvm/lib/Support/Valgrind.cpp

21–24

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.

26–27

If the caching is removed it's probably worth following @vtjnash's suggestion to remove the explicit check here (since it's apparently redundant).

[mehdi_amini]

llvm/lib/Support/Valgrind.cpp

21–24

No I had looked and it was added as part of the original revision: https://github.com/llvm/llvm-project/commit/3ddd88f523defacb922ea53e64fec2bdf25f2896#diff-d5d0a3cfb6f89c8c0d2367f426b48e438acce79ff3ddaff17c3ed1ada2ae5693

[dexonsmith]

llvm/lib/Support/Valgrind.cpp

21–24

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.

[mehdi_amini]

Nuking these methods and running the LLVM tests with Valgrind still worked: bin/llvm-lit ../llvm/test/ --vg

Maybe we can delete this entirely? Can one of the Julia folks look if you need this (and provide a test)?

[mehdi_amini]

Remove the "caching" flag entirely for now

[mehdi_amini]

Remove unused include now

[dexonsmith]

Updated patch LGTM, with a couple of nits inline.

(Agreed that it'd be great for Julia to write a test for the LLVM JIT that confirms it behaves correctly under Valgrind.)

llvm/lib/Support/Valgrind.cpp

17

You can drop this now.

22–23

Probably clang-format will want to delete some newlines here.

[mehdi_amini]

llvm/lib/Support/Valgrind.cpp

22–23

I have a clang-format running automatically on arc diff ;)

I think the lines appear empty here because of the removal on the left hand side, but if you look at the line numbers there aren't any blank line I think?

[dexonsmith]

llvm/lib/Support/Valgrind.cpp

22–23

Aha, yeah, that explains it :).

llvm/lib/Support/Valgrind.cpp

 //===-- Valgrind.cpp - Implement Valgrind communication ---------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  Defines Valgrind communication methods, if HAVE_VALGRIND_VALGRIND_H is
 //  defined.  If we have valgrind.h but valgrind isn't running, its macros are
 //  no-ops.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Valgrind.h"
 #include "llvm/Config/config.h"
 #include <cstddef>
 
 #if HAVE_VALGRIND_VALGRIND_H
 #include <valgrind/valgrind.h>
 
-static bool InitNotUnderValgrind() {
-  return !RUNNING_ON_VALGRIND;
-}
-
-// This bool is negated from what we'd expect because code may run before it
-// gets initialized.  If that happens, it will appear to be 0 (false), and we
-// want that to cause the rest of the code in this file to run the
-// Valgrind-provided macros.
-static const bool NotUnderValgrind = InitNotUnderValgrind();
-
 bool llvm::sys::RunningOnValgrind() {
-  if (NotUnderValgrind)
-    return false;
   return RUNNING_ON_VALGRIND;
 }
 
 void llvm::sys::ValgrindDiscardTranslations(const void *Addr, size_t Len) {
-  if (NotUnderValgrind)
-    return;
-
   VALGRIND_DISCARD_TRANSLATIONS(Addr, Len);
 }
 
 #else  // !HAVE_VALGRIND_VALGRIND_H
 
 bool llvm::sys::RunningOnValgrind() {
   return false;
 }
 
 void llvm::sys::ValgrindDiscardTranslations(const void *Addr, size_t Len) {
 }
 
 #endif  // !HAVE_VALGRIND_VALGRIND_H