Use aligned reads/writes in SandboxedPointer accessors when possible

Previously, when accessing SandboxedPointer fields with the sandbox
disabled, we would always do a ReadUnalignedValue/WriteUnalignedValue.
However, that is only necessary when pointer compression is enabled.
Otherwise, the field will be properly aligned.

This CL also factors out the logic to determine when to use an unaligned
or aligned read/write for a field into two new helper functions.

Bug: chromium:1292669
Change-Id: I2c1af187c5b2699101c3fee9cc551be788d3a845
Cq-Include-Trybots: luci.v8.try:v8_linux64_heap_sandbox_dbg_ng,v8_linux_arm64_sim_heap_sandbox_dbg_ng
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3429200Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Samuel Groß <saelo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78887}

BUILD.bazel

@@ -1146,6 +1146,7 @@ filegroup(
         "src/common/high-allocation-throughput-scope.h",
         "src/common/message-template.h",
         "src/common/ptr-compr-inl.h",
+        "src/common/ptr-compr.h",
         "src/compiler-dispatcher/lazy-compile-dispatcher.cc",
         "src/compiler-dispatcher/lazy-compile-dispatcher.h",
         "src/compiler-dispatcher/optimizing-compile-dispatcher.cc",

BUILD.gn

@@ -2735,6 +2735,7 @@ v8_header_set("v8_internal_headers") {
     "src/common/high-allocation-throughput-scope.h",
     "src/common/message-template.h",
     "src/common/ptr-compr-inl.h",
+    "src/common/ptr-compr.h",
     "src/compiler-dispatcher/lazy-compile-dispatcher.h",
     "src/compiler-dispatcher/optimizing-compile-dispatcher.h",
     "src/compiler/access-builder.h",

src/common/ptr-compr.h

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_COMMON_PTR_COMPR_H_
+#define V8_COMMON_PTR_COMPR_H_
+
+#include "src/base/memory.h"
+#include "src/common/globals.h"
+
+namespace v8 {
+namespace internal {
+
+// Accessors for fields that may be unaligned due to pointer compression.
+
+template <typename V>
+static inline V ReadMaybeUnalignedValue(Address p) {
+  // Pointer compression causes types larger than kTaggedSize to be unaligned.
+#ifdef V8_COMPRESS_POINTERS
+  constexpr bool v8_pointer_compression_unaligned = sizeof(V) > kTaggedSize;
+#else
+  constexpr bool v8_pointer_compression_unaligned = false;
+#endif
+  // Bug(v8:8875) Double fields may be unaligned.
+  constexpr bool unaligned_double_field =
+      std::is_same<V, double>::value && kDoubleSize > kTaggedSize;
+  if (unaligned_double_field || v8_pointer_compression_unaligned) {
+    return base::ReadUnalignedValue<V>(p);
+  } else {
+    return base::Memory<V>(p);
+  }
+}
+
+template <typename V>
+static inline void WriteMaybeUnalignedValue(Address p, V value) {
+  // Pointer compression causes types larger than kTaggedSize to be unaligned.
+#ifdef V8_COMPRESS_POINTERS
+  constexpr bool v8_pointer_compression_unaligned = sizeof(V) > kTaggedSize;
+#else
+  constexpr bool v8_pointer_compression_unaligned = false;
+#endif
+  // Bug(v8:8875) Double fields may be unaligned.
+  constexpr bool unaligned_double_field =
+      std::is_same<V, double>::value && kDoubleSize > kTaggedSize;
+  if (unaligned_double_field || v8_pointer_compression_unaligned) {
+    base::WriteUnalignedValue<V>(p, value);
+  } else {
+    base::Memory<V>(p) = value;
+  }
+}
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_COMMON_PTR_COMPR_H_

src/objects/objects.h

@@ -19,6 +19,7 @@
 #include "src/common/assert-scope.h"
 #include "src/common/checks.h"
 #include "src/common/message-template.h"
+#include "src/common/ptr-compr.h"
 #include "src/flags/flags.h"
 #include "src/objects/elements-kind.h"
 #include "src/objects/field-index.h"
@@ -669,18 +670,7 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
                                                  std::is_enum<T>::value,
                                              int>::type = 0>
   inline T ReadField(size_t offset) const {
-    // Pointer compression causes types larger than kTaggedSize to be unaligned.
-#ifdef V8_COMPRESS_POINTERS
-    constexpr bool v8_pointer_compression_unaligned = sizeof(T) > kTaggedSize;
-#else
-    constexpr bool v8_pointer_compression_unaligned = false;
-#endif
-    if (std::is_same<T, double>::value || v8_pointer_compression_unaligned) {
-      // Bug(v8:8875) Double fields may be unaligned.
-      return base::ReadUnalignedValue<T>(field_address(offset));
-    } else {
-      return base::Memory<T>(field_address(offset));
-    }
+    return ReadMaybeUnalignedValue<T>(field_address(offset));
   }
 
   // Atomically reads a field using relaxed memory ordering. Can only be used
@@ -696,18 +686,7 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
                                                  std::is_enum<T>::value,
                                              int>::type = 0>
   inline void WriteField(size_t offset, T value) const {
-    // Pointer compression causes types larger than kTaggedSize to be unaligned.
-#ifdef V8_COMPRESS_POINTERS
-    constexpr bool v8_pointer_compression_unaligned = sizeof(T) > kTaggedSize;
-#else
-    constexpr bool v8_pointer_compression_unaligned = false;
-#endif
-    if (std::is_same<T, double>::value || v8_pointer_compression_unaligned) {
-      // Bug(v8:8875) Double fields may be unaligned.
-      base::WriteUnalignedValue<T>(field_address(offset), value);
-    } else {
-      base::Memory<T>(field_address(offset)) = value;
-    }
+    return WriteMaybeUnalignedValue<T>(field_address(offset), value);
   }
 
   //

src/sandbox/sandboxed-pointer-inl.h

@@ -6,6 +6,7 @@
 #define V8_SANDBOX_SANDBOXED_POINTER_INL_H_
 
 #include "include/v8-internal.h"
+#include "src/common/ptr-compr.h"
 #include "src/execution/isolate.h"
 #include "src/sandbox/sandboxed-pointer.h"
 
@@ -22,7 +23,7 @@ V8_INLINE Address ReadSandboxedPointerField(Address field_address,
   Address pointer = cage_base.address() + offset;
   return pointer;
 #else
-  return base::ReadUnalignedValue<Address>(field_address);
+  return ReadMaybeUnalignedValue<Address>(field_address);
 #endif
 }
 
@@ -38,7 +39,7 @@ V8_INLINE void WriteSandboxedPointerField(Address field_address,
   base::WriteUnalignedValue<SandboxedPointer_t>(field_address,
                                                 sandboxed_pointer);
 #else
-  base::WriteUnalignedValue<Address>(field_address, pointer);
+  WriteMaybeUnalignedValue<Address>(field_address, pointer);
 #endif
 }
 

src/wasm/wasm-objects-inl.h

@@ -12,6 +12,7 @@
 #include <type_traits>
 
 #include "src/base/memory.h"
+#include "src/common/ptr-compr.h"
 #include "src/heap/heap-write-barrier-inl.h"
 #include "src/objects/contexts-inl.h"
 #include "src/objects/foreign.h"
@@ -66,30 +67,12 @@ CAST_ACCESSOR(WasmInstanceObject)
   ACCESSORS_CHECKED2(holder, name, type, offset,                        \
                      !value.IsUndefined(GetReadOnlyRoots(cage_base)), true)
 
-#define PRIMITIVE_ACCESSORS(holder, name, type, offset)                       \
-  type holder::name() const {                                                 \
-    if (COMPRESS_POINTERS_BOOL && alignof(type) > kTaggedSize) {              \
-      /* TODO(ishell, v8:8875): When pointer compression is enabled 8-byte */ \
-      /* size fields (external pointers, doubles and BigInt data) are only */ \
-      /* kTaggedSize aligned so we have to use unaligned pointer friendly  */ \
-      /* way of accessing them in order to avoid undefined behavior in C++ */ \
-      /* code. */                                                             \
-      return base::ReadUnalignedValue<type>(FIELD_ADDR(*this, offset));       \
-    } else {                                                                  \
-      return *reinterpret_cast<type const*>(FIELD_ADDR(*this, offset));       \
-    }                                                                         \
-  }                                                                           \
-  void holder::set_##name(type value) {                                       \
-    if (COMPRESS_POINTERS_BOOL && alignof(type) > kTaggedSize) {              \
-      /* TODO(ishell, v8:8875): When pointer compression is enabled 8-byte */ \
-      /* size fields (external pointers, doubles and BigInt data) are only */ \
-      /* kTaggedSize aligned so we have to use unaligned pointer friendly  */ \
-      /* way of accessing them in order to avoid undefined behavior in C++ */ \
-      /* code. */                                                             \
-      base::WriteUnalignedValue<type>(FIELD_ADDR(*this, offset), value);      \
-    } else {                                                                  \
-      *reinterpret_cast<type*>(FIELD_ADDR(*this, offset)) = value;            \
-    }                                                                         \
+#define PRIMITIVE_ACCESSORS(holder, name, type, offset)               \
+  type holder::name() const {                                         \
+    return ReadMaybeUnalignedValue<type>(FIELD_ADDR(*this, offset));  \
+  }                                                                   \
+  void holder::set_##name(type value) {                               \
+    WriteMaybeUnalignedValue<type>(FIELD_ADDR(*this, offset), value); \
   }
 
 #define SANDBOXED_POINTER_ACCESSORS(holder, name, type, offset)      \