Make imported functions inexact

CHANGELOG.md

@@ -15,6 +15,10 @@ full changeset diff at the end of each section.
 Current Trunk
 -------------
 
+ - C and JS APIs now assume RefFuncs are created after imported functions (non-
+   imported functions can still be created later). This is necessary because
+   imported function types can vary (due to Custom Descriptors), and we need to
+   look up that type at RefFunc creation time.
  - The --mod-asyncify-never-unwind and --mod-asyncify-always-and-only-unwind
    passed were deleted.  They only existed to support the lazy code loading
    support in emscripten that was removed. (#7893)

scripts/fuzz_opt.py

@@ -2482,7 +2482,7 @@ def get_random_opts():
 # disabled, its dependent features need to be disabled as well.
 IMPLIED_FEATURE_OPTS = {
     '--disable-reference-types': ['--disable-gc', '--disable-exception-handling', '--disable-strings'],
-    '--disable-gc': ['--disable-strings', '--disable-stack-switching'],
+    '--disable-gc': ['--disable-strings', '--disable-stack-switching', '--disable-custom-descriptors'],
 }
 
 print('''

src/abi/js.h

@@ -58,7 +58,8 @@ inline void ensureHelpers(Module* wasm, IString specific = IString()) {
     if (specific.is() && name != specific) {
       return;
     }
-    auto func = Builder::makeFunction(name, Signature(params, results), {});
+    auto func = Builder::makeFunction(
+      name, Type(Signature(params, results), NonNullable, Inexact), {});
     func->module = ENV;
     func->base = name;
     wasm->addFunction(std::move(func));

src/binaryen-c.cpp

@@ -157,7 +157,7 @@ Literal fromBinaryenLiteral(BinaryenLiteral x) {
     }
   }
   if (heapType.isSignature()) {
-    return Literal::makeFunc(Name(x.func), heapType);
+    return Literal::makeFunc(Name(x.func), type);
   }
   assert(heapType.isData());
   WASM_UNREACHABLE("TODO: gc data");
@@ -1609,8 +1609,21 @@ BinaryenExpressionRef BinaryenRefAs(BinaryenModuleRef module,
 BinaryenExpressionRef BinaryenRefFunc(BinaryenModuleRef module,
                                       const char* func,
                                       BinaryenHeapType type) {
-  return static_cast<Expression*>(
-    Builder(*(Module*)module).makeRefFunc(func, HeapType(type)));
+  // We can assume imports have been created at this point in time, but not
+  // other defined functions. See if the function exists already, and assume it
+  // is non-imported if not. TODO: If we want to allow creating imports later,
+  // we would need an API addition or change.
+  auto* wasm = (Module*)module;
+  if (wasm->getFunctionOrNull(func)) {
+    // Use the HeapType constructor, which will do a lookup on the module.
+    return static_cast<Expression*>(
+      Builder(*(Module*)module).makeRefFunc(func, HeapType(type)));
+  } else {
+    // Assume non-imported, and provide the full type for that.
+    Type full = Type(HeapType(type), NonNullable, Exact);
+    return static_cast<Expression*>(
+      Builder(*(Module*)module).makeRefFunc(func, full));
+  }
 }
 
 BinaryenExpressionRef BinaryenRefEq(BinaryenModuleRef module,
@@ -5096,9 +5109,9 @@ void BinaryenAddFunctionImport(BinaryenModuleRef module,
     func->name = internalName;
     func->module = externalModuleName;
     func->base = externalBaseName;
-    // TODO: Take a HeapType rather than params and results.
+    // TODO: Take a Type rather than params and results.
     func->type =
-      Type(Signature(Type(params), Type(results)), NonNullable, Exact);
+      Type(Signature(Type(params), Type(results)), NonNullable, Inexact);
     ((Module*)module)->addFunction(std::move(func));
   } else {
     // already exists so just set module and base

src/ir/ReFinalize.cpp

@@ -116,9 +116,7 @@ void ReFinalize::visitMemoryGrow(MemoryGrow* curr) { curr->finalize(); }
 void ReFinalize::visitRefNull(RefNull* curr) { curr->finalize(); }
 void ReFinalize::visitRefIsNull(RefIsNull* curr) { curr->finalize(); }
 void ReFinalize::visitRefFunc(RefFunc* curr) {
-  // TODO: should we look up the function and update the type from there? This
-  // could handle a change to the function's type, but is also not really what
-  // this class has been meant to do.
+  curr->finalize(curr->type.getHeapType(), *getModule());
 }
 void ReFinalize::visitRefEq(RefEq* curr) { curr->finalize(); }
 void ReFinalize::visitTableGet(TableGet* curr) { curr->finalize(); }

src/ir/module-splitting.cpp

@@ -73,6 +73,7 @@
 #include "ir/export-utils.h"
 #include "ir/module-utils.h"
 #include "ir/names.h"
+#include "ir/utils.h"
 #include "pass.h"
 #include "support/insert_ordered.h"
 #include "wasm-builder.h"
@@ -274,7 +275,8 @@ TableSlotManager::Slot TableSlotManager::getSlot(Name func, HeapType type) {
                   activeBase.index + Index(activeSegment->data.size())};
 
   Builder builder(module);
-  activeSegment->data.push_back(builder.makeRefFunc(func, type));
+  auto funcType = Type(type, NonNullable, Inexact);
+  activeSegment->data.push_back(builder.makeRefFunc(func, funcType));
 
   addSlot(func, newSlot);
   if (activeTable->initial <= newSlot.index) {
@@ -339,6 +341,7 @@ struct ModuleSplitter {
   void setupTablePatching();
   void shareImportableItems();
   void removeUnusedSecondaryElements();
+  void updateIR();
 
   ModuleSplitter(Module& primary, const Config& config)
     : config(config), primary(primary), tableManager(primary),
@@ -355,6 +358,7 @@ struct ModuleSplitter {
     setupTablePatching();
     shareImportableItems();
     removeUnusedSecondaryElements();
+    updateIR();
   }
 };
 
@@ -372,7 +376,7 @@ void ModuleSplitter::setupJSPI() {
     // Add an imported function to load the secondary module.
     auto import = Builder::makeFunction(
       ModuleSplitting::LOAD_SECONDARY_MODULE,
-      Type(Signature(Type::none, Type::none), NonNullable, Exact),
+      Type(Signature(Type::none, Type::none), NonNullable, Inexact),
       {});
     import->module = ENV;
     import->base = ModuleSplitting::LOAD_SECONDARY_MODULE;
@@ -516,6 +520,7 @@ void ModuleSplitter::exportImportFunction(Name funcName,
       func->hasExplicitName = primaryFunc->hasExplicitName;
       func->module = config.importNamespace;
       func->base = exportName;
+      func->type = func->type.with(Inexact);
       secondary->addFunction(std::move(func));
     }
   }
@@ -790,9 +795,8 @@ void ModuleSplitter::setupTablePatching() {
       placeholder->name = Names::getValidFunctionName(
         primary, std::string("placeholder_") + placeholder->base.toString());
       placeholder->hasExplicitName = true;
-      placeholder->type = secondaryFunc->type;
-      elem = Builder(primary).makeRefFunc(placeholder->name,
-                                          placeholder->type.getHeapType());
+      placeholder->type = secondaryFunc->type.with(Inexact);
+      elem = Builder(primary).makeRefFunc(placeholder->name, placeholder->type);
       primary.addFunction(std::move(placeholder));
     });
 
@@ -833,8 +837,7 @@ void ModuleSplitter::setupTablePatching() {
           // primarySeg->data[i] is a placeholder, so use the secondary
           // function.
           auto* func = replacement->second;
-          auto* ref = Builder(secondary).makeRefFunc(func->name,
-                                                     func->type.getHeapType());
+          auto* ref = Builder(secondary).makeRefFunc(func->name, func->type);
           secondaryElems.push_back(ref);
           ++replacement;
         } else if (auto* get = primarySeg->data[i]->dynCast<RefFunc>()) {
@@ -876,7 +879,7 @@ void ModuleSplitter::setupTablePatching() {
       }
       auto* func = curr->second;
       currData.push_back(
-        Builder(secondary).makeRefFunc(func->name, func->type.getHeapType()));
+        Builder(secondary).makeRefFunc(func->name, func->type));
     }
     if (currData.size()) {
       finishSegment();
@@ -971,11 +974,38 @@ void ModuleSplitter::removeUnusedSecondaryElements() {
   // code size in the primary module as well.
   for (auto& secondaryPtr : secondaries) {
     PassRunner runner(secondaryPtr.get());
+    // Do not validate here in the middle, as the IR still needs updating later.
+    runner.options.validate = false;
     runner.add("remove-unused-module-elements");
     runner.run();
   }
 }
 
+void ModuleSplitter::updateIR() {
+  // Imported functions may need type updates.
+  struct Fixer : public PostWalker<Fixer> {
+    void visitRefFunc(RefFunc* curr) {
+      auto& wasm = *getModule();
+      auto* func = wasm.getFunction(curr->func);
+      if (func->type != curr->type) {
+        // This became an import, and lost exactness.
+        assert(!func->type.isExact());
+        assert(curr->type.isExact());
+        if (wasm.features.hasCustomDescriptors()) {
+          // Add a cast, as the parent may depend on the exactness to validate.
+          // TODO: The cast may be needed even without CD enabled
+          replaceCurrent(Builder(wasm).makeRefCast(curr, curr->type));
+        }
+        curr->type = curr->type.with(Inexact);
+      }
+    }
+  } fixer;
+  fixer.walkModule(&primary);
+  for (auto& secondaryPtr : secondaries) {
+    fixer.walkModule(secondaryPtr.get());
+  }
+}
+
 } // anonymous namespace
 
 Results splitFunctions(Module& primary, const Config& config) {

src/ir/possible-contents.cpp

@@ -27,6 +27,7 @@
 #include "ir/module-utils.h"
 #include "ir/possible-contents.h"
 #include "support/insert_ordered.h"
+#include "wasm-type.h"
 #include "wasm.h"
 
 namespace std {
@@ -643,9 +644,9 @@ struct InfoCollector
   void visitRefFunc(RefFunc* curr) {
     if (!getModule()->getFunction(curr->func)->imported()) {
       // This is not imported, so we know the exact function literal.
-      addRoot(curr,
-              PossibleContents::literal(
-                Literal::makeFunc(curr->func, curr->type.getHeapType())));
+      addRoot(
+        curr,
+        PossibleContents::literal(Literal::makeFunc(curr->func, *getModule())));
     } else {
       // This is imported, so it is effectively a global.
       addRoot(curr,
@@ -1869,8 +1870,7 @@ void TNHOracle::infer() {
         //       lot of other optimizations become possible anyhow.
         auto target = possibleTargets[0]->name;
         info.inferences[call->target] =
-          PossibleContents::literal(Literal::makeFunc(
-            target, wasm.getFunction(target)->type.getHeapType()));
+          PossibleContents::literal(Literal::makeFunc(target, wasm));
         continue;
       }
 
@@ -3142,8 +3142,6 @@ void Flower::dump(Location location) {
     std::cout << "  sigparamloc " << '\n';
   } else if (auto* loc = std::get_if<SignatureResultLocation>(&location)) {
     std::cout << "  sigresultloc " << loc->type << " : " << loc->index << '\n';
-  } else if (auto* loc = std::get_if<RootLocation>(&location)) {
-    std::cout << "  rootloc " << loc->type << '\n';
   } else {
     std::cout << "  (other)\n";
   }

src/ir/properties.h

@@ -116,7 +116,7 @@ inline Literal getLiteral(const Expression* curr) {
   } else if (auto* n = curr->dynCast<RefNull>()) {
     return Literal(n->type);
   } else if (auto* r = curr->dynCast<RefFunc>()) {
-    return Literal::makeFunc(r->func, r->type.getHeapType());
+    return Literal::makeFunc(r->func, r->type);
   } else if (auto* i = curr->dynCast<RefI31>()) {
     if (auto* c = i->value->dynCast<Const>()) {
       return Literal::makeI31(c->value.geti32(),

src/literal.h

@@ -30,6 +30,7 @@
 
 namespace wasm {
 
+class Module;
 class Literals;
 struct FuncData;
 struct GCData;
@@ -70,6 +71,9 @@ class Literal {
 
 public:
   // Type of the literal. Immutable because the literal's payload depends on it.
+  // For references to defined heap types, this is almost always an exact type.
+  // The exception is references to imported functions, since the function
+  // provided at instantiation time may have a subtype of the import type.
   const Type type;
 
   Literal() : v128(), type(Type::none) {}
@@ -90,7 +94,7 @@ class Literal {
   explicit Literal(const std::array<Literal, 8>&);
   explicit Literal(const std::array<Literal, 4>&);
   explicit Literal(const std::array<Literal, 2>&);
-  explicit Literal(std::shared_ptr<FuncData> funcData, HeapType type);
+  explicit Literal(std::shared_ptr<FuncData> funcData, Type type);
   explicit Literal(std::shared_ptr<GCData> gcData, HeapType type);
   explicit Literal(std::shared_ptr<ExnData> exnData);
   explicit Literal(std::shared_ptr<ContData> contData);
@@ -252,7 +256,8 @@ class Literal {
   }
   // Simple way to create a function from the name and type, without a full
   // FuncData.
-  static Literal makeFunc(Name func, HeapType type);
+  static Literal makeFunc(Name func, Type type);
+  static Literal makeFunc(Name func, Module& wasm);
   static Literal makeI31(int32_t value, Shareability share) {
     auto lit = Literal(Type(HeapTypes::i31.getBasic(share), NonNullable));
     lit.i32 = value | 0x80000000;

src/parser/contexts.h

@@ -1419,6 +1419,9 @@ struct ParseModuleTypesCtx : TypeParserCtx<ParseModuleTypesCtx>,
       return in.err(pos, "expected signature type");
     }
     f->type = f->type.with(type.type);
+    if (f->imported()) {
+      f->type = f->type.with(Inexact);
+    }
     // If we are provided with too many names (more than the function has), we
     // will error on that later when we check the signature matches the type.
     // For now, avoid asserting in setLocalName.
@@ -1601,7 +1604,7 @@ struct ParseDefsCtx : TypeParserCtx<ParseDefsCtx>, AnnotationParserCtx {
     elems.push_back(expr);
   }
   void appendFuncElem(std::vector<Expression*>& elems, Name func) {
-    auto type = wasm.getFunction(func)->type.getHeapType();
+    auto type = wasm.getFunction(func)->type;
     elems.push_back(builder.makeRefFunc(func, type));
   }
 

src/passes/ExtractFunction.cpp

@@ -22,6 +22,7 @@
 
 #include <cctype>
 
+#include "ir/utils.h"
 #include "pass.h"
 #include "wasm-builder.h"
 #include "wasm.h"
@@ -37,6 +38,7 @@ static void extract(PassRunner* runner, Module* module, Name name) {
       func->module = "env";
       func->base = func->name;
       func->vars.clear();
+      func->type = func->type.with(Inexact);
       func->body = nullptr;
     } else {
       found = true;
@@ -46,6 +48,12 @@ static void extract(PassRunner* runner, Module* module, Name name) {
     Fatal() << "could not find the function to extract\n";
   }
 
+  // Update function references after making things imports.
+  ReFinalize().run(runner, module);
+  ReFinalize().walkModuleCode(module);
+  // TODO: Add casts when needed for exactness, like wasm-split, to handle
+  //       places that need the type remain exact.
+
   // Leave just one export, for the thing we want.
   module->exports.clear();
   module->updateMaps();

src/passes/FuncCastEmulation.cpp

@@ -178,7 +178,7 @@ struct FuncCastEmulation : public Pass {
         }
         auto* thunk = iter->second;
         ref->func = thunk->name;
-        ref->finalize(thunk->type.getHeapType());
+        ref->finalize(thunk->type.getHeapType(), *module);
       }
     }
 

src/passes/GUFA.cpp

@@ -173,16 +173,9 @@ struct GUFAOptimizer
     } else {
       // The type is not compatible: we cannot place |c| in this location, even
       // though we have proven it is the only value possible here.
-      if (Properties::isConstantExpression(c)) {
-        // The type is not compatible and this is a simple constant expression
-        // like a ref.func. That means this code must be unreachable. (See below
-        // for the case of a non-constant.)
-        replaceCurrent(getDroppedChildrenAndAppend(
-          curr, wasm, options, builder.makeUnreachable()));
-        optimized = true;
-      } else {
+      if (c->is<GlobalGet>() || c->is<RefFunc>()) {
         // This is not a constant expression, but we are certain it is the right
-        // value. Atm the only such case we handle is a global.get of an
+        // value. One such case that can happen is a global.get of an
         // immutable global. We don't know what the value will be, nor its
         // specific type, but we do know that a global.get will get that value
         // properly. However, in this case it does not have the right type for
@@ -204,9 +197,24 @@ struct GUFAOptimizer
         // non-nullable type like a ref.as_non_null must have, so we cannot
         // simply replace it.
         //
+        // Similarly, an imported function can cause this: imagine
+        //
+        //  (ref.cast (exact ..)
+        //    (ref.func $imported)
+        //  )
+        //
+        // The output of the cast is exact, but |c| is a RefFunc of inexact
+        // type. (In practice this will either trap at runtime or not.)
+        //
         // For now, do nothing here, but in some cases we could probably
-        // optimize (e.g. by adding a ref.as_non_null in the example) TODO
-        assert(c->is<GlobalGet>());
+        // optimize (e.g. by adding a ref.as_non_null in the first example) TODO
+      } else {
+        // Otherwise, the type is not compatible and this is a simple constant
+        // expression. That means this code must be unreachable.
+        assert(Properties::isConstantExpression(c));
+        replaceCurrent(getDroppedChildrenAndAppend(
+          curr, wasm, options, builder.makeUnreachable()));
+        optimized = true;
       }
     }
   }