[PATCH] D96120: [scudo] Port scudo sanitizer to Windows

Fri Feb 26 09:37:33 PST 2021

russell.gallop marked 2 inline comments as done.
russell.gallop added a comment.

In D96120#2550941 <https://reviews.llvm.org/D96120#2550941>, @mstorsjo wrote:

> In D96120#2550876 <https://reviews.llvm.org/D96120#2550876>, @russell.gallop wrote:
>
>> In D96120#2546077 <https://reviews.llvm.org/D96120#2546077>, @mstorsjo wrote:
>>
>>> As is, this breaks compilation for mingw. With the three modifications I suggest here, it no longer breaks compilation for me - I have no idea if it actually works in mingw configurations though, but not breaking compilation is at least the first step.
>>
>> Thanks for the information I'll try to run up a mingw environment and check it works.
>
> In case that turns out to be tricky, I might be able to help with that, at least for building a simple test program with it and running it, if you say how it's supposed to be used  (building/linking with `-fsanitize=scudo`?) and how to inspect whether it actually works.

Hi @mstorsjo. Thanks for the suggestions. I tried running up an mingw environment with msys but had trouble getting it working (running into cmake issues). Would you be able to help?

Yes, building and linking a program with -fsanitize=scudo is the simple way to build a simple program. The support for this is in clang/lib/Driver/ToolChains/MSVC.cpp. Would this require support in clang/lib/Driver/ToolChains/MinGW.cpp?

I think the best way to test is to run the scudo LIT tests with (e.g.) "ninja check-scudo". These build and run some simple test programs and check that problems are detected.

Beyond this I built LLVM with scudo and ran check-all. I don't know whether you want to go this far. Note that -fsanitize=scudo doesn't work for the MSVC LLVM build which uses link/lld-link for linking rather than going via the compiler driver so I hooked it into LLVM_INTEGRATED_CRT_ALLOC (see https://reviews.llvm.org/D96133). If mingw uses the compiler driver to link (like Linux) then you may be able to use -DLLVM_USE_SANITIZER=Scudo on a stage2 build. This would require some changes to llvm/cmake/modules/HandleLLVMOptions.cmake to permit it (I had similar changes in https://reviews.llvm.org/D86694). It's harder to tell whether this has worked correctly. With MSVC faster ThinLTO links on multi-core machines are a good indicator but I guess mingw wouldn't be using the MSVC allocator.

Thanks
Russ

================
Comment at: compiler-rt/lib/scudo/scudo_new_delete.cpp:30
+#ifdef _WIN64
+COMMENT_EXPORT("??2 at YAPEAX_K@Z")                    // operator new
+COMMENT_EXPORT("??2 at YAPEAX_KAEBUnothrow_t@std@@@Z") // operator new nothrow
----------------
mstorsjo wrote:
> These don't work in this form for mingw targets (which use the itanium c++ abi).
> 
> By changing `#if SANTIZER_WINDOWS` into `#if SANITIZER_WINDOWS && !defined(__MINGW32__)`, I fixed the linker errors at least.
I've applied this suggestion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120