[PATCH] D62927: [sanitizers][windows] Rtl-Heap Interception and tests

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Sorry for not testing out this commit until it hit the tree and not commenting on this before. This one turned out to break MinGW builds for a few different reasons, that I'll comment on inline.

Unrelated point: Please close the review on/after committing; this is easily handled by mentioning the review in the commit message in the form `Differential Revision: https://reviews.llvm.org/D62927`. In this case I was surprised why this could have broken anything as the review still wasn't closed.

If fixing these cases with the issues I'm pointing out is too cumbersome, I wouldn't mind adding a large `#ifndef __MINGW32__` around the new code. But the release branch is approaching fast, and I'd like to have address sanitizer at least buildable for MinGW in the release (even though issues can be fixed after the branch as well) - having these new interceptors usable is less of a deal.



================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:18
+#if defined(_M_IX86)
+#define _X86_
+#elif defined(_M_AMD64)
----------------
This needs an `|| defined(__i386__)` for MinGW, and same with `defined(__x86_64__)` below. And changing the `#define _X86_` into `#define _X86_ 1` avoids a warning about redefinition to a different value.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:25
+#include <heapapi.h>
+
 #include "sanitizer_common/sanitizer_platform.h"
----------------
This file implicitly includes a lot of windows specific headers. See the comment below about "Intentionally not including windows.h here"; this brings back the same issue that was fixed earlier by removing the include of windows.h here, in particular errors like these:

```
compiler-rt/lib/asan/asan_malloc_win.cc:84:6: error: redeclaration of 'free' cannot add 'dllexport' attribute
void free(void *ptr) {
     ^
```

To avoid this, we need to make sure we don't see any inline function that uses the function `free` up to this point, when we redefine it with new attributes.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:344
   if (ptr == nullptr)
     return nullptr;
   if (dwFlags & HEAP_ZERO_MEMORY) {
----------------
In MinGW, `min()` isn't defined by the headers included so far. I guess that could be considered a MinGW header bug (it's not defined if `__cplusplus` is defined - fixing that if necessary should hopefully be doable), but the issue mentioned above about explicitly avoiding windows platform headers might also lead to it being unavailable in MSVC style builds.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:407
+  }
+  GET_STACK_TRACE_FREE;
+  asan_free(BaseAddress, &stack, FROM_MALLOC);
----------------
`_Frees_ptr_opt_` is not available/defined in MinGW - can it be left out here, or does that cause errors about mismatching attributes in an MSVC build?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62927