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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 3 inline comments as done.
tejohnson added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_mapping.h:43
+constexpr uptr kZeroBaseShadowStart = 0;
+constexpr uptr kZeroBaseMaxShadowStart = 1 << 18;
+
----------------
vitalybuka wrote:
> Why do we need them in the header? I don't see these constants used outside hwasan_linux.
> If this is to pass kHighMemEnd into high_mem_end, then we can make it optional
> uptr* high_mem_end = null
You are right, the constants kZeroBaseShadowStart and kZeroBaseMaxShadowStart are only used in hwasan_linux.cpp, so I can move them there (I put them here initially since they are in asan_mapping.h there).
For kHighMemEnd etc, I made them extern so that they could be passed tp MapDynamicShadow in hwasan_dynamic_shadow.cpp. While that arg is only needed right now on mac, and hwasan is only supporting linux, does it make sense to be consistent (will hwasan ever support mac)?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common.h:126
+// granularity and shadow size.
+uptr GetHighMemEnd(uptr shadow_scale);
+
----------------
vitalybuka wrote:
> Could you please keep this one in asan/hwasan for now.
> It looks like a random mem-region related code  moved here when the rest is still there.
> 
ok, there is only one call from each of those, so I can move it back.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common.h:135
+uptr MapDynamicShadow(uptr shadow_size_bytes, uptr shadow_scale,
+                      uptr &high_mem_end, uptr min_shadow_base_alignment = 0);
+
----------------
vitalybuka wrote:
> and maybe drop default 0 and make out argument the last one?
> 
> Also high_mem_end in this interface looks unrelated. Maybe mapped shadow size? Better to land as is and followup later.
> 

> Also high_mem_end in this interface looks unrelated. Maybe mapped shadow size? Better to land as is and followup later.

I'm not sure I understand the comment about it being unrelated, or what you are asking me to do here? Do you mean just change the parameter name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83247