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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_mapping.h:307
+  return SANITIZER_MYRIAD2 ? (a & ~kMyriadCacheBitMask32) : a;
+}
+
----------------
alekseyshl wrote:
> It would be more in the spirit of this file to define a macro here:
> #define RAW_ADDR(mem) (mem)
> and in asan_mapping_myriad.h:
> #define RAW_ADDR(mem) (mem & ~kMyriadCacheBitMask32)
The rest of the function is trivial, so not much value in code reuse.
Maybe just move what ever is using RAW_ADDR into myriad file?


================
Comment at: compiler-rt/lib/asan/asan_mapping_myriad.h:26
+
+#define kHighMemBeg    (kMyriadMemoryEnd32 + 1)
+
----------------
waltl wrote:
> waltl wrote:
> > 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.
> > 
> I went back to setting kHighMemBeg to 0.
> 
It's not just fewer changes, with [begin, begin - 1] you don't need sentinel value like 0.  This also confusing because kLowMemBeg is 0 and it's not sentinel in that case.
Still we already use 0 as sentinel for kMidMemBeg.

So I'd prefer to convert all code into [begin, begin - 1] for unavailable regions, but feel free to keep as 0, as it's already a mix of approaches.



Repository:
  rL LLVM

https://reviews.llvm.org/D46456