Page MenuHomePhabricator

Allow building with scudo memory allocator on Windows
Needs ReviewPublic

Authored by russell.gallop on Feb 5 2021, 6:30 AM.

Details

Reviewers
aganea
cryptoad
Summary

Following on from https://reviews.llvm.org/D96120, this allows scudo to be used as a memory allocator for Windows.

For example, adding:

-DLLVM_INTEGRATED_CRT_ALLOC="<folder>/llvm-project/stage1/lib/clang/13.0.0/lib/windows/clang_rt.scudo-x86_64.lib"

Note that -DLLVM_INTEGRATED_CRT_ALLOC only allows hooking in C library so this doesn't replace C++ new/delete etc.

check-all passes with scudo. Intermittently see failures in:

ExecutionEngine/MCJIT/test-global-init-nonzero-sm-pic.ll
ExecutionEngine/MCJIT/test-ptr-reloc-sm-pic.ll
ExecutionEngine/MCJIT/multi-module-sm-pic-a.ll
ExecutionEngine/MCJIT/cross-module-sm-pic-a.ll

Which appear to be due to https://bugs.llvm.org/show_bug.cgi?id=24978 (see analysis here: https://reviews.llvm.org/D86694#2539630). This bug happens more frequently with Scudo due to the difference in allocated addresses.

Diff Detail

Unit TestsFailed

TimeTest
490 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::strcmp.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/asan/TestCases/strcmp.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/strcmp.c.tmp
120 msx64 debian > Clang-Unit.ASTMatchers/_/ASTMatchersTests::ASTMatchersTest.Finder_DynamicOnlyAcceptsSomeMatchers
Note: Google Test filter = ASTMatchersTest.Finder_DynamicOnlyAcceptsSomeMatchers [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
360 msx64 debian > libarcher.races::task-dependency.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c
420 msx64 debian > libarcher.races::task-taskgroup-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c
420 msx64 debian > libarcher.races::task-taskwait-nested.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c
View Full Test Results (16 Failed)

Event Timeline

russell.gallop created this revision.Feb 5 2021, 6:30 AM
russell.gallop requested review of this revision.Feb 5 2021, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 6:30 AM

Add documentation to CMake.rst

aganea added inline comments.Feb 5 2021, 9:09 AM
llvm/docs/CMake.rst
496

How did you end up fixing the new/delete link issues in https://reviews.llvm.org/D86694#2463429 ?

llvm/lib/Support/CMakeLists.txt
70

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

russell.gallop added inline comments.Feb 5 2021, 9:36 AM
llvm/docs/CMake.rst
496

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

llvm/lib/Support/CMakeLists.txt
70

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.

aganea added inline comments.Feb 5 2021, 10:47 AM
llvm/lib/Support/CMakeLists.txt
70

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}")
russell.gallop added inline comments.Feb 9 2021, 10:00 AM
llvm/lib/Support/CMakeLists.txt
70

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