[vm] Fix freelist interaction with dual mapping

Freelist implementation was trying to mark pages executable (RX)
which does not respect possibility that it is working with RW mapping
which can't be directly marked as executable. It needed to use RO
permission instead if dual mapping is enabled.

This CL consolidates all code mentioning RX permission in one place:
VirtualMemory::WriteProtectCode so that we avoid this mistake in the future.

[email protected]
TEST=tested by doing large reloads which cause more churn in code space

Change-Id: If99692cac3ce3ff54b907e4c43d7f26bae7439ff
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/435860
Commit-Queue: Martin Kustermann <[email protected]>
Auto-Submit: Slava Egorov <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: mraleph

runtime/vm/code_patcher.cc

@@ -37,8 +37,7 @@ WritableInstructionsScope::WritableInstructionsScope(uword address,
 
 WritableInstructionsScope::~WritableInstructionsScope() {
   if (FLAG_write_protect_code) {
-    VirtualMemory::Protect(reinterpret_cast<void*>(address_), size_,
-                           VirtualMemory::kReadExecute);
+    VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address_), size_);
   }
 }
 #endif  // defined(TARGET_ARCH_IA32)

runtime/vm/compiler/relocation_test.cc

@@ -263,8 +263,7 @@ struct RelocatorTestHelper {
         ASSERT(!VirtualMemory::ShouldDualMapExecutablePages());
         const uword address = UntaggedObject::ToAddr(instructions.ptr());
         const auto size = instructions.ptr()->untag()->HeapSize();
-        VirtualMemory::Protect(reinterpret_cast<void*>(address), size,
-                               VirtualMemory::kReadExecute);
+        VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address), size);
       }
       CPU::FlushICache(instructions.PayloadStart(), instructions.Size());
     }

runtime/vm/ffi_callback_metadata.cc

@@ -187,16 +187,12 @@ VirtualMemory* FfiCallbackMetadata::AllocateTrampolinePage() {
     memcpy(new_page->address(), stub_page_->address(),  // NOLINT
            stub_page_->size());
 
+    VirtualMemory::WriteProtectCode(new_page->address(), aligned_size);
     if (VirtualMemory::ShouldDualMapExecutablePages()) {
       ASSERT(new_page->OffsetToExecutableAlias() != 0);
-      VirtualMemory::Protect(new_page->address(), aligned_size,
-                             VirtualMemory::kReadOnly);
       VirtualMemory::Protect(
           reinterpret_cast<void*>(RXAreaStart(new_page) + RXMappingSize()),
           RWMappingSize(), VirtualMemory::kReadWrite);
-    } else {
-      VirtualMemory::Protect(new_page->address(), aligned_size,
-                             VirtualMemory::kReadExecute);
     }
   }
 

runtime/vm/heap/freelist.cc

@@ -172,8 +172,8 @@ uword FreeList::TryAllocateLocked(intptr_t size, bool is_protected) {
         }
         previous->set_next(current->next());
         if (target_is_protected) {
-          VirtualMemory::Protect(reinterpret_cast<void*>(target_address),
-                                 kWordSize, VirtualMemory::kReadExecute);
+          VirtualMemory::WriteProtectCode(
+              reinterpret_cast<void*>(target_address), kWordSize);
         }
       }
       SplitElementAfterAndEnqueue(current, size, is_protected);
@@ -266,12 +266,11 @@ void FreeList::SplitElementAfterAndEnqueue(FreeListElement* element,
     if (!VirtualMemory::InSamePage(
             remainder_address - 1,
             remainder_address + remainder_header_size - 1)) {
-      VirtualMemory::Protect(
+      VirtualMemory::WriteProtectCode(
           reinterpret_cast<void*>(
               Utils::RoundUp(remainder_address, VirtualMemory::PageSize())),
           remainder_address + remainder_header_size -
-              Utils::RoundUp(remainder_address, VirtualMemory::PageSize()),
-          VirtualMemory::kReadExecute);
+              Utils::RoundUp(remainder_address, VirtualMemory::PageSize()));
     }
   }
 }

runtime/vm/heap/freelist_test.cc

@@ -14,8 +14,7 @@ namespace dart {
 static uword Allocate(FreeList* free_list, intptr_t size, bool is_protected) {
   uword result = free_list->TryAllocate(size, is_protected);
   if ((result != 0u) && is_protected) {
-    VirtualMemory::Protect(reinterpret_cast<void*>(result), size,
-                           VirtualMemory::kReadExecute);
+    VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(result), size);
   }
   return result;
 }
@@ -30,8 +29,7 @@ static void Free(FreeList* free_list,
   }
   free_list->Free(address, size);
   if (is_protected) {
-    VirtualMemory::Protect(reinterpret_cast<void*>(address), size,
-                           VirtualMemory::kReadExecute);
+    VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address), size);
   }
 }
 
@@ -47,7 +45,7 @@ static void TestFreeList(VirtualMemory* region,
 
   if (is_protected) {
     // Write protect the whole region.
-    region->Protect(VirtualMemory::kReadExecute);
+    region->WriteProtectCode();
   }
 
   // Allocate a small object. Expect it to be positioned as the first element.
@@ -122,7 +120,7 @@ TEST_CASE(FreeListProtectedTinyObjects) {
   free_list->Free(blob->start(), blob->size());
 
   // Write protect the whole region.
-  blob->Protect(VirtualMemory::kReadExecute);
+  blob->WriteProtectCode();
 
   // Allocate small objects.
   for (intptr_t i = 0; i < blob->size() / kObjectSize; i++) {
@@ -221,10 +219,9 @@ static void TestRegress38528(intptr_t header_overlap) {
   memcpy(other_code, ret, sizeof(ret));  // NOLINT
 
   free_list->Free(blob->start(), alloc_size + remainder_size);
-  blob->Protect(VirtualMemory::kReadExecute);  // not writable
+  blob->WriteProtectCode();  // not writable
   Allocate(free_list.get(), alloc_size, /*is_protected=*/true);
-  VirtualMemory::Protect(blob->address(), alloc_size,
-                         VirtualMemory::kReadExecute);
+  VirtualMemory::WriteProtectCode(blob->address(), alloc_size);
   reinterpret_cast<void (*)()>(other_code)();
 }
 

runtime/vm/heap/page.cc

@@ -293,19 +293,13 @@ void Page::ResetProgressBar() {
 
 void Page::WriteProtect(bool read_only) {
   ASSERT(!is_image());
-  VirtualMemory::Protection prot;
-  if (read_only) {
-    // When dual mapping code pages we don't change protection on RX page, but
-    // flip RW to R and back.
-    if (is_executable() && !VirtualMemory::ShouldDualMapExecutablePages()) {
-      prot = VirtualMemory::kReadExecute;
-    } else {
-      prot = VirtualMemory::kReadOnly;
-    }
+  if (is_executable() && read_only) {
+    // Handle making code executable in a special way.
+    memory_->WriteProtectCode();
   } else {
-    prot = VirtualMemory::kReadWrite;
+    memory_->Protect(read_only ? VirtualMemory::kReadOnly
+                               : VirtualMemory::kReadWrite);
   }
-  memory_->Protect(prot);
 }
 
 }  // namespace dart

runtime/vm/object.cc

@@ -18746,11 +18746,8 @@ CodePtr Code::FinalizeCode(FlowGraphCompiler* compiler,
       // RX mapping never changes protection while RW mapping flips between
       // R and RW.
       uword address = UntaggedObject::ToAddr(instrs.ptr());
-      VirtualMemory::Protect(reinterpret_cast<void*>(address),
-                             instrs.ptr()->untag()->HeapSize(),
-                             VirtualMemory::ShouldDualMapExecutablePages()
-                                 ? VirtualMemory::kReadOnly
-                                 : VirtualMemory::kReadExecute);
+      VirtualMemory::WriteProtectCode(reinterpret_cast<void*>(address),
+                                      instrs.ptr()->untag()->HeapSize());
     }
 
     // Hook up Code and Instructions objects.

runtime/vm/virtual_memory.h

@@ -80,6 +80,15 @@ class VirtualMemory {
 #endif
   }
 
+  // Write protect a chunk of machine code which is currently writable.
+  DART_FORCE_INLINE static void WriteProtectCode(void* address, intptr_t size) {
+    Protect(address, size,
+            ShouldDualMapExecutablePages() ? kReadOnly : kReadExecute);
+  }
+  DART_FORCE_INLINE void WriteProtectCode() const {
+    WriteProtectCode(address(), size());
+  }
+
   bool Contains(uword addr) const { return region_.Contains(addr); }
 
   // Changes the protection of the virtual memory area.

runtime/vm/virtual_memory_posix.cc

@@ -493,9 +493,7 @@ bool CheckIfRXWorks() {
       0xd65f03c0   // ret
   };
   memmove(mem->address(), kSquareFunctionCode, sizeof(kSquareFunctionCode));
-  mem->Protect(VirtualMemory::ShouldDualMapExecutablePages()
-                   ? VirtualMemory::kReadOnly
-                   : VirtualMemory::kReadExecute);
+  VirtualMemory::WriteProtectCode(mem->address(), mem->size());
 
   // Get executable entry point and check that write have succeeded.
   const uword entry_point = mem->start() + mem->OffsetToExecutableAlias();