Bug 1533932 - Handle prebarriers properly for wasm tables. r=bbouvier

Two problems needed to be fixed:

- JS::Heap<> only has a postbarrier, not a prebarrier also; this is
  what triggered the original error report.  The type in
  table-of-anyref therefore needs to be js::HeapPtr<>, so that we get
  both barriers.  (It can't be GCPtr because the vector storage may be
  moved as the table grows.)

- The API for table.get returns a pointer into underlying vector
  storage or null; if not null, the pointer must be dereferenced to
  obtain the actual value.  Since the vector storage may move during
  GC and grow operations the pointer is really only valid until the
  next operation that might move the vector.  That lifetime
  restriction was not enforced in Ion-generated code and the
  dereference could in principle move far enough during optimization
  for it to become invalid, though in practice it is extremely
  unlikely that this was actually a problem.

  We fix this by flagging the dereference as non-movable, and by
  changing some names and comments to highlight the issue.

Differential Revision: https://phabricator.services.mozilla.com/D23665

--HG--
extra : rebase_source : 67c9d5f4e11b9b2886ce7cc16206a391d97d826e
extra : histedit_source : c9ecc54aab5e3b4c2ab5723b7e0bc1467b6907fe

js/src/jit-test/tests/wasm/regress/table-of-anyref.js

@@ -0,0 +1,11 @@
+// |jit-test| skip-if: !wasmReftypesEnabled()
+
+// Faulty prebarrier for tables.
+gczeal(4, 8);
+let ins = wasmEvalText(
+    `(module
+       (table (export "t") 10 anyref)
+       (func (export "set_anyref") (param i32) (param anyref)
+         (table.set (get_local 0) (get_local 1))))`);
+ins.exports.set_anyref(3, {});
+ins.exports.set_anyref(3, null);

js/src/jit/MIR.h

@@ -11932,11 +11932,14 @@ class MWasmDerivedPointer : public MUnaryInstruction,
 class MWasmLoadRef : public MUnaryInstruction, public NoTypePolicy::Data {
   AliasSet::Flag aliasSet_;
 
-  explicit MWasmLoadRef(MDefinition* valueAddr, AliasSet::Flag aliasSet)
+  explicit MWasmLoadRef(MDefinition* valueAddr, AliasSet::Flag aliasSet,
+                        bool isMovable = true)
       : MUnaryInstruction(classOpcode, valueAddr), aliasSet_(aliasSet) {
     MOZ_ASSERT(valueAddr->type() == MIRType::Pointer);
     setResultType(MIRType::RefOrNull);
-    setMovable();
+    if (isMovable) {
+      setMovable();
+    }
   }
 
  public:

js/src/wasm/WasmInstance.cpp

@@ -857,6 +857,10 @@ Instance::tableInit(Instance* instance, uint32_t dstOffset, uint32_t srcOffset,
 // Ion code has to hold a value that may or may not be a pointer to GC'd
 // storage, or where Ion has to pass in a pointer to storage where a return
 // value can be written.
+//
+// Note carefully that the pointer that is returned may not be valid past
+// operations that change the size of the table or cause GC work; it is strictly
+// to be used to retrieve the return value.
 
 /* static */ void* /* nullptr to signal trap; pointer to table location
                       otherwise */
@@ -868,7 +872,7 @@ Instance::tableGet(Instance* instance, uint32_t index, uint32_t tableIndex) {
                               JSMSG_WASM_TABLE_OUT_OF_BOUNDS);
     return nullptr;
   }
-  return const_cast<void*>(table.getAnyRefLocForCompiledCode(index));
+  return const_cast<void*>(table.getShortlivedAnyRefLocForCompiledCode(index));
 }
 
 /* static */ uint32_t /* infallible */

js/src/wasm/WasmIonCompile.cpp

@@ -729,8 +729,11 @@ class FunctionCompiler {
   }
 
   MDefinition* derefTableElementPointer(MDefinition* base) {
+    // Table element storage may be moved by GC operations, so reads from that
+    // storage are not movable.
     MWasmLoadRef* load =
-        MWasmLoadRef::New(alloc(), base, AliasSet::WasmTableElement);
+        MWasmLoadRef::New(alloc(), base, AliasSet::WasmTableElement,
+                          /*isMovable=*/ false);
     curBlock_->add(load);
     return load;
   }
@@ -3160,8 +3163,8 @@ static bool EmitTableGet(FunctionCompiler& f) {
     return false;
   }
 
-  // The return value here is either null, denoting an error, or a pointer to an
-  // unmovable location containing a possibly-null ref.
+  // The return value here is either null, denoting an error, or a short-lived
+  // pointer to a location containing a possibly-null ref.
   MDefinition* result;
   if (!f.builtinInstanceMethodCall(SASigTableGet, lineOrBytecode, args,
                                    &result)) {

js/src/wasm/WasmTable.cpp

@@ -148,9 +148,9 @@ AnyRef Table::getAnyRef(uint32_t index) const {
   return AnyRef::fromJSObject(objects_[index]);
 }
 
-const void* Table::getAnyRefLocForCompiledCode(uint32_t index) const {
+const void* Table::getShortlivedAnyRefLocForCompiledCode(uint32_t index) const {
   MOZ_ASSERT(!isFunction());
-  return objects_[index].address();
+  return const_cast<HeapPtr<JSObject*>&>(objects_[index]).unsafeUnbarrieredForTracing();
 }
 
 void Table::setAnyFunc(uint32_t index, void* code, const Instance* instance) {

js/src/wasm/WasmTable.h

@@ -38,7 +38,7 @@ namespace wasm {
 // the most appropriate representation for Cell::anyref.
 STATIC_ASSERT_ANYREF_IS_JSOBJECT;
 
-typedef GCVector<JS::Heap<JSObject*>, 0, SystemAllocPolicy> TableAnyRefVector;
+typedef GCVector<HeapPtr<JSObject*>, 0, SystemAllocPolicy> TableAnyRefVector;
 
 class Table : public ShareableBase<Table> {
   using InstanceSet = JS::WeakCache<GCHashSet<
@@ -87,7 +87,7 @@ class Table : public ShareableBase<Table> {
   void setAnyFunc(uint32_t index, void* code, const Instance* instance);
 
   AnyRef getAnyRef(uint32_t index) const;
-  const void* getAnyRefLocForCompiledCode(uint32_t index) const;
+  const void* getShortlivedAnyRefLocForCompiledCode(uint32_t index) const;
   void setAnyRef(uint32_t index, AnyRef);
 
   void setNull(uint32_t index);