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

[Mitch Phillips via Phabricator via cfe-commits]

hctim added a comment.

In D96120#2615085 , @russell.gallop 
wrote:

> In D96120#2597877 , @kcc wrote:
>
>> I am afraid we will have to delete the older (non-standalone) variant 
>> entirely. 
>> (And the sooner the better)
>
> Okay, think we found a few things out by doing this before porting standalone.
>
> If I find time I’ll have a go at removing the sanitizer version.
>
> The scudo documentation (https://llvm.org/docs/ScudoHardenedAllocator.html) 
> still seems to refer to the sanitizer version so will need updating.

When we remove the non-standalone variant, it would be great to move 
`-fsanitize=scudo` over to scudo-standalone :).


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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop abandoned this revision.
russell.gallop added a comment.

In D96120#2597877 , @kcc wrote:

> I am afraid we will have to delete the older (non-standalone) variant 
> entirely. 
> (And the sooner the better)

Okay, think we found a few things out by doing this before porting standalone.

If I find time I’ll have a go at removing the sanitizer version.

The scudo documentation (https://llvm.org/docs/ScudoHardenedAllocator.html) 
still seems to refer to the sanitizer version so will need updating.


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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:489
 
+  if (TC.getSanitizerArgs().needsScudoRt()) {
+for (const auto  : {"scudo", "scudo_cxx"}) {

We need this for both legacy and standalone scudo?





Comment at: compiler-rt/lib/scudo/scudo_platform.h:19
 
-#if !SANITIZER_LINUX && !SANITIZER_FUCHSIA
+#if !SANITIZER_LINUX && !SANITIZER_FUCHSIA && !SANITIZER_WINDOWS
 # error "The Scudo hardened allocator is not supported on this platform."

For the reason @kcc mentioned, if possible could you not update legacy scudo at 
at all, and keep only stuff needed for standalone version?

BTW. Patches to remove legacy one are welcomed.


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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kostya Serebryany via Phabricator via cfe-commits]

kcc added reviewers: kcc, pcc, vitalybuka.
kcc added a comment.

We can't possibly maintain two variants of scudo. 
All effort is currently spent on the newer (standalone) version. 
I am afraid we will have to delete the older (non-standalone) variant entirely. 
(And the sooner the better)


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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2591368 , @mstorsjo wrote:

> Yes, it would need something similar - I tried whipping something together, 
> which after some tweaks seems to work:
> Feel free to squash that into your patch (which saves me a bit of effort) :-)

Thanks for doing that. I've added this in.

> ...I did test building the double-free.cpp test with `-fsanitize=scudo`, and 
> it seems to work as it should for all three cases it tests...

Great, thanks.


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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 327123.
russell.gallop added a comment.

Update with suggested changes to MinGW.cpp


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

https://reviews.llvm.org/D96120

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c

Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test
===
--- compiler-rt/test/scudo/symbols.test
+++ compiler-rt/test/scudo/symbols.test
@@ -1,4 +1,4 @@
-UNSUPPORTED: android
+UNSUPPORTED: android, windows
 
 Verify that various functions are *not* present in the minimal binary. Presence
 of those symbols in the minimal runtime would mean that the split code made it
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
+#include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#else
+#include 
 #include 
+#endif
 
+#ifdef _WIN32
+DWORD getsystempagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo) {
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long getsystempagesize() {
+  return sysconf(_SC_PAGESIZE);
+}
+#endif
 
 int main(int argc, char **argv)
 {
   // The size must be large enough to be serviced by the secondary allocator.
-  long page_size = sysconf(_SC_PAGESIZE);
-  size_t size = (1U << 17) + page_size;
-  struct sigaction a;
+  long page_size = getsystempagesize();
+  size_t size = (1U << 19) + page_size;
 
   assert(argc == 2);
-  memset(, 0, sizeof(a));
-  a.sa_sigaction = handler;
-  a.sa_flags = SA_SIGINFO;
 
   char *p = (char *)malloc(size);
   assert(p);
   memset(p, 'A', size); // This should not trigger anything.
   // Set up the SIGSEGV handler now, as the rest should trigger an AV.
+#ifdef _WIN32
+  SetUnhandledExceptionFilter(handler);
+#else
+  struct sigaction a = {0};
+  a.sa_sigaction = handler;
+  a.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, , NULL);
+#endif
+
   if (!strcmp(argv[1], 

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2591418 , @vitalybuka wrote:

>>> This is intended as a step to porting scudo standalone.
>
> Why this is needed for scudo stadalone?

It’s not strictly needed. It seemed like it was easier, as some of the work had 
already been done, and some of the issues worked through on this would probably 
also come up when porting standalone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

>> This is intended as a step to porting scudo standalone.

Why this is needed for scudo stadalone?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D96120#2590587 , @russell.gallop 
wrote:

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

Yeah, building things in that environment is a bit challenging.

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

Yes, it would need something similar - I tried whipping something together, 
which after some tweaks seems to work:

  diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp 
b/clang/lib/Driver/ToolChains/MinGW.cpp
  index f6cead412236..aab141377204 100644
  --- a/clang/lib/Driver/ToolChains/MinGW.cpp
  +++ b/clang/lib/Driver/ToolChains/MinGW.cpp
  @@ -260,6 +260,14 @@ void tools::MinGW::Linker::ConstructJob(Compilation , 
const JobAction ,
   }
 }
   
  +  if (Sanitize.needsScudoRt()) {
  +for (const auto  : {"scudo", "scudo_cxx"}) {
  +  CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
  +}
  +CmdArgs.push_back("--require-defined");
  +CmdArgs.push_back(Args.MakeArgString("malloc"));
  +  }
  +
 AddLibGCC(Args, CmdArgs);
   
 if (Args.hasArg(options::OPT_pg))
  @@ -492,6 +500,7 @@ SanitizerMask toolchains::MinGW::getSupportedSanitizers() 
const {
 Res |= SanitizerKind::PointerCompare;
 Res |= SanitizerKind::PointerSubtract;
 Res |= SanitizerKind::Vptr;
  +  Res |= SanitizerKind::Scudo;
 return Res;
   }

Feel free to squash that into your patch (which saves me a bit of effort) :-)

(First I had placed this later in the function, after the asan bits, but in 
that case, it ended up linking against the normal malloc function instead of 
the one from scudo - the libraries that provide malloc end up added via the 
`AddLibGCC()` call.

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

I didn't try this right now (I primarily cross compile so running tests for the 
runtimes is a bit challenging), but I did test building the double-free.cpp 
test with `-fsanitize=scudo`, and it seems to work as it should for all three 
cases it tests, so with that it seems to be at least roughly working, so I 
think that's good enough to include the clang driver bits in 
ToolChains/MinGW.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

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

In D96120#2550941 , @mstorsjo wrote:

> In D96120#2550876 , @russell.gallop 
> wrote:
>
>> In 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@YAPEAX_K@Z")// operator new
+COMMENT_EXPORT("??2@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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 326716.
russell.gallop added a comment.

Added comment on AllocatorSize.

Applied Mingw changes suggested by @mstorsjo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c

Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test
===
--- compiler-rt/test/scudo/symbols.test
+++ compiler-rt/test/scudo/symbols.test
@@ -1,4 +1,4 @@
-UNSUPPORTED: android
+UNSUPPORTED: android, windows
 
 Verify that various functions are *not* present in the minimal binary. Presence
 of those symbols in the minimal runtime would mean that the split code made it
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
+#include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#else
+#include 
 #include 
+#endif
 
+#ifdef _WIN32
+DWORD getsystempagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo) {
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long getsystempagesize() {
+  return sysconf(_SC_PAGESIZE);
+}
+#endif
 
 int main(int argc, char **argv)
 {
   // The size must be large enough to be serviced by the secondary allocator.
-  long page_size = sysconf(_SC_PAGESIZE);
-  size_t size = (1U << 17) + page_size;
-  struct sigaction a;
+  long page_size = getsystempagesize();
+  size_t size = (1U << 19) + page_size;
 
   assert(argc == 2);
-  memset(, 0, sizeof(a));
-  a.sa_sigaction = handler;
-  a.sa_flags = SA_SIGINFO;
 
   char *p = (char *)malloc(size);
   assert(p);
   memset(p, 'A', size); // This should not trigger anything.
   // Set up the SIGSEGV handler now, as the rest should trigger an AV.
+#ifdef _WIN32
+  SetUnhandledExceptionFilter(handler);
+#else
+  struct sigaction a = {0};
+  a.sa_sigaction = handler;
+  a.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, , NULL);

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D96120#2550876 , @russell.gallop 
wrote:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

In D96120#2545151 , @aganea wrote:

> A few questions: Does this work on x86 targets?

I haven't tried. I'll test this out.

> Are the scudo tests below being built with /MD or with /MT?

The lit tests? They don't specify either. Does that imply /MT?

> Would this change compile/work on MinGW as well?

I'll check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.




Comment at: compiler-rt/lib/sanitizer_common/sanitizer_win.cpp:28
+#define SystemFunction036 NTAPI SystemFunction036
+#include 
+#undef SystemFunction036

The includes need to be all-lowercase for mingw. (The windows SDK isn't 
self-consistent so it can't be used as-is on case sensitive filesystems, thus 
the all-lowercase convention of mingw headers is the only one that works for 
case sentisitivity for now.)



Comment at: compiler-rt/lib/scudo/CMakeLists.txt:18
 append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG -fno-omit-frame-pointer
SCUDO_CFLAGS)
 

This needs an extra `append_list_if(MINGW "${MINGW_LIBRARIES}" 
SCUDO_MINIMAL_DYNAMIC_LIBS)` here (similar to a line in 
compiler-rt/lib/asan/CMakeLists.txt) to fix building in mingw configurations. 
(That's needed because these libs are built with `-nodefaultlibs`, which omits 
a few essential libs.)



Comment at: compiler-rt/lib/scudo/scudo_new_delete.cpp:30
+#ifdef _WIN64
+COMMENT_EXPORT("??2@YAPEAX_K@Z")// operator new
+COMMENT_EXPORT("??2@YAPEAX_KAEBUnothrow_t@std@@@Z") // operator new nothrow

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

A few questions: Does this work on x86 targets? Are the scudo tests below being 
built with /MD or with /MT? Would this change compile/work on MinGW as well?




Comment at: compiler-rt/lib/scudo/scudo_platform.h:72
+#elif SANITIZER_WINDOWS
+const uptr AllocatorSize = 0x40ULL; // 256G.
 # else

It is not clear for readers why some platforms have a value different than 
others. It'd be really nice if there was a short comment explaining that, at 
least for Windows. For Windows at least, the reason is explained here: 
https://reviews.llvm.org/D86694#2277371


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96120

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop created this revision.
russell.gallop added reviewers: cryptoad, aganea.
Herald added subscribers: phosek, mgorny.
russell.gallop requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

Based on https://reviews.llvm.org/D42519, this ports the sanitizer version of 
scudo to Windows.

Passes lit tests and when used as the allocator for LLVM, that passes 
check-all. Have noticed that on LLVM with Scudo, 
https://bugs.llvm.org/show_bug.cgi?id=24978 does intermittently occur when 
running lli tests.

For more details of evaluation see https://reviews.llvm.org/D86694

A separate review will allow hooking this in as the memory allocator for LLVM.

I'm aware that scudo sanitizer version is not under active development. This is 
intended as a step to porting scudo standalone.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96120

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
  compiler-rt/lib/scudo/CMakeLists.txt
  compiler-rt/lib/scudo/scudo_allocator.cpp
  compiler-rt/lib/scudo/scudo_crc32.cpp
  compiler-rt/lib/scudo/scudo_new_delete.cpp
  compiler-rt/lib/scudo/scudo_platform.h
  compiler-rt/lib/scudo/scudo_tsd.h
  compiler-rt/lib/scudo/scudo_tsd_shared.cpp
  compiler-rt/lib/scudo/scudo_tsd_shared.inc
  compiler-rt/test/scudo/cxx_threads.cpp
  compiler-rt/test/scudo/dealloc-race.c
  compiler-rt/test/scudo/fsanitize.c
  compiler-rt/test/scudo/interface.cpp
  compiler-rt/test/scudo/lit.cfg.py
  compiler-rt/test/scudo/malloc.cpp
  compiler-rt/test/scudo/memalign.c
  compiler-rt/test/scudo/mismatch.cpp
  compiler-rt/test/scudo/overflow.c
  compiler-rt/test/scudo/preload.cpp
  compiler-rt/test/scudo/rss.c
  compiler-rt/test/scudo/secondary.c
  compiler-rt/test/scudo/symbols.test
  compiler-rt/test/scudo/threads.c
  compiler-rt/test/scudo/tsd_destruction.c
  compiler-rt/test/scudo/valloc.c

Index: compiler-rt/test/scudo/valloc.c
===
--- compiler-rt/test/scudo/valloc.c
+++ compiler-rt/test/scudo/valloc.c
@@ -2,7 +2,7 @@
 // RUN: %run %t valid   2>&1
 // RUN: not %run %t invalid 2>&1 | FileCheck %s
 // RUN: %env_scudo_opts=allocator_may_return_null=1 %run %t invalid 2>&1
-// UNSUPPORTED: android
+// UNSUPPORTED: android, windows
 
 // Tests that valloc and pvalloc work as intended.
 
Index: compiler-rt/test/scudo/tsd_destruction.c
===
--- compiler-rt/test/scudo/tsd_destruction.c
+++ compiler-rt/test/scudo/tsd_destruction.c
@@ -1,5 +1,6 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %run %t 2>&1
+// UNSUPPORTED: windows
 
 #include 
 #include 
Index: compiler-rt/test/scudo/threads.c
===
--- compiler-rt/test/scudo/threads.c
+++ compiler-rt/test/scudo/threads.c
@@ -1,6 +1,7 @@
 // RUN: %clang_scudo %s -o %t
 // RUN: %env_scudo_opts="QuarantineSizeKb=0:ThreadLocalQuarantineSizeKb=0" %run %t 5 100 2>&1
 // RUN: %env_scudo_opts="QuarantineSizeKb=1024:ThreadLocalQuarantineSizeKb=64" %run %t 5 100 2>&1
+// UNSUPPORTED: windows
 
 // Tests parallel allocations and deallocations of memory chunks from a number
 // of concurrent threads, with and without quarantine.
Index: compiler-rt/test/scudo/symbols.test
===
--- compiler-rt/test/scudo/symbols.test
+++ compiler-rt/test/scudo/symbols.test
@@ -1,4 +1,4 @@
-UNSUPPORTED: android
+UNSUPPORTED: android, windows
 
 Verify that various functions are *not* present in the minimal binary. Presence
 of those symbols in the minimal runtime would mean that the split code made it
Index: compiler-rt/test/scudo/secondary.c
===
--- compiler-rt/test/scudo/secondary.c
+++ compiler-rt/test/scudo/secondary.c
@@ -6,37 +6,60 @@
 // allocated by the Secondary allocator, or writing too far in front of it.
 
 #include 
-#include 
-#include 
+#include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#else
+#include 
 #include 
+#endif
 
+#ifdef _WIN32
+DWORD getsystempagesize() {
+  SYSTEM_INFO si;
+  GetSystemInfo();
+  return si.dwPageSize;
+}
+LONG WINAPI handler(EXCEPTION_POINTERS *ExceptionInfo) {
+  fprintf(stderr, "AccessViolation\n");
+  ExitProcess(0);
+}
+#else
 void handler(int signo, siginfo_t *info, void *uctx) {
   if (info->si_code == SEGV_ACCERR) {
-fprintf(stderr, "SCUDO SIGSEGV\n");
+fprintf(stderr, "AccessViolation\n");
 exit(0);
   }
   exit(1);
 }
+long getsystempagesize() {
+  return sysconf(_SC_PAGESIZE);
+}
+#endif
 
 int main(int argc, char **argv)
 {
   // The size must be large enough to be serviced by the secondary allocator.
-  long page_size =