[PATCH] D71786: RFC: [Support] On Windows, add optional support for rpmalloc

[Reid Kleckner via Phabricator via llvm-commits]

rnk added subscribers: inglorion, gbiv.
rnk added a comment.
Herald added a subscriber: george.burgess.iv.

> It currently uses /MT because it is easier that way,

Based on my experience with Windows ASan, yes, let's limit this to /MT[d]. Malloc interception gets very hard otherwise.

+ thinlto users: @gbiv @inglorion



================
Comment at: llvm/CMakeLists.txt:337
+    message(FATAL_ERROR "LLVM_ENABLE_RPMALLOC only works with /MT or /MTd. Use LLVM_USE_CRT_${uppercase_CMAKE_BUILD_TYPE} to set the appropriate option.")
+  endif()
+  set(ENABLE_RPMALLOC 1)
----------------
The malloc interception will 100% interfere with sanitizers. I think it's worth preemptively defending against that by erroring out if LLVM_USE_SANITIZER is non-empty and we are using rpmalloc.


================
Comment at: llvm/include/llvm/Config/config.h.cmake:356-357
 
+/* When enabled, overrides the Windows CRT allocator with rpmalloc. */
+#cmakedefine01 ENABLE_RPMALLOC
+
----------------
I think this should be LLVM_ENABLE_RPMALLOC, even though it is an internal option (not in llvm-config.h.cmake).


================
Comment at: llvm/lib/Support/Windows/Memory.inc:219
+    char PageSizeStr[16];
+    // Use a Win32 API call because o::getenv uses the CRT, and at
+    // this point it isn't initialized yet
----------------
I guess it should just be `::getenv` instead of `o::getenv`.


================
Comment at: llvm/lib/Support/Windows/rpmalloc/malloc.c:1
+/* malloc.c  -  Memory allocator  -  Public Domain  -  2016 Mattias Jansson
+ *
----------------
Since this code is portable, I wonder if it would be better to move it to lib/Support/rpmalloc. I would expect it essentially work out of the box on Linux. I have heard that teams at Google found that tcmalloc greatly speeds up clang, even in a non-threaded context. We can let folks experiment with that use case.


================
Comment at: llvm/unittests/Support/DynamicLibrary/CMakeLists.txt:42-45
+  # We need to link in the Support lib for the Memory allocator override,
+  # otherwise the DynamicLibrary.Shutdown test will fail, because it would
+  # allocate memory with the CRT allocator, and release it with our custom
+  # allocator (see llvm/lib/Support/Windows/Memory.inc).
----------------
What an exciting failure mode. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71786