Simplify semantics of Code::{safepoint,handler}_table_offset()

For these two fields, an offset of 0 was used to mark non-existent
tables. After this CL, it always contains a real offset even if the
table is empty. This matches behavior of other embedded metadata
(constant pool and code comments). Remnants of the old behavior still
remain in WasmCode and HandlerTable.

Drive-by: Update comments describing Code object layout.
Drive-by: Unify naming of Code offset constants.

Bug: v8:8758
Change-Id: Ia8a1f66988b6c294a026b96f4f272fc5583a8c30
Reviewed-on: https://chromium-review.googlesource.com/c/1451880Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59360}

src/code-desc.h

@@ -22,10 +22,6 @@ namespace internal {
 //  │ instructions │         data               │ free │  reloc info  │
 //  ├───────────────────────────────────────────┴──────┴──────────────┘
 
-// TODO(jgruber): Change Code::safepoint_table_offset() semantics to always
-// contain a real offset, and add has_safepoint_table() and
-// safepoint_table_size() helpers. Likewise for other inlined metadata.
-// TODO(jgruber): Update documentation about inlined metadata in code.h.
 // TODO(jgruber): Add a single chokepoint for specifying the instruction area
 // layout (i.e. the order of inlined metadata fields).
 // TODO(jgruber): Systematically maintain inlined metadata offsets and sizes

src/handler-table.cc

@@ -14,7 +14,9 @@ namespace v8 {
 namespace internal {
 
 HandlerTable::HandlerTable(Code code)
-    : HandlerTable(code->InstructionStart(), code->handler_table_offset()) {}
+    : HandlerTable(code->InstructionStart(), code->has_handler_table()
+                                                 ? code->handler_table_offset()
+                                                 : 0) {}
 
 HandlerTable::HandlerTable(BytecodeArray bytecode_array)
     : HandlerTable(bytecode_array->handler_table()) {}
@@ -29,6 +31,11 @@ HandlerTable::HandlerTable(ByteArray byte_array)
           reinterpret_cast<Address>(byte_array->GetDataStartAddress())) {
 }
 
+// TODO(jgruber,v8:8758): This constructor should eventually take the handler
+// table size in addition to the offset. That way the {HandlerTable} class
+// remains independent of how the offset/size is encoded in the various code
+// objects. This could even allow us to change the encoding to no longer expect
+// the "number of entries" in the beginning.
 HandlerTable::HandlerTable(Address instruction_start,
                            size_t handler_table_offset)
     : number_of_entries_(0),

src/heap/factory.cc

@@ -77,15 +77,6 @@ void InitializeCode(Heap* heap, Handle<Code> code, int object_size,
       !heap->memory_allocator()->code_range().is_empty(),
       heap->memory_allocator()->code_range().contains(code->address()));
 
-  // TODO(jgruber): Simplify the meaning of relevant fields on Code objects.
-  // Offset fields should probably always contain a real offset; or
-  // alternatively, constant pool and code comment offset fields should behave
-  // similarly (i.e. an offset of 0 means 'empty').
-  const int safepoint_table_offset =
-      desc.safepoint_table_size == 0 ? 0 : desc.safepoint_table_offset;
-  const int handler_table_offset =
-      desc.handler_table_size == 0 ? 0 : desc.handler_table_offset;
-
   constexpr bool kIsNotOffHeapTrampoline = false;
   const bool has_unwinding_info = desc.unwinding_info != nullptr;
 
@@ -96,8 +87,8 @@ void InitializeCode(Heap* heap, Handle<Code> code, int object_size,
   code->set_code_data_container(*data_container);
   code->set_deoptimization_data(*deopt_data);
   code->set_source_position_table(*source_position_table);
-  code->set_safepoint_table_offset(safepoint_table_offset);
-  code->set_handler_table_offset(handler_table_offset);
+  code->set_safepoint_table_offset(desc.safepoint_table_offset);
+  code->set_handler_table_offset(desc.handler_table_offset);
   code->set_constant_pool_offset(desc.constant_pool_offset);
   code->set_code_comments_offset(desc.code_comments_offset);
   code->set_builtin_index(builtin_index);
@@ -2871,18 +2862,16 @@ Handle<Code> Factory::NewOffHeapTrampolineFor(Handle<Code> code,
     const bool set_is_off_heap_trampoline = true;
     const int stack_slots =
         code->has_safepoint_info() ? code->stack_slots() : 0;
+    result->code_data_container()->set_kind_specific_flags(
+        code->code_data_container()->kind_specific_flags());
     result->initialize_flags(code->kind(), code->has_unwinding_info(),
                              code->is_turbofanned(), stack_slots,
                              set_is_off_heap_trampoline);
-    result->set_builtin_index(code->builtin_index());
+    result->set_safepoint_table_offset(code->safepoint_table_offset());
     result->set_handler_table_offset(code->handler_table_offset());
-    result->code_data_container()->set_kind_specific_flags(
-        code->code_data_container()->kind_specific_flags());
     result->set_constant_pool_offset(code->constant_pool_offset());
-    if (code->has_safepoint_info()) {
-      result->set_safepoint_table_offset(code->safepoint_table_offset());
-    }
     result->set_code_comments_offset(code->code_comments_offset());
+    result->set_builtin_index(code->builtin_index());
 
     // Replace the newly generated trampoline's RelocInfo ByteArray with the
     // canonical one stored in the roots to avoid duplicating it for every

src/objects-debug.cc

@@ -1202,8 +1202,10 @@ void CodeDataContainer::CodeDataContainerVerify(Isolate* isolate) {
 }
 
 void Code::CodeVerify(Isolate* isolate) {
-  // TODO(v8:8758): Should be <= handler_table_offset once we have real offsets.
-  CHECK_LE(safepoint_table_offset(), constant_pool_offset());
+  CHECK_IMPLIES(
+      has_safepoint_table(),
+      IsAligned(safepoint_table_offset(), static_cast<unsigned>(kIntSize)));
+  CHECK_LE(safepoint_table_offset(), handler_table_offset());
   CHECK_LE(handler_table_offset(), constant_pool_offset());
   CHECK_LE(constant_pool_offset(), code_comments_offset());
   CHECK_LE(code_comments_offset(), InstructionSize());

src/objects.cc

@@ -5386,17 +5386,13 @@ void ObjectVisitor::VisitRelocInfo(RelocIterator* it) {
 }
 
 int Code::safepoint_table_size() const {
-  if (safepoint_table_offset() == 0) return 0;
-  const int end_offset = handler_table_offset() != 0 ? handler_table_offset()
-                                                     : constant_pool_offset();
-  DCHECK_GE(end_offset - safepoint_table_offset(), 0);
-  return end_offset - safepoint_table_offset();
+  DCHECK_GE(handler_table_offset() - safepoint_table_offset(), 0);
+  return handler_table_offset() - safepoint_table_offset();
 }
 
 bool Code::has_safepoint_table() const { return safepoint_table_size() > 0; }
 
 int Code::handler_table_size() const {
-  if (handler_table_offset() == 0) return 0;
   DCHECK_GE(constant_pool_offset() - handler_table_offset(), 0);
   return constant_pool_offset() - handler_table_offset();
 }
@@ -5406,6 +5402,7 @@ bool Code::has_handler_table() const { return handler_table_size() > 0; }
 int Code::constant_pool_size() const {
   const int size = code_comments_offset() - constant_pool_offset();
   DCHECK_IMPLIES(!FLAG_enable_embedded_constant_pool, size == 0);
+  DCHECK_GE(size, 0);
   return size;
 }
 
@@ -5418,12 +5415,7 @@ int Code::code_comments_size() const {
 
 bool Code::has_code_comments() const { return code_comments_size() > 0; }
 
-int Code::ExecutableInstructionSize() const {
-  return safepoint_table_offset() != 0
-             ? safepoint_table_offset()
-             : (handler_table_offset() != 0 ? handler_table_offset()
-                                            : constant_pool_offset());
-}
+int Code::ExecutableInstructionSize() const { return safepoint_table_offset(); }
 
 void Code::ClearEmbeddedObjects(Heap* heap) {
   HeapObject undefined = ReadOnlyRoots(heap).undefined_value();

src/objects/code-inl.h

@@ -196,7 +196,9 @@ OBJECT_CONSTRUCTORS_IMPL(Code, HeapObject)
 NEVER_READ_ONLY_SPACE_IMPL(Code)
 
 INT_ACCESSORS(Code, raw_instruction_size, kInstructionSizeOffset)
+INT_ACCESSORS(Code, safepoint_table_offset, kSafepointTableOffsetOffset)
 INT_ACCESSORS(Code, handler_table_offset, kHandlerTableOffsetOffset)
+INT_ACCESSORS(Code, code_comments_offset, kCodeCommentsOffsetOffset)
 #define CODE_ACCESSORS(name, type, offset)           \
   ACCESSORS_CHECKED2(Code, name, type, offset, true, \
                      !Heap::InYoungGeneration(value))
@@ -482,17 +484,6 @@ int Code::stack_slots() const {
   return StackSlotsField::decode(READ_UINT32_FIELD(this, kFlagsOffset));
 }
 
-int Code::safepoint_table_offset() const {
-  return READ_INT32_FIELD(this, kSafepointTableOffsetOffset);
-}
-
-void Code::set_safepoint_table_offset(int offset) {
-  CHECK_LE(0, offset);
-  DCHECK(has_safepoint_info() || offset == 0);  // Allow zero initialization.
-  DCHECK(IsAligned(offset, static_cast<unsigned>(kIntSize)));
-  WRITE_INT32_FIELD(this, kSafepointTableOffsetOffset, offset);
-}
-
 bool Code::marked_for_deoptimization() const {
   DCHECK(kind() == OPTIMIZED_FUNCTION);
   int32_t flags = code_data_container()->kind_specific_flags();
@@ -540,13 +531,13 @@ bool Code::is_wasm_code() const { return kind() == WASM_FUNCTION; }
 
 int Code::constant_pool_offset() const {
   if (!FLAG_enable_embedded_constant_pool) return code_comments_offset();
-  return READ_INT_FIELD(this, kConstantPoolOffset);
+  return READ_INT_FIELD(this, kConstantPoolOffsetOffset);
 }
 
 void Code::set_constant_pool_offset(int value) {
   if (!FLAG_enable_embedded_constant_pool) return;
   DCHECK_LE(value, InstructionSize());
-  WRITE_INT_FIELD(this, kConstantPoolOffset, value);
+  WRITE_INT_FIELD(this, kConstantPoolOffsetOffset, value);
 }
 
 Address Code::constant_pool() const {
@@ -554,19 +545,6 @@ Address Code::constant_pool() const {
   return InstructionStart() + constant_pool_offset();
 }
 
-int Code::code_comments_offset() const {
-  int offset = READ_INT_FIELD(this, kCodeCommentsOffset);
-  DCHECK_LE(0, offset);
-  DCHECK_LE(offset, InstructionSize());
-  return offset;
-}
-
-void Code::set_code_comments_offset(int offset) {
-  DCHECK_LE(0, offset);
-  DCHECK_LE(offset, InstructionSize());
-  WRITE_INT_FIELD(this, kCodeCommentsOffset, offset);
-}
-
 Address Code::code_comments() const {
   if (!has_code_comments()) return kNullAddress;
   return InstructionStart() + code_comments_offset();

src/objects/code.h

@@ -275,8 +275,11 @@ class Code : public HeapObject {
   //  |       instructions       |
   //  |           ...            |
   //  +--------------------------+
-  //  |      relocation info     |
-  //  |           ...            |
+  //  |     embedded metadata    |  <-- safepoint_table_offset()
+  //  |           ...            |  <-- handler_table_offset()
+  //  |                          |  <-- constant_pool_offset()
+  //  |                          |  <-- code_comments_offset()
+  //  |                          |
   //  +--------------------------+  <-- raw_instruction_end()
   //
   // If has_unwinding_info() is false, raw_instruction_end() points to the first
@@ -370,25 +373,26 @@ class Code : public HeapObject {
   class OptimizedCodeIterator;
 
   // Layout description.
-#define CODE_FIELDS(V)                                                      \
-  V(kRelocationInfoOffset, kTaggedSize)                                     \
-  V(kDeoptimizationDataOffset, kTaggedSize)                                 \
-  V(kSourcePositionTableOffset, kTaggedSize)                                \
-  V(kCodeDataContainerOffset, kTaggedSize)                                  \
-  /* Data or code not directly visited by GC directly starts here. */       \
-  /* The serializer needs to copy bytes starting from here verbatim. */     \
-  /* Objects embedded into code is visited via reloc info. */               \
-  V(kDataStart, 0)                                                          \
-  V(kInstructionSizeOffset, kIntSize)                                       \
-  V(kFlagsOffset, kIntSize)                                                 \
-  V(kSafepointTableOffsetOffset, kIntSize)                                  \
-  V(kHandlerTableOffsetOffset, kIntSize)                                    \
-  V(kConstantPoolOffset, FLAG_enable_embedded_constant_pool ? kIntSize : 0) \
-  V(kBuiltinIndexOffset, kIntSize)                                          \
-  V(kCodeCommentsOffset, kIntSize)                                          \
-  /* Add padding to align the instruction start following right after */    \
-  /* the Code object header. */                                             \
-  V(kHeaderPaddingStart, CODE_POINTER_PADDING(kHeaderPaddingStart))         \
+#define CODE_FIELDS(V)                                                   \
+  V(kRelocationInfoOffset, kTaggedSize)                                  \
+  V(kDeoptimizationDataOffset, kTaggedSize)                              \
+  V(kSourcePositionTableOffset, kTaggedSize)                             \
+  V(kCodeDataContainerOffset, kTaggedSize)                               \
+  /* Data or code not directly visited by GC directly starts here. */    \
+  /* The serializer needs to copy bytes starting from here verbatim. */  \
+  /* Objects embedded into code is visited via reloc info. */            \
+  V(kDataStart, 0)                                                       \
+  V(kInstructionSizeOffset, kIntSize)                                    \
+  V(kFlagsOffset, kIntSize)                                              \
+  V(kSafepointTableOffsetOffset, kIntSize)                               \
+  V(kHandlerTableOffsetOffset, kIntSize)                                 \
+  V(kConstantPoolOffsetOffset,                                           \
+    FLAG_enable_embedded_constant_pool ? kIntSize : 0)                   \
+  V(kCodeCommentsOffsetOffset, kIntSize)                                 \
+  V(kBuiltinIndexOffset, kIntSize)                                       \
+  /* Add padding to align the instruction start following right after */ \
+  /* the Code object header. */                                          \
+  V(kHeaderPaddingStart, CODE_POINTER_PADDING(kHeaderPaddingStart))      \
   V(kHeaderSize, 0)
 
   DEFINE_FIELD_OFFSET_CONSTANTS(HeapObject::kHeaderSize, CODE_FIELDS)

src/wasm/wasm-code-manager.cc

@@ -540,16 +540,23 @@ WasmCode* NativeModule::AddAnonymousCode(Handle<Code> code, WasmCode::Kind kind,
   Vector<const byte> instructions(
       reinterpret_cast<byte*>(code->InstructionStart()),
       static_cast<size_t>(code->InstructionSize()));
-  int stack_slots = code->has_safepoint_info() ? code->stack_slots() : 0;
-  int safepoint_table_offset =
-      code->has_safepoint_info() ? code->safepoint_table_offset() : 0;
+  const int stack_slots = code->has_safepoint_info() ? code->stack_slots() : 0;
+
+  // TODO(jgruber,v8:8758): Remove this translation. It exists only because
+  // Code objects contains real offsets but WasmCode expects an offset of 0 to
+  // mean 'empty'.
+  const int safepoint_table_offset =
+      code->has_safepoint_table() ? code->safepoint_table_offset() : 0;
+  const int handler_table_offset =
+      code->has_handler_table() ? code->handler_table_offset() : 0;
+
   WasmCode* ret =
       AddOwnedCode(WasmCode::kAnonymousFuncIndex,  // index
                    instructions,                   // instructions
                    stack_slots,                    // stack_slots
                    0,                              // tagged_parameter_slots
                    safepoint_table_offset,         // safepoint_table_offset
-                   code->handler_table_offset(),   // handler_table_offset
+                   handler_table_offset,           // handler_table_offset
                    code->constant_pool_offset(),   // constant_pool_offset
                    code->code_comments_offset(),   // code_comments_offset
                    instructions.size(),            // unpadded_binary_size
@@ -601,8 +608,9 @@ WasmCode* NativeModule::AddCode(
            desc.reloc_size);
   }
 
-  // TODO(jgruber): Remove this translation. It exists only because CodeDesc
-  // contains real offsets but WasmCode expects an offset of 0 to mean 'empty'.
+  // TODO(jgruber,v8:8758): Remove this translation. It exists only because
+  // CodeDesc contains real offsets but WasmCode expects an offset of 0 to mean
+  // 'empty'.
   const int safepoint_table_offset =
       desc.safepoint_table_size == 0 ? 0 : desc.safepoint_table_offset;
   const int handler_table_offset =