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

[Matthew G McGovern via Phabricator via llvm-commits]

mcgov added a comment.

Addressing comments



================
Comment at: .gitignore:48
 /.idea
+#azure pipelines build config
+.azure-pipelines.yml
----------------
vitalybuka wrote:
> Separate patch?
Sure, I can wrap this up with some other msvc host compat stuff (coming soon)


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:39
 #define HEAP_REALLOC_IN_PLACE_ONLY 0x00000010

+#define HEAP_ALLOCATE_SUPPORTED_FLAGS (HEAP_ZERO_MEMORY)
+#define HEAP_ALLOCATE_UNSUPPORTED_FLAGS (~HEAP_ALLOCATE_SUPPORTED_FLAGS)
----------------
vitalybuka wrote:
> Why these and new constants are macros and not regular consts or constexpr?
No reason, I'll switch them


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:277
+                     SIZE_T dwBytes) {
+  CHECK(reallocFunc && heapSizeFunc && freeFunc && allocFunc);
+  size_t old_usable_size = 0;
----------------
vitalybuka wrote:
> Leave this to compiler?
> 
> ```
> template<class ReAllocFunction...> 
> static void* SharedReAlloc(ReAllocFunction reallocFunc, ...
> ```
I did try this solution first, but we're stuck here because the template parameters would end up being pointers that are not compile-time constants. The arguments that are being pointed to are not the regular HeapAlloc address (which get patched), it's the pointers to the REAL(HeapAlloc) which are not known until hooking is finished.

The templated solution is cleaner but doesn't work in this situation. I will address these other concerns though it should be in the namespace. Thank you for the feedback!




================
Comment at: compiler-rt/lib/asan/asan_win.cc:218
+  return teb->Reserved1[TEB_RESERVED_FIELDS_THREAD_LOCAL_STORAGE_OFFSET] !=
+         nullptr;
+}
----------------
vitalybuka wrote:
> could you please use consts?
been reading too much old win32 code sorry 😂
will fix!


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62927