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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
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$")
----------------
russell.gallop wrote:
> 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.
Ah I see - would that trigger any asserts in scudo? (if scudo was compiled with `-DLLVM_ENABLE_ASSERTIONS=ON`). In that case, should we add the second lib below? ie.
```STRING(REGEX REPLACE "(.+)\\.scudo-(.+)" "\\1.scudo_cxx-\\2" LLVM_INTEGRATED_CRT_ALLOC2 ${LLVM_INTEGRATED_CRT_ALLOC})
set(system_libs ${system_libs} "${LLVM_INTEGRATED_CRT_ALLOC2}")
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96133