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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 6 inline comments as done.
tejohnson 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
----------------
vitalybuka wrote:
> MemToShadowSize(GetHighMemEnd(SHADOW_SCALE))
The MemToShadowSize facility currently only exists in hwasan, let me go ahead and add it to asan as well (I see I accidentally started referencing it in the asan_mac/win code!).


================
Comment at: compiler-rt/lib/asan/asan_linux.cpp:122
-  uptr granularity = GetMmapGranularity();
-  uptr alignment = granularity * 8;
-  uptr left_padding = granularity;
----------------
vitalybuka wrote:
> kcc wrote:
> > tejohnson wrote:
> > > The code in asan is multiplying the mmap granularity by 8, whereas the hwasan version shifts it by kShadowScale. I wasn't sure if the 8 here is supposed to be equivalent to a left shift by the shadow scale (which is typically 3 in asan), or is specifically hardcoded separately not using SHADOW_SCALE since it could be something other than 3 in some cases (e.g. 5 for myriad, or user set via ASAN_SHADOW_SCALE). Depending on what was intended here, I would keep the hardcoding of "3" passed to my refactored MapDynamicShadow, or change that to SHADOW_SCALE.
> > I frankly don't remember :(
> It should be SHADOW_SCALE, myriad works only because it does not use dynamic shadow
Ok thanks, I'll change the code to use that.


================
Comment at: compiler-rt/lib/hwasan/hwasan.cpp:289
 
-  MadviseShadow();
-
----------------
vitalybuka wrote:
> why it's gone?
It is now embedded in the refactored code (similar to the way asan was doing the madvise calls). See ReserveShadowMemoryRange (the refactored one and the original asan one).


================
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
----------------
vitalybuka wrote:
> 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 ?
> 
> 
> 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.

I started to look at trying to coalesce some of the other *san shadow setup code with the *asan versions, but decided to punt on that, at least for now. There are a lot of things that looked similar there too, but not as much as in *asan. As I mentioned in my reply on the RFC just now, it seemed like a good idea to at least common the *asan versions since they were structured somewhat different but essentially functionally the same in almost every way, and it seemed confusing to have multiple versions that aren't obviously duplicates but in fact are.

That being said, I could have the new heap profiler duplicate them (at least the parts that I need) if that is much preferred. I don't technically need ProtectGap for the heap profiler, but it seems like a good thing to use at least while debugging the instrumentation code.

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

I can do that. There is a small amount of shadow setup code already here, I could move that too.


================
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
----------------
vitalybuka wrote:
> 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
Sounds good


================
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) {
----------------
vitalybuka wrote:
> SHADOW_SCALE is undefined here
For this and other comments on the win/mac version, obviously I didn't do a very careful job of moving this code from asan to sanitizer_common. =( Will work on fixing those and need to find a way to at least compile these codes for those platforms to flush out these issues. If you have any pointers on setting up a cross compile of this code on a linux system please let me know.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83247