[compiler] Add extra synchronization to source position table's get/set

Original CL by neis@: http://crrev.com/c/v8/v8/+/2362693/1

Bug: v8:7790, v8:10853
Fixed: v8:10853
Change-Id: If0bd45e9dfb00f8ef1a358953dab1f5e1c9ae29e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2387960Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69742}

src/codegen/compiler.cc

@@ -1655,7 +1655,7 @@ bool Compiler::CollectSourcePositions(Isolate* isolate,
       shared_info->GetDebugInfo().HasInstrumentedBytecodeArray()) {
     ByteArray source_position_table =
         job->compilation_info()->bytecode_array()->SourcePositionTable();
-    shared_info->GetDebugBytecodeArray().set_source_position_table(
+    shared_info->GetDebugBytecodeArray().set_synchronized_source_position_table(
         source_position_table);
   }
 

src/compiler/bytecode-graph-builder.cc

@@ -977,7 +977,7 @@ BytecodeGraphBuilder::BytecodeGraphBuilder(
           bytecode_array().parameter_count(), bytecode_array().register_count(),
           shared_info.object())),
       source_position_iterator_(std::make_unique<SourcePositionTableIterator>(
-          bytecode_array().source_positions())),
+          bytecode_array().SourcePositionTable())),
       bytecode_iterator_(
           std::make_unique<OffHeapBytecodeArray>(bytecode_array())),
       bytecode_analysis_(broker_->GetBytecodeAnalysis(

src/compiler/heap-refs.h

@@ -735,8 +735,7 @@ class BytecodeArrayRef : public FixedArrayBaseRef {
   uint8_t get(int index) const;
   Address GetFirstBytecodeAddress() const;
 
-  // Source position table.
-  Handle<ByteArray> source_positions() const;
+  Handle<ByteArray> SourcePositionTable() const;
 
   // Constant pool access.
   Handle<Object> GetConstantAtIndex(int index) const;

src/compiler/js-heap-broker.cc

@@ -3299,9 +3299,8 @@ void BytecodeArrayRef::SerializeForCompilation() {
   data()->AsBytecodeArray()->SerializeForCompilation(broker());
 }
 
-Handle<ByteArray> BytecodeArrayRef::source_positions() const {
-  return broker()->CanonicalPersistentHandle(
-      object()->SourcePositionTableIfCollected());
+Handle<ByteArray> BytecodeArrayRef::SourcePositionTable() const {
+  return broker()->CanonicalPersistentHandle(object()->SourcePositionTable());
 }
 
 Address BytecodeArrayRef::handler_table_address() const {

src/debug/liveedit.cc

@@ -1025,7 +1025,7 @@ void TranslateSourcePositionTable(Isolate* isolate, Handle<BytecodeArray> code,
 
   Handle<ByteArray> new_source_position_table(
       builder.ToSourcePositionTable(isolate));
-  code->set_source_position_table(*new_source_position_table);
+  code->set_synchronized_source_position_table(*new_source_position_table);
   LOG_CODE_EVENT(isolate,
                  CodeLinePosInfoRecordEvent(code->GetFirstBytecodeAddress(),
                                             *new_source_position_table));

src/diagnostics/objects-debug.cc

@@ -291,9 +291,9 @@ void BytecodeArray::BytecodeArrayVerify(Isolate* isolate) {
   CHECK(IsBytecodeArray());
   CHECK(constant_pool().IsFixedArray());
   VerifyHeapPointer(isolate, constant_pool());
-  CHECK(source_position_table().IsUndefined() ||
-        source_position_table().IsException() ||
-        source_position_table().IsByteArray());
+  CHECK(synchronized_source_position_table().IsUndefined() ||
+        synchronized_source_position_table().IsException() ||
+        synchronized_source_position_table().IsByteArray());
   CHECK(handler_table().IsByteArray());
 }
 

src/heap/factory-base.cc

@@ -195,7 +195,8 @@ Handle<BytecodeArray> FactoryBase<Impl>::NewBytecodeArray(
   instance->set_bytecode_age(BytecodeArray::kNoAgeBytecodeAge);
   instance->set_constant_pool(*constant_pool);
   instance->set_handler_table(read_only_roots().empty_byte_array());
-  instance->set_source_position_table(read_only_roots().undefined_value());
+  instance->set_synchronized_source_position_table(
+      read_only_roots().undefined_value());
   CopyBytes(reinterpret_cast<byte*>(instance->GetFirstBytecodeAddress()),
             raw_bytecodes, length);
   instance->clear_padding();

src/heap/factory.cc

@@ -2162,7 +2162,8 @@ Handle<BytecodeArray> Factory::CopyBytecodeArray(
       bytecode_array->incoming_new_target_or_generator_register());
   copy->set_constant_pool(bytecode_array->constant_pool());
   copy->set_handler_table(bytecode_array->handler_table());
-  copy->set_source_position_table(bytecode_array->source_position_table());
+  copy->set_synchronized_source_position_table(
+      bytecode_array->synchronized_source_position_table());
   copy->set_osr_loop_nesting_level(bytecode_array->osr_loop_nesting_level());
   copy->set_bytecode_age(bytecode_array->bytecode_age());
   bytecode_array->CopyBytecodesTo(*copy);

src/interpreter/interpreter.cc

@@ -250,7 +250,7 @@ InterpreterCompilationJob::Status InterpreterCompilationJob::DoFinalizeJobImpl(
       SourcePositionTableBuilder::RecordingMode::RECORD_SOURCE_POSITIONS) {
     Handle<ByteArray> source_position_table =
         generator()->FinalizeSourcePositionTable(isolate);
-    bytecodes->set_source_position_table(*source_position_table);
+    bytecodes->set_synchronized_source_position_table(*source_position_table);
   }
 
   if (ShouldPrintBytecode(shared_info)) {

src/objects/code-inl.h

@@ -208,21 +208,12 @@ void Code::clear_padding() {
          CodeSize() - (data_end - address()));
 }
 
-ByteArray Code::SourcePositionTableIfCollected() const {
-  ReadOnlyRoots roots = GetReadOnlyRoots();
-  Object maybe_table = source_position_table();
-  if (maybe_table.IsUndefined(roots) || maybe_table.IsException(roots)) {
-    return roots.empty_byte_array();
-  }
-  DCHECK(maybe_table.IsByteArray());
-  return ByteArray::cast(maybe_table);
-}
-
 ByteArray Code::SourcePositionTable() const {
   Object maybe_table = source_position_table();
-  DCHECK(!maybe_table.IsUndefined() && !maybe_table.IsException());
-  DCHECK(maybe_table.IsByteArray());
-  return ByteArray::cast(maybe_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();
 }
 
 Object Code::next_code_link() const {
@@ -710,8 +701,8 @@ int32_t BytecodeArray::parameter_count() const {
 
 ACCESSORS(BytecodeArray, constant_pool, FixedArray, kConstantPoolOffset)
 ACCESSORS(BytecodeArray, handler_table, ByteArray, kHandlerTableOffset)
-ACCESSORS(BytecodeArray, source_position_table, Object,
-          kSourcePositionTableOffset)
+SYNCHRONIZED_ACCESSORS(BytecodeArray, synchronized_source_position_table,
+                       Object, kSourcePositionTableOffset)
 
 void BytecodeArray::clear_padding() {
   int data_size = kHeaderSize + length();
@@ -724,40 +715,37 @@ Address BytecodeArray::GetFirstBytecodeAddress() {
 }
 
 bool BytecodeArray::HasSourcePositionTable() const {
-  Object maybe_table = source_position_table();
+  Object maybe_table = synchronized_source_position_table();
   return !(maybe_table.IsUndefined() || DidSourcePositionGenerationFail());
 }
 
 bool BytecodeArray::DidSourcePositionGenerationFail() const {
-  return source_position_table().IsException();
+  return synchronized_source_position_table().IsException();
 }
 
 void BytecodeArray::SetSourcePositionsFailedToCollect() {
-  set_source_position_table(GetReadOnlyRoots().exception());
+  set_synchronized_source_position_table(GetReadOnlyRoots().exception());
 }
 
 ByteArray BytecodeArray::SourcePositionTable() const {
-  Object maybe_table = source_position_table();
+  // WARNING: This function may be called from a background thread, hence
+  // changes to how it accesses the heap can easily lead to bugs.
+  Object maybe_table = synchronized_source_position_table();
   if (maybe_table.IsByteArray()) return ByteArray::cast(maybe_table);
   ReadOnlyRoots roots = GetReadOnlyRoots();
-  DCHECK(maybe_table.IsException(roots));
+  DCHECK(maybe_table.IsUndefined(roots) || maybe_table.IsException(roots));
   return roots.empty_byte_array();
 }
 
-ByteArray BytecodeArray::SourcePositionTableIfCollected() const {
-  if (!HasSourcePositionTable()) return GetReadOnlyRoots().empty_byte_array();
-
-  return SourcePositionTable();
-}
-
 int BytecodeArray::BytecodeArraySize() { return SizeFor(this->length()); }
 
 int BytecodeArray::SizeIncludingMetadata() {
   int size = BytecodeArraySize();
   size += constant_pool().Size();
   size += handler_table().Size();
-  if (HasSourcePositionTable()) {
-    size += SourcePositionTable().Size();
+  ByteArray table = SourcePositionTable();
+  if (table.length() != 0) {
+    size += table.Size();
   }
   return size;
 }

src/objects/code.cc

@@ -712,8 +712,7 @@ void Code::Disassemble(const char* name, std::ostream& os, Isolate* isolate,
 
   {
     SourcePositionTableIterator it(
-        SourcePositionTableIfCollected(),
-        SourcePositionTableIterator::kJavaScriptOnly);
+        SourcePositionTable(), SourcePositionTableIterator::kJavaScriptOnly);
     if (!it.done()) {
       os << "Source positions:\n pc offset  position\n";
       for (; !it.done(); it.Advance()) {
@@ -726,7 +725,7 @@ void Code::Disassemble(const char* name, std::ostream& os, Isolate* isolate,
   }
 
   {
-    SourcePositionTableIterator it(SourcePositionTableIfCollected(),
+    SourcePositionTableIterator it(SourcePositionTable(),
                                    SourcePositionTableIterator::kExternalOnly);
     if (!it.done()) {
       os << "External Source positions:\n pc offset  fileid  line\n";
@@ -808,8 +807,7 @@ void BytecodeArray::Disassemble(std::ostream& os) {
   os << "Frame size " << frame_size() << "\n";
 
   Address base_address = GetFirstBytecodeAddress();
-  SourcePositionTableIterator source_positions(
-      SourcePositionTableIfCollected());
+  SourcePositionTableIterator source_positions(SourcePositionTable());
 
   // Storage for backing the handle passed to the iterator. This handle won't be
   // updated by the gc, but that's ok because we've disallowed GCs anyway.
@@ -868,11 +866,12 @@ void BytecodeArray::Disassemble(std::ostream& os) {
   }
 #endif
 
-  os << "Source Position Table (size = "
-     << SourcePositionTableIfCollected().length() << ")\n";
+  ByteArray source_position_table = SourcePositionTable();
+  os << "Source Position Table (size = " << source_position_table.length()
+     << ")\n";
 #ifdef OBJECT_PRINT
-  if (SourcePositionTableIfCollected().length() > 0) {
-    os << Brief(SourcePositionTableIfCollected()) << std::endl;
+  if (source_position_table.length() > 0) {
+    os << Brief(source_position_table) << std::endl;
   }
 #endif
 }

src/objects/code.h

@@ -109,8 +109,10 @@ class Code : public HeapObject {
 
   // [source_position_table]: ByteArray for the source positions table.
   DECL_ACCESSORS(source_position_table, Object)
+
+  // 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 SourcePositionTableIfCollected() const;
 
   // [code_data_container]: A container indirection for all mutable fields.
   DECL_ACCESSORS(code_data_container, CodeDataContainer)
@@ -773,18 +775,15 @@ class BytecodeArray : public FixedArrayBase {
   // * ByteArray (when source positions have been collected for the bytecode)
   // * exception (when an error occurred while explicitly collecting source
   // positions for pre-existing bytecode).
-  DECL_ACCESSORS(source_position_table, Object)
+  DECL_SYNCHRONIZED_ACCESSORS(source_position_table, Object)
 
-  // This must only be called if source position collection has already been
-  // attempted. (If it failed because of an exception then it will return
-  // empty_byte_array).
-  inline ByteArray SourcePositionTable() const;
-  // If source positions have not been collected or an exception has been thrown
-  // this will return empty_byte_array.
-  inline ByteArray SourcePositionTableIfCollected() const;
   inline bool HasSourcePositionTable() const;
   inline bool DidSourcePositionGenerationFail() const;
 
+  // If source positions have not been collected or an exception has been thrown
+  // this will return empty_byte_array.
+  inline ByteArray SourcePositionTable() const;
+
   // Indicates that an attempt was made to collect source positions, but that it
   // failed most likely due to stack exhaustion. When in this state
   // |SourcePositionTable| will return an empty byte array rather than crashing

src/objects/object-macros.h

@@ -121,6 +121,12 @@
   V8_EXPORT_PRIVATE void set_##name(         \
       type value, WriteBarrierMode mode = UPDATE_WRITE_BARRIER);
 
+// TODO(solanes, neis): Unify naming for synchronized accessor uses.
+#define DECL_SYNCHRONIZED_ACCESSORS(name, type) \
+  DECL_GETTER(synchronized_##name, type)        \
+  inline void set_synchronized_##name(          \
+      type value, WriteBarrierMode mode = UPDATE_WRITE_BARRIER);
+
 #define DECL_CAST_NONINLINE(Type)                    \
   V8_EXPORT_PRIVATE static Type cast(Object object); \
   inline static Type unchecked_cast(Object object) { \

test/unittests/interpreter/bytecode-array-writer-unittest.cc

@@ -142,7 +142,7 @@ TEST_F(BytecodeArrayWriterUnittest, SimpleExample) {
 
   Handle<BytecodeArray> bytecode_array =
       writer()->ToBytecodeArray(isolate(), 0, 0, factory()->empty_byte_array());
-  bytecode_array->set_source_position_table(
+  bytecode_array->set_synchronized_source_position_table(
       *writer()->ToSourcePositionTable(isolate()));
   CHECK_EQ(bytecodes()->size(), arraysize(expected_bytes));
 
@@ -229,7 +229,7 @@ TEST_F(BytecodeArrayWriterUnittest, ComplexExample) {
 
   Handle<BytecodeArray> bytecode_array =
       writer()->ToBytecodeArray(isolate(), 0, 0, factory()->empty_byte_array());
-  bytecode_array->set_source_position_table(
+  bytecode_array->set_synchronized_source_position_table(
       *writer()->ToSourcePositionTable(isolate()));
   SourcePositionTableIterator source_iterator(
       bytecode_array->SourcePositionTable());
@@ -278,7 +278,7 @@ TEST_F(BytecodeArrayWriterUnittest, ElideNoneffectfulBytecodes) {
 
   Handle<BytecodeArray> bytecode_array =
       writer()->ToBytecodeArray(isolate(), 0, 0, factory()->empty_byte_array());
-  bytecode_array->set_source_position_table(
+  bytecode_array->set_synchronized_source_position_table(
       *writer()->ToSourcePositionTable(isolate()));
   SourcePositionTableIterator source_iterator(
       bytecode_array->SourcePositionTable());
@@ -346,7 +346,7 @@ TEST_F(BytecodeArrayWriterUnittest, DeadcodeElimination) {
 
   Handle<BytecodeArray> bytecode_array =
       writer()->ToBytecodeArray(isolate(), 0, 0, factory()->empty_byte_array());
-  bytecode_array->set_source_position_table(
+  bytecode_array->set_synchronized_source_position_table(
       *writer()->ToSourcePositionTable(isolate()));
   SourcePositionTableIterator source_iterator(
       bytecode_array->SourcePositionTable());