Revert "Reland "[builtins] Load external references from the external-reference-table""

This reverts commit f81847388c.

Reason for revert: arm is still unhappy https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20GC%20Stress/builds/6633

Original change's description:
> Reland "[builtins] Load external references from the external-reference-table"
> 
> This is a reland of 9afde91b94
> 
> Original change's description:
> > [builtins] Load external references from the external-reference-table
> >
> > Off-heap code cannot embed external references. With this CL, we load
> > from the external reference table (reached through the root pointer)
> > instead.
> >
> > In a follow-up, the table could be stored within the isolate itself,
> > removing one more level of indirection.
> >
> > Bug: v8:6666
> > Change-Id: I4c612ad3d4112ec03c3b389f5bfb9cdc3dc8a671
> > Reviewed-on: https://chromium-review.googlesource.com/970468
> > Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> > Reviewed-by: Yang Guo <yangguo@chromium.org>
> > Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#52073}
> 
> TBR=mstarzinger@chromium.org
> 
> Bug: v8:6666, v8:7580
> Change-Id: I163cfc15605c1183b79ead77df0e37d71d60b6f7
> Reviewed-on: https://chromium-review.googlesource.com/972821
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#52118}

TBR=yangguo@chromium.org,mstarzinger@chromium.org,jgruber@chromium.org

Change-Id: I5bcd1a1c84c6e9a6a24364390c9359d43c77120d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:6666, v8:7580
Reviewed-on: https://chromium-review.googlesource.com/973782
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52121}

src/builtins/constants-table-builder.cc

@@ -46,6 +46,25 @@ uint32_t BuiltinsConstantsTableBuilder::AddObject(Handle<Object> object) {
   }
 }
 
+uint32_t BuiltinsConstantsTableBuilder::AddExternalReference(
+    ExternalReference reference) {
+  // Not yet finalized.
+  DCHECK_EQ(isolate_->heap()->empty_fixed_array(),
+            isolate_->heap()->builtins_constants_table());
+
+  auto maybe_entry = external_reference_map_.find(reference.address());
+  if (maybe_entry == external_reference_map_.end()) {
+    HandleScope handle_scope(isolate_);
+    Handle<Foreign> object =
+        isolate_->factory()->NewForeign(reference.address(), TENURED);
+    uint32_t index = AddObject(object);
+    external_reference_map_.emplace(reference.address(), index);
+    return index;
+  } else {
+    return maybe_entry->second;
+  }
+}
+
 void BuiltinsConstantsTableBuilder::Finalize() {
   HandleScope handle_scope(isolate_);
 

src/builtins/constants-table-builder.h

@@ -5,6 +5,8 @@
 #ifndef V8_BUILTINS_CONSTANTS_TABLE_BUILDER_H_
 #define V8_BUILTINS_CONSTANTS_TABLE_BUILDER_H_
 
+#include <unordered_map>
+
 #include "src/allocation.h"
 #include "src/base/macros.h"
 #include "src/handles.h"
@@ -28,6 +30,10 @@ class BuiltinsConstantsTableBuilder final {
   // object, possibly adding the object to the table. Objects are deduplicated.
   uint32_t AddObject(Handle<Object> object);
 
+  // External references can also be added, and end up as a Foreign object in
+  // the constants table.
+  uint32_t AddExternalReference(ExternalReference reference);
+
   // Should be called after all affected code (e.g. builtins and bytecode
   // handlers) has been generated.
   void Finalize();
@@ -39,6 +45,10 @@ class BuiltinsConstantsTableBuilder final {
   typedef IdentityMap<uint32_t, FreeStoreAllocationPolicy> ConstantsMap;
   ConstantsMap map_;
 
+  // Maps external references to corresponding indices within the constants
+  // list. Note that external references are also in map_ as Foreign objects.
+  std::unordered_map<Address, uint32_t> external_reference_map_;
+
   DISALLOW_COPY_AND_ASSIGN(BuiltinsConstantsTableBuilder)
 };
 

src/compiler/code-assembler.cc

@@ -21,7 +21,6 @@
 #include "src/machine-type.h"
 #include "src/macro-assembler.h"
 #include "src/objects-inl.h"
-#include "src/snapshot/serializer-common.h"
 #include "src/utils.h"
 #include "src/zone/zone.h"
 
@@ -262,37 +261,37 @@ TNode<HeapObject> CodeAssembler::LookupConstant(Handle<HeapObject> object) {
       Load(MachineType::AnyTagged(), builtins_constants_table, offset));
 }
 
-// External references are stored in the external reference table.
+// External references are stored on the builtins constants list, wrapped in
+// Foreign objects.
+// TODO(jgruber,v8:6666): A more efficient solution.
 TNode<ExternalReference> CodeAssembler::LookupExternalReference(
     ExternalReference reference) {
   DCHECK(isolate()->serializer_enabled());
 
-  // Encode as an index into the external reference table stored on the isolate.
+  // Ensure the given object is in the builtins constants table and fetch its
+  // index.
+  BuiltinsConstantsTableBuilder* builder =
+      isolate()->builtins_constants_table_builder();
+  uint32_t index = builder->AddExternalReference(reference);
 
-  ExternalReferenceEncoder encoder(isolate());
-  ExternalReferenceEncoder::Value v = encoder.Encode(reference.address());
-  CHECK(!v.is_from_api());
-  uint32_t index = v.index();
+  // The builtins constants table is loaded through the root register on all
+  // supported platforms. This is checked by the
+  // VerifyBuiltinsIsolateIndependence cctest, which disallows embedded objects
+  // in isolate-independent builtins.
+  DCHECK(isolate()->heap()->RootCanBeTreatedAsConstant(
+      Heap::kBuiltinsConstantsTableRootIndex));
+  TNode<FixedArray> builtins_constants_table = UncheckedCast<FixedArray>(
+      LoadRoot(Heap::kBuiltinsConstantsTableRootIndex));
 
-  // Generate code to load from the external reference table.
-  // TODO(jgruber,v8:6666): If the external reference points within the isolate,
-  // we could return kRootPointer + offset without loading through the table.
+  // Generate the lookup.
+  const int32_t header_size = FixedArray::kHeaderSize - kHeapObjectTag;
+  TNode<IntPtrT> offset = IntPtrConstant(header_size + kPointerSize * index);
+  TNode<Object> foreign = UncheckedCast<HeapObject>(
+      Load(MachineType::AnyTagged(), builtins_constants_table, offset));
 
-  TNode<IntPtrT> roots_ptr = LoadRootsPointer();
-
-  static const intptr_t roots_to_isolate_offset =
-#ifdef V8_TARGET_ARCH_X64
-      Isolate::offset_from_roots_to_external_reference_table() -
-      kRootRegisterBias;
-#else
-      Isolate::offset_from_roots_to_external_reference_table();
-#endif
-
-  Node* external_refs_ptr = Load(MachineType::Pointer(), roots_ptr,
-                                 IntPtrConstant(roots_to_isolate_offset));
   return UncheckedCast<ExternalReference>(
-      Load(MachineType::Pointer(), external_refs_ptr,
-           IntPtrConstant(ExternalReferenceTable::OffsetOfEntry(index))));
+      Load(MachineType::Pointer(), foreign,
+           IntPtrConstant(Foreign::kForeignAddressOffset - kHeapObjectTag)));
 }
 #endif  // V8_EMBEDDED_BUILTINS
 

src/external-reference-table.h

@@ -51,12 +51,6 @@ class ExternalReferenceTable {
 
   static const char* ResolveSymbol(void* address);
 
-  static uint32_t OffsetOfEntry(uint32_t i) {
-    // Used in CodeAssembler::LookupExternalReference.
-    STATIC_ASSERT(offsetof(ExternalReferenceEntry, address) == 0);
-    return i * sizeof(ExternalReferenceEntry);
-  }
-
  private:
   struct ExternalReferenceEntry {
     Address address;

src/isolate.cc

@@ -2586,6 +2586,8 @@ void Isolate::GlobalTearDown() {
 
 
 void Isolate::ClearSerializerData() {
+  delete external_reference_table_;
+  external_reference_table_ = nullptr;
   delete external_reference_map_;
   external_reference_map_ = nullptr;
 }
@@ -2673,9 +2675,6 @@ void Isolate::Deinit() {
   delete root_index_map_;
   root_index_map_ = nullptr;
 
-  delete external_reference_table_;
-  external_reference_table_ = nullptr;
-
   ClearSerializerData();
 }
 
@@ -2890,14 +2889,6 @@ void Isolate::PrepareEmbeddedBlobForSerialization() {
 
   CreateOffHeapTrampolines(this);
 }
-
-// static
-int32_t Isolate::offset_from_roots_to_external_reference_table() {
-  int32_t isolate_to_roots = OFFSET_OF(Isolate, heap_.roots_);
-  int32_t isolate_to_external_reference_table =
-      OFFSET_OF(Isolate, external_reference_table_);
-  return isolate_to_external_reference_table - isolate_to_roots;
-}
 #endif  // V8_EMBEDDED_BUILTINS
 
 bool Isolate::Init(StartupDeserializer* des) {
@@ -3035,9 +3026,6 @@ bool Isolate::Init(StartupDeserializer* des) {
 
   InitializeThreadLocal();
 
-  // Ensure that the external reference table has been allocated.
-  ExternalReferenceTable::instance(this);
-
   bootstrapper_->Initialize(create_heap_objects);
   setup_delegate_->SetupBuiltins(this);
   if (create_heap_objects) heap_.CreateFixedStubs();

src/isolate.h

@@ -1261,8 +1261,6 @@ class Isolate {
     return builtins_constants_table_builder_;
   }
 
-  static int32_t offset_from_roots_to_external_reference_table();
-
   const uint8_t* embedded_blob() const;
   uint32_t embedded_blob_size() const;
 #endif