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

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop 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$")
----------------
aganea wrote:
> 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}")
> ```
> Ah I see - would that trigger any asserts in scudo?

I don't think assertions, but errors.

>  In that case, should we add the second lib below? 

I tried similar to and it fails with a multiply defined symbol error:

```
clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: "void * __cdecl operator new(unsigned __int64)" (??2 at YAPEAX_K@Z) already defined in libclang.lib(libclang.dll)
clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: "void * __cdecl operator new(unsigned __int64,struct std::nothrow_t const &)" (??2 at YAPEAX_KAEBUnothrow_t@std@@@Z) already defined in libclang.lib(libclang.dll)
clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: "void __cdecl operator delete(void *)" (??3 at YAXPEAX@Z) already defined in libclang.lib(libclang.dll)
clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: "void __cdecl operator delete(void *,unsigned __int64)" (??3 at YAXPEAX_K@Z) already defined in libclang.lib(libclang.dll)
clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: "void * __cdecl operator new[](unsigned __int64)" (??_U at YAPEAX_K@Z) already defined in libclang.lib(libclang.dll)
clang_rt.scudo_cxx-x86_64.lib(scudo_new_delete.cpp.obj) : error LNK2005: "void __cdecl operator delete[](void *)" (??_V at YAXPEAX@Z) already defined in libclang.lib(libclang.dll)
   Creating library lib\c-index-test.lib and object lib\c-index-test.exp
bin\c-index-test.exe : fatal error LNK1169: one or more multiply defined symbols found
```

I'm not sure whether it is integrated correctly for the rest of the executables. I can take another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96133