[heap] Fix i-cache flushing operation order.

This unifies the order of i-cache flushing and permission changing
throughout V8. According to cctest/test-icache flushing after the
permission change is not robust on some ARM32 and ARM64 devices.

There have been observed failures of {TestFlushICacheOfExecutable} on
some devices. So far there haven't been any observed failures of the
corresponding {TestFlushICacheOfWritable} test.

Also the order of flushing before the permission change is the natural
order in which the GC currently performs operations. Until we see
concrete data substantiating the opposite, the following is the
supported and intended order throughout V8:

  exec -> perm(RW) -> patch -> flush -> perm(RX) -> exec

This CL tries to establish said order throughout the codebase.

R=ulan@chromium.org
TEST=cctest/test-icache
BUG=v8:8507,chromium:845877

Change-Id: Ic945082e643aa2d142d222a7913a99816aff4644
Reviewed-on: https://chromium-review.googlesource.com/c/1351025
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57869}

src/heap/factory.cc

@@ -2726,9 +2726,14 @@ MaybeHandle<Code> Factory::TryNewCode(
                    source_position_table, deopt_data, reloc_info,
                    data_container, stub_key, is_turbofanned, stack_slots,
                    safepoint_table_offset, handler_table_offset);
+
+    // Flush the instruction cache before changing the permissions.
+    // Note: we do this before setting permissions to ReadExecute because on
+    // some older ARM kernels there is a bug which causes an access error on
+    // cache flush instructions to trigger access error on non-writable memory.
+    // See https://bugs.chromium.org/p/v8/issues/detail?id=8157
+    code->FlushICache();
   }
-  // Flush the instruction cache after changing the permissions.
-  code->FlushICache();
 
   return code;
 }
@@ -2775,9 +2780,14 @@ Handle<Code> Factory::NewCode(
                    source_position_table, deopt_data, reloc_info,
                    data_container, stub_key, is_turbofanned, stack_slots,
                    safepoint_table_offset, handler_table_offset);
+
+    // Flush the instruction cache before changing the permissions.
+    // Note: we do this before setting permissions to ReadExecute because on
+    // some older ARM kernels there is a bug which causes an access error on
+    // cache flush instructions to trigger access error on non-writable memory.
+    // See https://bugs.chromium.org/p/v8/issues/detail?id=8157
+    code->FlushICache();
   }
-  // Flush the instruction cache after changing the permissions.
-  code->FlushICache();
 
   return code;
 }

src/objects/code.h

@@ -324,7 +324,7 @@ class Code : public HeapObjectPtr {
                                               const CodeDesc& desc);
 
   // Flushes the instruction cache for the executable instructions of this code
-  // object.
+  // object. Make sure to call this while the code is still writable.
   void FlushICache() const;
 
   // Returns the object size for a given body (used for allocation).

test/cctest/test-icache.cc

@@ -116,13 +116,17 @@ TEST(TestFlushICacheOfWritable) {
   }
 }
 
-#if V8_TARGET_ARCH_ARM64
-// Note that this order of operations is not supported on ARM64 because on
-// some older Arm64 kernels there is a bug which causes an access error on
+#if V8_TARGET_ARCH_ARM || V8_TARGET_ARCH_ARM64
+// Note that this order of operations is not supported on ARM32/64 because on
+// some older ARM32/64 kernels there is a bug which causes an access error on
 // cache flush instructions to trigger access error on non-writable memory.
 // See https://bugs.chromium.org/p/v8/issues/detail?id=8157
 //
 // Also note that this requires {kBufferSize == 8 * KB} to reproduce.
+//
+// The order of operations in V8 is akin to {TestFlushICacheOfWritable} above.
+// It is hence OK to disable the below test on some architectures. Only the
+// above test case should remain enabled on all architectures.
 #define CONDITIONAL_TEST DISABLED_TEST
 #else
 #define CONDITIONAL_TEST TEST

test/common/assembler-tester.h

@@ -27,7 +27,7 @@ static inline void MakeAssemblerBufferExecutable(uint8_t* buffer,
                                                  size_t allocated) {
   // Flush the instruction cache as part of making the buffer executable.
   // Note: we do this before setting permissions to ReadExecute because on
-  // some older Arm64 kernels there is a bug which causes an access error on
+  // some older ARM kernels there is a bug which causes an access error on
   // cache flush instructions to trigger access error on non-writable memory.
   // See https://bugs.chromium.org/p/v8/issues/detail?id=8157
   Assembler::FlushICache(buffer, allocated);