[PATCH] D96133: Allow building with scudo memory allocator on Windows

Fri Feb 5 09:36:52 PST 2021

russell.gallop added inline comments.

================
Comment at: llvm/docs/CMake.rst:496
+
+    $ D:\llvm-project> cmake ... -DLLVM_INTEGRATED_CRT_ALLOC=<stage1>\lib\clang\<version>\lib\windows\clang_rt.scudo-x86_64.lib
+
----------------
aganea wrote:
> How did you end up fixing the new/delete link issues in https://reviews.llvm.org/D86694#2463429 ?
-fsanitize adds both to the link if going via the clang driver: https://reviews.llvm.org/D96120#change-6VXGONdzQcwB

This works for the lit tests, but -fsanitize doesn't work for building LLVM on Windows as the link steps don't go via the compiler, and wouldn't work with cl.exe.

================
Comment at: llvm/lib/Support/CMakeLists.txt:70
     add_definitions(-DENABLE_OVERRIDE -DENABLE_PRELOAD)
     set(ALLOCATOR_FILES "${LLVM_INTEGRATED_CRT_ALLOC}/rpmalloc/rpmalloc.c")
   elseif(LLVM_INTEGRATED_CRT_ALLOC MATCHES "snmalloc$")
----------------
aganea wrote:
> Strangely we don't `#include <rpnew.h>`[1] anywhere when activating rpmalloc, and that still works (I don't see calls to Win32 Heap functions in the profile traces, only calls to rpmalloc functions). I think this is because the default behavior of the Windows CRT new/delete is to call malloc/free. So I'm not quite sure in which situation we need `clang_rt.scudo_cxx` / if it is needed here?
> 
> [1] https://github.com/mjansson/rpmalloc/blob/develop/rpmalloc/rpnew.h
Yes, I don't think that they're "required" for scudo to take over memory allocations via malloc. I believe that Windows new then goes to malloc which ends up with scudoAllocate.

> So I'm not quite sure in which situation we need clang_rt.scudo_cxx / if it is needed here?

AllocType mismatch detection won't work between new->free and malloc->delete.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96133