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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka marked an inline comment as done.
vitalybuka added inline comments.


================
Comment at: .gitignore:48
 /.idea
+#azure pipelines build config
+.azure-pipelines.yml
----------------
Separate patch?


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:462
+  void *p;
+  // Reading MSDN suggests that the *entire* usable allocation is zeroed out.
+  // Otherwise it is difficult to HeapReAlloc with HEAP_ZERO_MEMORY.
----------------
mcgov wrote:
> vitalybuka wrote:
> > Isn't better to use REAL in cases above instead of CHECK(Flags...) ?
> I need more context on what you mean here, where would it be better?
Ignore my comment, now I understand your approach.


================
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)
----------------
Why these and new constants are macros and not regular consts or constexpr?


================
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;
----------------
Leave this to compiler?

```
template<class ReAllocFunction...> 
static void* SharedReAlloc(ReAllocFunction reallocFunc, ...
```


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:280
   GET_STACK_TRACE_MALLOC;
   GET_CURRENT_PC_BP_SP;
   (void)sp;
----------------
this should be static on in namespace {}



================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:280
   GET_STACK_TRACE_MALLOC;
   GET_CURRENT_PC_BP_SP;
   (void)sp;
----------------
vitalybuka wrote:
> this should be static on in namespace {}
> 
void* SharedReAlloc?


================
Comment at: compiler-rt/lib/asan/asan_win.cc:181
   // Try to intercept kernel32!RaiseException, and if that fails, intercept
   // ntdll!RtlRaiseException instead.
   if (!::__interception::OverrideFunction("RaiseException",
----------------
Still shows "Context not available."


================
Comment at: compiler-rt/lib/asan/asan_win.cc:218
+  return teb->Reserved1[TEB_RESERVED_FIELDS_THREAD_LOCAL_STORAGE_OFFSET] !=
+         nullptr;
+}
----------------
could you please use consts?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62927