[PATCH] D46456: [asan] Add support for Myriad RTEMS memory map

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added a comment.

Looks good, thanks!



================
Comment at: compiler-rt/lib/asan/asan_mapping.h:352
   return AddrIsInLowMem(a) || AddrIsInMidMem(a) || AddrIsInHighMem(a) ||
+      (SANITIZER_MYRIAD2 && (AddrIsInLowShadow(a) || AddrIsInShadowGap(a))) ||
       (flags()->protect_shadow_gap == 0 && AddrIsInShadowGap(a));
----------------
waltl wrote:
> alekseyshl wrote:
> > Why the address is shadow is also considered in mem?
> > 
> > AddrIsInShadowGap(a) is covered by the next check, protect_shadow_gap is always 0 in your config, right?
> > Why the address is shadow is also considered in mem?
> 
> I had a few places where I had to check AddrIsInMem(addr) && AddrIsInShadow(addr).  Vitaly suggested that I define AddIsInMem accordingly so I don't have to do both checks.  This makes sense to me conceptually and I checked that it shouldn't break existing uses of the function.
> 
> > AddrIsInShadowGap(a) is covered by the next check, protect_shadow_gap is always 0 in
> > your config, right?
> 
> True.  I'll simplify.
> 
I see. What are these places? I'd rather have those checks explicit, because, on top of being pretty confusing to read, for one example, GetShadowAddressInformation() won't work on Myriad2.


================
Comment at: compiler-rt/lib/asan/asan_mapping.h:365
+  a = RawAddr(a);
   return a >= kHighShadowBeg && a <= kHighShadowEnd;
 }
----------------
alekseyshl wrote:
> return kHighShadowBeg && a >= kHighShadowBeg && a <= kHighShadowEnd;
> 
> otherwise nullptr will pass as a high shadow.
kHighMemBeg -> kHighShadowBeg


================
Comment at: compiler-rt/lib/asan/asan_rtl.cc:59
     } else {
-      UnmapOrDie((void*)kLowShadowBeg, kHighShadowEnd - kLowShadowBeg);
+      if (!SANITIZER_MYRIAD2)
+        UnmapOrDie((void*)kLowShadowBeg, kHighShadowEnd - kLowShadowBeg);
----------------
waltl wrote:
> alekseyshl wrote:
> > Instead of this condition, add CHECK(!f->unmap_shadow_on_exit) to InitializeFlags(), the same way you do for SANITIZER_RTEMS. By the way, isn't that existing check enough already?
> Yes this code never executes.  But without the guard I get a compiler error even at -O2:
> 
> error: implicit conversion from 'unsigned long long' to '__sanitizer::uptr' (aka 'unsigned long') changes value from 18446744071041974272 to 1627389952
> 
Right. Then let's change it to "if (kHighShadowEnd)" to make the code more generic:

  if (kMidMemBeg) {
    UnmapOrDie((void*)kLowShadowBeg, kMidMemBeg - kLowShadowBeg);
    if (kHighShadowEnd)
      UnmapOrDie((void*)kMidMemEnd, kHighShadowEnd - kMidMemEnd);
  } else {
    if (kHighShadowEnd)
      UnmapOrDie((void*)kLowShadowBeg, kHighShadowEnd - kLowShadowBeg);
  }


================
Comment at: compiler-rt/lib/asan/asan_rtl.cc:318
 #endif  // !ASAN_FIXED_MAPPING
   CHECK_EQ((kHighMemBeg % GetMmapGranularity()), 0);
 }
----------------
I think it should be

  #if !SANITIZER_MYRIAD2
  #if !ASAN_FIXED_MAPPING
    kHighMemEnd = GetMaxUserVirtualAddress();
    // Increase kHighMemEnd to make sure it's properly
    // aligned together with kHighMemBeg:
    kHighMemEnd |= SHADOW_GRANULARITY * GetMmapGranularity() - 1;
  #endif  // !ASAN_FIXED_MAPPING
    CHECK_EQ((kHighMemBeg % GetMmapGranularity()), 0);
  #endif  // !SANITIZER_MYRIAD2


Repository:
  rL LLVM

https://reviews.llvm.org/D46456