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

[Walter Lee via Phabricator via llvm-commits]

waltl added inline comments.


================
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));
----------------
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.



================
Comment at: compiler-rt/lib/asan/asan_mapping_myriad.h:26
+
+#define kHighMemBeg    (kMyriadMemoryEnd32 + 1)
+
----------------
alekseyshl wrote:
> If you do not have a concept of high mem, why not set kHighMemBeg and kHighMemEnd to 0? It'll make other conditions more explicit.
That's what I did originally.  Vitally suggested that I define things this way.  I'm fine with either.  The tradeoffs I see are:

- define as 0: cleaner conceptually, consistent with handling of mid memory.

- define as range [begin, begin - 1]: fewer changes required.



================
Comment at: compiler-rt/lib/asan/asan_rtl.cc:59
     } else {
-      UnmapOrDie((void*)kLowShadowBeg, kHighShadowEnd - kLowShadowBeg);
+      if (!SANITIZER_MYRIAD2)
+        UnmapOrDie((void*)kLowShadowBeg, kHighShadowEnd - kLowShadowBeg);
----------------
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



Repository:
  rL LLVM

https://reviews.llvm.org/D46456