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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 4 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;
+
----------------
tejohnson wrote:
> 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)?
For now I have moved the kZeroBase* constants to hwasan_linux.h, but left the new externs.


================
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);
+
----------------
tejohnson wrote:
> 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?
Made the new out parameter last.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83247