[sparkplug] source_position_table != bytecode_offset_table

- Make explicit that Code::bytecode_offset_table is only used with
  sparkplug code.
- Add more DCHECKs on CodeBuilder setter
- Code::source_position_table is always a ByteArray

Bug: v8:11429
Change-Id: I27f84f0d6e325ca5b616412084227b9a7198d367
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2721769Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73217}

src/diagnostics/perf-jit.cc

@@ -340,12 +340,7 @@ void PerfJitLogger::LogWriteDebugInfo(Handle<Code> code,
                                       Handle<SharedFunctionInfo> shared) {
   DisallowGarbageCollection no_gc;
   // TODO(v8:11429,cbruni): add proper baseline source position iterator
-  bool is_baseline = code->kind() == CodeKind::BASELINE;
-  ByteArray source_position_table = code->SourcePositionTable();
-  if (is_baseline) {
-    source_position_table =
-        shared->GetBytecodeArray(shared->GetIsolate()).SourcePositionTable();
-  }
+  ByteArray source_position_table = code->SourcePositionTable(*shared);
   // Compute the entry count and get the name of the script.
   uint32_t entry_count = 0;
   for (SourcePositionTableIterator iterator(source_position_table);

src/extensions/statistics-extension.cc

@@ -4,9 +4,11 @@
 
 #include "src/extensions/statistics-extension.h"
 
+#include "src/common/assert-scope.h"
 #include "src/execution/isolate.h"
 #include "src/heap/heap-inl.h"  // crbug.com/v8/8499
 #include "src/logging/counters.h"
+#include "src/roots/roots.h"
 
 namespace v8 {
 namespace internal {
@@ -124,23 +126,28 @@ void StatisticsExtension::GetCounters(
               "amount_of_external_allocated_memory");
   args.GetReturnValue().Set(result);
 
+  DisallowGarbageCollection no_gc;
   HeapObjectIterator iterator(
       reinterpret_cast<Isolate*>(args.GetIsolate())->heap());
   int reloc_info_total = 0;
   int source_position_table_total = 0;
   for (HeapObject obj = iterator.Next(); !obj.is_null();
        obj = iterator.Next()) {
+    Object maybe_source_positions;
     if (obj.IsCode()) {
       Code code = Code::cast(obj);
       reloc_info_total += code.relocation_info().Size();
-      ByteArray source_position_table = code.SourcePositionTable();
-      if (source_position_table.length() > 0) {
-        source_position_table_total += code.SourcePositionTable().Size();
-      }
+      maybe_source_positions = code.source_position_table();
     } else if (obj.IsBytecodeArray()) {
-      source_position_table_total +=
-          BytecodeArray::cast(obj).SourcePositionTable().Size();
+      maybe_source_positions =
+          BytecodeArray::cast(obj).source_position_table(kAcquireLoad);
+    } else {
+      continue;
     }
+    if (!maybe_source_positions.IsByteArray()) continue;
+    ByteArray source_positions = ByteArray::cast(maybe_source_positions);
+    if (source_positions.length() == 0) continue;
+    source_position_table_total += source_positions.Size();
   }
 
   AddNumber(args.GetIsolate(), result, reloc_info_total,

src/heap/factory.cc

@@ -74,7 +74,7 @@ Factory::CodeBuilder::CodeBuilder(Isolate* isolate, const CodeDesc& desc,
     : isolate_(isolate),
       code_desc_(desc),
       kind_(kind),
-      source_position_table_(isolate_->factory()->empty_byte_array()) {}
+      position_table_(isolate_->factory()->empty_byte_array()) {}
 
 MaybeHandle<Code> Factory::CodeBuilder::BuildInternal(
     bool retry_allocation_or_fail) {
@@ -167,7 +167,11 @@ MaybeHandle<Code> Factory::CodeBuilder::BuildInternal(
     code->set_inlined_bytecode_size(inlined_bytecode_size_);
     code->set_code_data_container(*data_container, kReleaseStore);
     code->set_deoptimization_data(*deoptimization_data_);
-    code->set_source_position_table(*source_position_table_);
+    if (kind_ == CodeKind::BASELINE) {
+      code->set_bytecode_offset_table(*position_table_);
+    } else {
+      code->set_source_position_table(*position_table_);
+    }
     code->set_handler_table_offset(code_desc_.handler_table_offset_relative());
     code->set_constant_pool_offset(code_desc_.constant_pool_offset_relative());
     code->set_code_comments_offset(code_desc_.code_comments_offset_relative());

src/heap/factory.h

@@ -826,26 +826,29 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
     }
 
     CodeBuilder& set_builtin_index(int32_t builtin_index) {
+      DCHECK_IMPLIES(builtin_index != Builtins::kNoBuiltinId,
+                     !CodeKindIsJSFunction(kind_));
       builtin_index_ = builtin_index;
       return *this;
     }
 
     CodeBuilder& set_inlined_bytecode_size(uint32_t size) {
+      DCHECK_IMPLIES(size != 0, CodeKindIsOptimizedJSFunction(kind_));
       inlined_bytecode_size_ = size;
       return *this;
     }
 
     CodeBuilder& set_source_position_table(Handle<ByteArray> table) {
+      DCHECK_NE(kind_, CodeKind::BASELINE);
       DCHECK(!table.is_null());
-      source_position_table_ = table;
+      position_table_ = table;
       return *this;
     }
 
     CodeBuilder& set_bytecode_offset_table(Handle<ByteArray> table) {
+      DCHECK_EQ(kind_, CodeKind::BASELINE);
       DCHECK(!table.is_null());
-      // TODO(v8:11429): Rename this and clean up calls to SourcePositionTable
-      // under Baseline.
-      source_position_table_ = table;
+      position_table_ = table;
       return *this;
     }
 
@@ -857,11 +860,13 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
     }
 
     CodeBuilder& set_is_turbofanned() {
+      DCHECK(!CodeKindIsUnoptimizedJSFunction(kind_));
       is_turbofanned_ = true;
       return *this;
     }
 
     CodeBuilder& set_is_executable(bool executable) {
+      DCHECK_EQ(kind_, CodeKind::BUILTIN);
       is_executable_ = executable;
       return *this;
     }
@@ -896,8 +901,9 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
     int32_t builtin_index_ = Builtins::kNoBuiltinId;
     uint32_t inlined_bytecode_size_ = 0;
     int32_t kind_specific_flags_ = 0;
-    // Contains bytecode offset table for baseline
-    Handle<ByteArray> source_position_table_;
+    // Either source_position_table for non-baseline code
+    // or bytecode_offset_table for baseline code.
+    Handle<ByteArray> position_table_;
     Handle<DeoptimizationData> deoptimization_data_ =
         DeoptimizationData::Empty(isolate_);
     BasicBlockProfilerData* profiler_data_ = nullptr;

src/logging/log.cc

@@ -1234,7 +1234,7 @@ void Logger::LogSourceCodeInformation(Handle<AbstractCode> code,
   // iteration.
   bool hasInlined = false;
   if (code->kind() != CodeKind::BASELINE) {
-    SourcePositionTableIterator iterator(code->source_position_table());
+    SourcePositionTableIterator iterator(code->SourcePositionTable(*shared));
     for (; !iterator.done(); iterator.Advance()) {
       SourcePosition pos = iterator.source_position();
       msg << "C" << iterator.code_offset() << "O" << pos.ScriptOffset();

src/objects/code-inl.h

@@ -17,6 +17,7 @@
 #include "src/objects/map-inl.h"
 #include "src/objects/maybe-object-inl.h"
 #include "src/objects/oddball.h"
+#include "src/objects/shared-function-info.h"
 #include "src/objects/smi-inl.h"
 
 // Has to be the last include (doesn't have include guards):
@@ -56,9 +57,18 @@ int AbstractCode::InstructionSize() {
   }
 }
 
-ByteArray AbstractCode::source_position_table() {
+ByteArray AbstractCode::SourcePositionTableInternal() {
   if (IsCode()) {
-    return GetCode().SourcePositionTable();
+    DCHECK_NE(GetCode().kind(), CodeKind::BASELINE);
+    return GetCode().source_position_table();
+  } else {
+    return GetBytecodeArray().SourcePositionTable();
+  }
+}
+
+ByteArray AbstractCode::SourcePositionTable(SharedFunctionInfo sfi) {
+  if (IsCode()) {
+    return GetCode().SourcePositionTable(sfi);
   } else {
     return GetBytecodeArray().SourcePositionTable();
   }
@@ -178,7 +188,14 @@ INT32_ACCESSORS(Code, unwinding_info_offset, kUnwindingInfoOffsetOffset)
 
 CODE_ACCESSORS(relocation_info, ByteArray, kRelocationInfoOffset)
 CODE_ACCESSORS(deoptimization_data, FixedArray, kDeoptimizationDataOffset)
-CODE_ACCESSORS(source_position_table, Object, kSourcePositionTableOffset)
+#define IS_BASELINE() (kind() == CodeKind::BASELINE)
+ACCESSORS_CHECKED2(Code, source_position_table, ByteArray, kPositionTableOffset,
+                   !IS_BASELINE(),
+                   !IS_BASELINE() && !ObjectInYoungGeneration(value))
+ACCESSORS_CHECKED2(Code, bytecode_offset_table, ByteArray, kPositionTableOffset,
+                   IS_BASELINE(),
+                   IS_BASELINE() && !ObjectInYoungGeneration(value))
+#undef IS_BASELINE
 // Concurrent marker needs to access kind specific flags in code data container.
 RELEASE_ACQUIRE_CODE_ACCESSORS(code_data_container, CodeDataContainer,
                                kCodeDataContainerOffset)
@@ -188,7 +205,7 @@ RELEASE_ACQUIRE_CODE_ACCESSORS(code_data_container, CodeDataContainer,
 void Code::WipeOutHeader() {
   WRITE_FIELD(*this, kRelocationInfoOffset, Smi::FromInt(0));
   WRITE_FIELD(*this, kDeoptimizationDataOffset, Smi::FromInt(0));
-  WRITE_FIELD(*this, kSourcePositionTableOffset, Smi::FromInt(0));
+  WRITE_FIELD(*this, kPositionTableOffset, Smi::FromInt(0));
   WRITE_FIELD(*this, kCodeDataContainerOffset, Smi::FromInt(0));
 }
 
@@ -205,12 +222,12 @@ void Code::clear_padding() {
   memset(reinterpret_cast<void*>(raw_body_end()), 0, trailing_padding_size);
 }
 
-ByteArray Code::SourcePositionTable() const {
-  Object maybe_table = source_position_table();
-  if (maybe_table.IsByteArray()) return ByteArray::cast(maybe_table);
-  ReadOnlyRoots roots = GetReadOnlyRoots();
-  DCHECK(maybe_table.IsUndefined(roots) || maybe_table.IsException(roots));
-  return roots.empty_byte_array();
+ByteArray Code::SourcePositionTable(SharedFunctionInfo sfi) const {
+  DisallowGarbageCollection no_gc;
+  if (kind() == CodeKind::BASELINE) {
+    return sfi.GetBytecodeArray(sfi.GetIsolate()).SourcePositionTable();
+  }
+  return source_position_table();
 }
 
 Object Code::next_code_link() const {
@@ -336,7 +353,7 @@ int Code::GetBytecodeOffsetForBaselinePC(Address baseline_pc) {
   CHECK(!is_baseline_prologue_builtin());
   if (is_baseline_leave_frame_builtin()) return kFunctionExitBytecodeOffset;
   CHECK_EQ(kind(), CodeKind::BASELINE);
-  ByteArray data = ByteArray::cast(source_position_table());
+  ByteArray data = ByteArray::cast(bytecode_offset_table());
   Address lookup_pc = 0;
   Address pc = baseline_pc - InstructionStart();
   int index = 0;
@@ -355,7 +372,7 @@ uintptr_t Code::GetBaselinePCForBytecodeOffset(int bytecode_offset,
                                                bool precise) {
   DisallowGarbageCollection no_gc;
   CHECK_EQ(kind(), CodeKind::BASELINE);
-  ByteArray data = ByteArray::cast(source_position_table());
+  ByteArray data = ByteArray::cast(bytecode_offset_table());
   intptr_t pc = 0;
   int index = 0;
   int offset = 0;

src/objects/code.cc

@@ -10,12 +10,14 @@
 #include "src/codegen/cpu-features.h"
 #include "src/codegen/reloc-info.h"
 #include "src/codegen/safepoint-table.h"
+#include "src/codegen/source-position.h"
 #include "src/deoptimizer/deoptimizer.h"
 #include "src/execution/isolate-utils.h"
 #include "src/interpreter/bytecode-array-iterator.h"
 #include "src/interpreter/bytecode-decoder.h"
 #include "src/interpreter/interpreter.h"
 #include "src/objects/allocation-site-inl.h"
+#include "src/objects/code-kind.h"
 #include "src/roots/roots-inl.h"
 #include "src/snapshot/embedded/embedded-data.h"
 #include "src/utils/ostreams.h"
@@ -190,8 +192,10 @@ Address Code::OffHeapMetadataEnd() const {
          d.MetadataSizeOfBuiltin(builtin_index());
 }
 
+// TODO(cbruni): Move to BytecodeArray
 int AbstractCode::SourcePosition(int offset) {
-  Object maybe_table = source_position_table();
+  CHECK_NE(kind(), CodeKind::BASELINE);
+  Object maybe_table = SourcePositionTableInternal();
   if (maybe_table.IsException()) return kNoSourcePosition;
 
   ByteArray source_position_table = ByteArray::cast(maybe_table);
@@ -208,13 +212,15 @@ int AbstractCode::SourcePosition(int offset) {
   return position;
 }
 
+// TODO(cbruni): Move to BytecodeArray
 int AbstractCode::SourceStatementPosition(int offset) {
+  CHECK_NE(kind(), CodeKind::BASELINE);
   // First find the closest position.
   int position = SourcePosition(offset);
   // Now find the closest statement position before the position.
   int statement_position = 0;
-  for (SourcePositionTableIterator it(source_position_table()); !it.done();
-       it.Advance()) {
+  for (SourcePositionTableIterator it(SourcePositionTableInternal());
+       !it.done(); it.Advance()) {
     if (it.is_statement()) {
       int p = it.source_position().ScriptOffset();
       if (statement_position < p && p <= position) {
@@ -535,10 +541,12 @@ void Code::Disassemble(const char* name, std::ostream& os, Isolate* isolate,
   }
   os << "\n";
 
+  // TODO(cbruni): add support for baseline code.
   if (kind() != CodeKind::BASELINE) {
     {
       SourcePositionTableIterator it(
-          SourcePositionTable(), SourcePositionTableIterator::kJavaScriptOnly);
+          source_position_table(),
+          SourcePositionTableIterator::kJavaScriptOnly);
       if (!it.done()) {
         os << "Source positions:\n pc offset  position\n";
         for (; !it.done(); it.Advance()) {
@@ -552,7 +560,7 @@ void Code::Disassemble(const char* name, std::ostream& os, Isolate* isolate,
 
     {
       SourcePositionTableIterator it(
-          SourcePositionTable(), SourcePositionTableIterator::kExternalOnly);
+          source_position_table(), SourcePositionTableIterator::kExternalOnly);
       if (!it.done()) {
         os << "External Source positions:\n pc offset  fileid  line\n";
         for (; !it.done(); it.Advance()) {

src/objects/code.h

@@ -13,6 +13,7 @@
 #include "src/objects/fixed-array.h"
 #include "src/objects/heap-object.h"
 #include "src/objects/objects.h"
+#include "src/objects/shared-function-info.h"
 #include "src/objects/struct.h"
 
 // Has to be the last include (doesn't have include guards):
@@ -220,12 +221,16 @@ class Code : public HeapObject {
   // [deoptimization_data]: Array containing data for deopt.
   DECL_ACCESSORS(deoptimization_data, FixedArray)
 
-  // [source_position_table]: ByteArray for the source positions table.
-  DECL_ACCESSORS(source_position_table, Object)
+  // [source_position_table]: ByteArray for the source positions table for
+  // non-baseline code.
+  DECL_ACCESSORS(source_position_table, ByteArray)
+  // [bytecode_offset_table]: ByteArray for the bytecode offset for baseline
+  // code.
+  DECL_ACCESSORS(bytecode_offset_table, ByteArray)
 
   // If source positions have not been collected or an exception has been thrown
   // this will return empty_byte_array.
-  inline ByteArray SourcePositionTable() const;
+  inline ByteArray SourcePositionTable(SharedFunctionInfo sfi) const;
 
   // [code_data_container]: A container indirection for all mutable fields.
   DECL_RELEASE_ACQUIRE_ACCESSORS(code_data_container, CodeDataContainer)
@@ -428,7 +433,7 @@ class Code : public HeapObject {
 #define CODE_FIELDS(V)                                                        \
   V(kRelocationInfoOffset, kTaggedSize)                                       \
   V(kDeoptimizationDataOffset, kTaggedSize)                                   \
-  V(kSourcePositionTableOffset, kTaggedSize)                                  \
+  V(kPositionTableOffset, 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. */       \
@@ -577,8 +582,8 @@ class AbstractCode : public HeapObject {
   // at instruction_start.
   inline int InstructionSize();
 
-  // Return the source position table.
-  inline ByteArray source_position_table();
+  // Return the source position table for interpreter code.
+  inline ByteArray SourcePositionTable(SharedFunctionInfo sfi);
 
   void DropStackFrameCache();
 
@@ -600,6 +605,9 @@ class AbstractCode : public HeapObject {
   static const int kMaxLoopNestingMarker = 6;
 
   OBJECT_CONSTRUCTORS(AbstractCode, HeapObject);
+
+ private:
+  inline ByteArray SourcePositionTableInternal();
 };
 
 // Dependent code is a singly linked list of weak fixed arrays. Each array

src/objects/objects-body-descriptors-inl.h

@@ -619,9 +619,8 @@ class Code::BodyDescriptor final : public BodyDescriptorBase {
   STATIC_ASSERT(kRelocationInfoOffset + kTaggedSize ==
                 kDeoptimizationDataOffset);
   STATIC_ASSERT(kDeoptimizationDataOffset + kTaggedSize ==
-                kSourcePositionTableOffset);
-  STATIC_ASSERT(kSourcePositionTableOffset + kTaggedSize ==
-                kCodeDataContainerOffset);
+                kPositionTableOffset);
+  STATIC_ASSERT(kPositionTableOffset + kTaggedSize == kCodeDataContainerOffset);
   STATIC_ASSERT(kCodeDataContainerOffset + kTaggedSize == kDataStart);
 
   static bool IsValidSlot(Map map, HeapObject obj, int offset) {

src/profiler/heap-snapshot-generator.cc

@@ -1184,10 +1184,17 @@ void V8HeapExplorer::ExtractCodeReferences(HeapEntry* entry, Code code) {
   TagObject(code.deoptimization_data(), "(code deopt data)");
   SetInternalReference(entry, "deoptimization_data", code.deoptimization_data(),
                        Code::kDeoptimizationDataOffset);
-  TagObject(code.source_position_table(), "(source position table)");
-  SetInternalReference(entry, "source_position_table",
-                       code.source_position_table(),
-                       Code::kSourcePositionTableOffset);
+  if (code.kind() == CodeKind::BASELINE) {
+    TagObject(code.bytecode_offset_table(), "(bytecode offset table)");
+    SetInternalReference(entry, "bytecode_offset_table",
+                         code.bytecode_offset_table(),
+                         Code::kPositionTableOffset);
+  } else {
+    TagObject(code.source_position_table(), "(source position table)");
+    SetInternalReference(entry, "source_position_table",
+                         code.source_position_table(),
+                         Code::kPositionTableOffset);
+  }
 }
 
 void V8HeapExplorer::ExtractCellReferences(HeapEntry* entry, Cell cell) {

src/profiler/profiler-listener.cc

@@ -119,11 +119,7 @@ void ProfilerListener::CodeCreateEvent(LogEventsAndTags tag,
     // TODO(v8:11429,cbruni): improve iteration for baseline code
     bool is_baseline = abstract_code->kind() == CodeKind::BASELINE;
     Handle<ByteArray> source_position_table(
-        abstract_code->source_position_table(), isolate_);
-    if (is_baseline) {
-      source_position_table = handle(
-          shared->GetBytecodeArray(isolate_).SourcePositionTable(), isolate_);
-    }
+        abstract_code->SourcePositionTable(*shared), isolate_);
     // Add each position to the source position table and store inlining stacks
     // for inline positions. We store almost the same information in the
     // profiler as is stored on the code object, except that we transform source

src/snapshot/embedded/embedded-file-writer.cc

@@ -289,9 +289,9 @@ void EmbeddedFileWriter::PrepareBuiltinSourcePositionMap(Builtins* builtins) {
     // Verify that the code object is still the "real code" and not a
     // trampoline (which wouldn't have source positions).
     DCHECK(!code.is_off_heap_trampoline());
-    std::vector<unsigned char> data(
-        code.SourcePositionTable().GetDataStartAddress(),
-        code.SourcePositionTable().GetDataEndAddress());
+    ByteArray source_position_table = code.source_position_table();
+    std::vector<unsigned char> data(source_position_table.GetDataStartAddress(),
+                                    source_position_table.GetDataEndAddress());
     source_positions_[i] = data;
   }
 }

src/wasm/wasm-code-manager.cc

@@ -892,7 +892,7 @@ WasmCode* NativeModule::AddCodeForTesting(Handle<Code> code) {
     reloc_info = OwnedVector<byte>::Of(
         Vector<byte>{code->relocation_start(), relocation_size});
   }
-  Handle<ByteArray> source_pos_table(code->SourcePositionTable(),
+  Handle<ByteArray> source_pos_table(code->source_position_table(),
                                      code->GetIsolate());
   OwnedVector<byte> source_pos =
       OwnedVector<byte>::NewForOverwrite(source_pos_table->length());