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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added a comment.

In D83247#2154738 <https://reviews.llvm.org/D83247#2154738>, @vitalybuka wrote:

> Thank you! LGTM
>
> Windows should work. For the rest we need to rely on bots.


Great thanks - did you still want me to move the shadow functions to some new files (e.g. sanitizer_common/sanitizer_shadow_{linux_libcdep, win, mac}.cpp before committing?



================
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:
> tejohnson wrote:
> > 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.
> > 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?
> 
> Looking at function interface, it's about shadow. For reader is likely non-intuitive to understand why there is high_mem_end out parameter.
> Pure renaming is not going to help. Maybe out parameter with actually mapped side.
> Anyway I am not sure what could be a nicer approach for now. So we can keep it as-is and maybe fix later.
> 
> btw. it could be nice to write something into high_mem_end, even just 0, on other platforms.
> btw. it could be nice to write something into high_mem_end, even just 0, on other platforms.

That would actually break things since we are passing in kHighMemEnd, and don't want it reset if it shouldn't change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83247