[PATCH] D83247: [compiler-rt][asan][hwasan] Refactor shadow setup into sanitizer_common (NFCI)

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_linux.cpp:103
 uptr FindDynamicShadowStart() {
+  uptr shadow_size_bytes = GetHighMemEnd(SHADOW_SCALE) >> SHADOW_SCALE;
 #if ASAN_PREMAP_SHADOW
----------------
MemToShadowSize(GetHighMemEnd(SHADOW_SCALE))


================
Comment at: compiler-rt/lib/hwasan/hwasan.cpp:289
 
-  MadviseShadow();
-
----------------
why it's gone?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common.h:123
 void MprotectMallocZones(void *addr, int prot);
 
+// Get the max address, taking into account alignment due to the mmap
----------------
Shadow is specific to only some sanitizers, so I don't like to have it in /sanitizer_common/
Also we have msan/tsan/dfsan with different shadows for which is not clear if we can reuse these functions without exposing more particular sanitizer details. Maybe keeping all shadow code with some redundancy but independent is going to be easier to maintain in long term.

Anyway, if we still go this way, maybe we put code into sanitizer_common/sanitizer_shadow_* files ?




================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common_libcdep.cpp:204
+  const uptr granularity = GetMmapGranularity();
+  const uptr alignment = shadow_base_alignment
+                             ? 1ULL << shadow_base_alignment
----------------
I think it's going to be cleaner if we replace 
uptr mmap_alignment_scale, uptr shadow_base_alignment
with
uptr shadow_scale, uptr min_shadow_base_alignment
and adjust calculations accordingly:
const uptr alignment = max(granularity << shadow_scale, min_shadow_base_alignment)

it should be copied into mac and win, even if they use 0 there, for consistency


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp:1083
+        largest_gap_found, max_occupied_addr);
+    uptr new_max_vm = RoundDownTo(largest_gap_found << SHADOW_SCALE, alignment);
+    if (new_max_vm < max_occupied_addr) {
----------------
SHADOW_SCALE is undefined here


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp:1092
+    RestrictMemoryToMaxAddress(new_max_vm);
+    kHighMemEnd = new_max_vm - 1;
+    space_size = kHighShadowEnd + left_padding;
----------------
kHighShadowEnd and kHighMemEnd are undefined here


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp:1093
+    kHighMemEnd = new_max_vm - 1;
+    space_size = kHighShadowEnd + left_padding;
+    VReport(2, "FindDynamicShadowStart, space_size = %p\n", space_size);
----------------
because if  kHighMemEnd here we need to return mapped size on all platforms :(


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_win.cpp:351
 
+uptr MapDynamicShadow(uptr shadow_size_bytes, uptr mmap_alignment_scale,
+                      uptr shadow_base_alignment) {
----------------
shadow_size_bytes is not used


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_win.cpp:353
+                      uptr shadow_base_alignment) {
+  CHECK(shadow_base_alignment == 0);
+  uptr granularity = GetMmapGranularity();
----------------
CHECK_NE


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_win.cpp:357
+  uptr left_padding = granularity;
+  uptr space_size = kHighShadowEnd + left_padding;
+  uptr shadow_start = FindAvailableMemoryRange(space_size, alignment,
----------------
kHighShadowEnd is asan constant
probably space_size should be calculated from shadow_size_bytes
same for mac version


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83247