Page MenuHomePhabricator

[asan] Add support for Myriad RTEMS memory map
ClosedPublic

Authored by waltl on May 4 2018, 1:45 PM.

Details

Summary

The Myriad RTEMS memory system has a few unique aspects that
require support in the ASan run-time.

  • A limited amount of memory (currently 512M).
  • No virtual memory, no memory protection.
  • DRAM starts at address 0x80000000. Other parts of memory may be used for MMIO, etc.
  • The second highest address bit is the "cache" bit, and 0x80000000 and 0x84000000 alias to the same memory.

To support the above, we make the following changes:

  • Use a ShadowScale of 5, to reduce shadow memory overhead.
  • Adjust some existing macros to remove assumption that the lowest memory address is 0.
  • add a RAW_ADDR macro that on Myriad strips the cache bit from the input address, before using the address for shadow memory (for other archs this does nothing).
  • We must check that an address is in DRAM range before using it to index into shadow memory.

Diff Detail

Repository
rL LLVM

Event Timeline

waltl created this revision.May 4 2018, 1:45 PM
kcc added inline comments.May 4 2018, 2:48 PM
compiler-rt/lib/asan/asan_mapping.h
231 ↗(On Diff #145273)

this section is very hard to read (and, hence, maintain) due to extra ?:
I'd prefer if you create a separate asan_mapping_myriad.h or some such and hide the specific logic there.

waltl updated this revision to Diff 145767.May 8 2018, 1:29 PM

Move Myriad memory mapping definitions to its own file.

waltl marked an inline comment as done.May 8 2018, 1:30 PM
waltl updated this revision to Diff 146334.May 11 2018, 8:32 AM

Rebase.

krytarowski added inline comments.
compiler-rt/lib/asan/asan_mapping.h
141 ↗(On Diff #146334)

Is this RTEMS/Myriad2 CPU?

waltl marked an inline comment as done.May 11 2018, 8:52 AM
waltl added inline comments.
compiler-rt/lib/asan/asan_mapping.h
141 ↗(On Diff #146334)

The memory map is for Myriad2, which is a chip comprising multiple CPUs (2 Sparcs and one Shave). RTEMS is the OS used but it is not tied to the memory map.

vitalybuka added inline comments.May 11 2018, 11:20 AM
compiler-rt/lib/asan/asan_mapping.h
201 ↗(On Diff #146334)

please define SHADOW_SCALE here

294 ↗(On Diff #146334)

you can use ASAN_FIXED_MAPPING and platform specific FindDynamicShadowStart()

311 ↗(On Diff #146334)

what is this for?

329 ↗(On Diff #146334)

Instead of checking kHighMemBeg here, can you just set kHighMemEnd = kHighMemBeg - 1

compiler-rt/lib/asan/asan_poisoning.cc
187 ↗(On Diff #146334)

Why do you need to check AddrIsInShadow here? Could you make sure that ranges set so that not DRAM is not AddrIsInMem

waltl marked an inline comment as done.May 11 2018, 12:12 PM
waltl added inline comments.
compiler-rt/lib/asan/asan_mapping.h
311 ↗(On Diff #146334)

Myriad addresses have a bit that tells the processor whether its data should be cached. For example, addresses 0x80abcdef and 0x84abcdef correspond to the same addresses. So to get the raw address we need to strip the cache bit.

waltl added inline comments.May 11 2018, 12:54 PM
compiler-rt/lib/asan/asan_mapping.h
294 ↗(On Diff #146334)

Can you clarify? If I use ASAN_FIXED_MAPPING I have to clone the definitions in this header, and initialize them in the header and not FindDynamicShadowStart().

vitalybuka added inline comments.May 11 2018, 2:07 PM
compiler-rt/lib/asan/asan_mapping.h
294 ↗(On Diff #146334)

Sorry, I meant to write "NOT ASAN_FIXED_MAPPING" and reset pointers in runtime from platform specific FindDynamicShadowStart

waltl updated this revision to Diff 146439.May 11 2018, 4:40 PM

Address CR comments

waltl marked 5 inline comments as done.May 11 2018, 4:43 PM
waltl added inline comments.
compiler-rt/lib/asan/asan_mapping.h
294 ↗(On Diff #146334)

Myriad port doesn't use FindDynamicShadowStart, but I've put the initialization in InitializeShadowMemory() defined in asan_rtems.cc (see https://reviews.llvm.org/D46468).

waltl marked 2 inline comments as done.May 11 2018, 4:44 PM
alekseyshl added inline comments.May 16 2018, 3:24 PM
compiler-rt/lib/asan/asan_mapping.h
307 ↗(On Diff #146439)

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)

352 ↗(On Diff #146439)

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?

365 ↗(On Diff #146439)

return kHighShadowBeg && a >= kHighShadowBeg && a <= kHighShadowEnd;

otherwise nullptr will pass as a high shadow.

compiler-rt/lib/asan/asan_mapping_myriad.h
26 ↗(On Diff #146439)

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.

compiler-rt/lib/asan/asan_rtl.cc
59 ↗(On Diff #146439)

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?

144 ↗(On Diff #146439)

Why __asan_region_is_poisoned special cases SANITIZER_MYRIAD2 and here you check for SANITIZER_RTEMS? Shouldn't it be the same?

322 ↗(On Diff #146439)

"if (kHighShadowBeg) {" or, if you set kHighMemBeg to 0, "if (kHighMemBeg) {"

349 ↗(On Diff #146439)

if (kHighShadowBeg) {

waltl added inline comments.May 16 2018, 6:45 PM
compiler-rt/lib/asan/asan_mapping.h
352 ↗(On Diff #146439)

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.

compiler-rt/lib/asan/asan_mapping_myriad.h
26 ↗(On Diff #146439)

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.
compiler-rt/lib/asan/asan_rtl.cc
59 ↗(On Diff #146439)

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

waltl updated this revision to Diff 147227.May 16 2018, 7:28 PM

Address CR comments

waltl edited the summary of this revision. (Show Details)May 16 2018, 7:29 PM
waltl marked 9 inline comments as done.May 16 2018, 7:32 PM
waltl added inline comments.
compiler-rt/lib/asan/asan_mapping_myriad.h
26 ↗(On Diff #146439)

I went back to setting kHighMemBeg to 0.

waltl updated this revision to Diff 147362.May 17 2018, 11:17 AM
waltl marked an inline comment as done.

Fixes to support changing the shadow scale.

Looks good, thanks!

compiler-rt/lib/asan/asan_mapping.h
352 ↗(On Diff #146439)

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.

365 ↗(On Diff #146439)

kHighMemBeg -> kHighShadowBeg

compiler-rt/lib/asan/asan_rtl.cc
318 ↗(On Diff #147227)

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
59 ↗(On Diff #146439)

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);
}
vitalybuka added inline comments.May 17 2018, 11:42 AM
compiler-rt/lib/asan/asan_mapping.h
307 ↗(On Diff #146439)

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?

compiler-rt/lib/asan/asan_mapping_myriad.h
26 ↗(On Diff #146439)

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.

alekseyshl added inline comments.May 17 2018, 11:48 AM
compiler-rt/lib/asan/asan_mapping_myriad.h
26 ↗(On Diff #146439)

Yep, that was my point, let's keep the code consistent, it is pretty non-trivial as it is.

waltl added inline comments.May 17 2018, 12:01 PM
compiler-rt/lib/asan/asan_mapping.h
352 ↗(On Diff #146439)

They are in __asan_region_is_poisoned (2 places) and
ASAN_MEMORY_ACCESS_CALLBACK_BODY, where there are now checks for just !AddrIsInMem(addr).

365 ↗(On Diff #146439)

This should be ok, if we keep kHighMemBeg = 0? I was keeping consistent with AddrIsInMidShadow().

alekseyshl added inline comments.May 17 2018, 1:43 PM
compiler-rt/lib/asan/asan_mapping.h
352 ↗(On Diff #146439)

Yep, move them there and do not change AddrIsInMem. As I mentioned, it breaks other stuff using AddrIsInMem.

365 ↗(On Diff #146439)

Makes sense, keep kHighMemBeg.

waltl updated this revision to Diff 147392.May 17 2018, 2:29 PM

Address CR comments

waltl marked 16 inline comments as done.May 17 2018, 2:37 PM
waltl added inline comments.
compiler-rt/lib/asan/asan_mapping_myriad.h
26 ↗(On Diff #146439)

Using 0 for now.

alekseyshl accepted this revision.May 17 2018, 4:02 PM
alekseyshl added inline comments.
compiler-rt/lib/asan/asan_mapping.h
153 ↗(On Diff #147392)

How about dropping kMyriadShadowScale:

#if defined(ASAN_SHADOW_SCALE)
static const u64 kDefaultShadowScale = ASAN_SHADOW_SCALE;
#elif defined(SANITIZER_MYRIAD2)
static const u64 kDefaultShadowScale = 5;
#else
static const u64 kDefaultShadowScale = 3;
#endif

then you won't need to fiddle with SHADOW_SCALE defines

This revision is now accepted and ready to land.May 17 2018, 4:02 PM
waltl updated this revision to Diff 147416.May 17 2018, 5:12 PM
waltl marked an inline comment as done.

Address CR comments

waltl marked an inline comment as done.May 17 2018, 5:14 PM
This revision was automatically updated to reflect the committed changes.