[PATCH] D40423: [ARM][AArch64] Workaround ARM/AArch64 percularity in clearing icache.

[Maxim Kuvyrkov via Phabricator via llvm-commits]

maxim-kuvyrkov added inline comments.


================
Comment at: lib/Support/Unix/Memory.inc:134
+      return MemoryBlock();
+  }
 
----------------
zatrazz wrote:
> I think instead of issue a mmap plus two mprotects for MF_EXEC, it would be better if we could just mmap with read permission, call InvalidateInstructionCache and then mmap to the expected protection. Something as:
> 
> ```
> int adjustAllocateMappedProtectionFlags (unsigned Flags)
> {
> #if defined(__NetBSD__) && defined(PROT_MPROTECT)
>   Protect |= PROT_MPROTECT(PROT_READ | PROT_WRITE | PROT_EXEC);
> #endif
> #if defined(__arm__) || defined(__aarch64__)
>   Protect |= PROT_READ;
> #endif
>   return Protect;
> }
> 
> [...]
> 
> MemoryBlock
> Memory::allocateMappedMemory(size_t NumBytes,
>                              const MemoryBlock *const NearBlock,
>                              unsigned PFlags,
>                              std::error_code &EC) {
>   [...]
>   int Protect = getPosixProtectionFlags(PFlags);
> 
>   Protect = adjustAllocateMappedProtectionFlags (Protect);
>   [...]
> ```
> 
> And then issue the InvalidateInstructionCache without requiring to mprotect to PROT_READ first:
> 
> ```
> std::error_code
> Memory::protectMappedMemory(const MemoryBlock &M, unsigned Flags) {
>   [...]
>   bool InvalidateCache = (Flags & MF_EXEC);
> 
> #if defined(__arm__) || defined(__aarch64__)
>   // Certain ARM implementations treat icache clear instruction as a memory read,
>   // and CPU segfaults on trying to clear cache on !PROT_READ page.  Therefore we need
>   // to temporarily add PROT_READ for the sake of flushing the instruction caches.
>   if (InvalidateCache && !(Protect & PROT_READ)) {
>     Memory::InvalidateInstructionCache(M.Address, M.Size);
>     InvalidateCache = false;
>   }
> #endif
> ```
I experimented with that, and the code starts to look much uglier.  This file is target-independent, and it seems better to localize all workaround code in a single function, rather than spreading it across several functions.


https://reviews.llvm.org/D40423