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

[Matthew G McGovern via Phabricator via llvm-commits]

mcgov closed this revision.
mcgov marked 8 inline comments as done.
mcgov added a comment.

@mrstojo no worries! I'm very new and still learning how to be a good contributor so I appreciate feedback on what I'm doing wrong. I will remove the heapapi include and replace with just the function prototypes I needed along with those constants.

I'm closing this review and will open a new one for the mingw32 changes



================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:18
+#if defined(_M_IX86)
+#define _X86_
+#elif defined(_M_AMD64)
----------------
mstorsjo wrote:
> 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.
Will just yank this, we don't need the full include.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:25
+#include <heapapi.h>
+
 #include "sanitizer_common/sanitizer_platform.h"
----------------
mstorsjo wrote:
> 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.
Whoops! will just replace with the items I need. We should need the function prototypes so if that's correct I'll just yank them out of the header.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:344
   if (ptr == nullptr)
     return nullptr;
   if (dwFlags & HEAP_ZERO_MEMORY) {
----------------
mstorsjo wrote:
> 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.
no trouble, will replace


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:407
+  }
+  GET_STACK_TRACE_FREE;
+  asan_free(BaseAddress, &stack, FROM_MALLOC);
----------------
mstorsjo wrote:
> `_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?
Nope, will remove this. My guess is this is some static analysis macro actually that resolves to nothing anyway, I just missed this was here since it's defined somewhere in that headerm so it shouldn't hurt anything to remove.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62927