[NFC] Eagerly set local names in binary reader (#7076)

Instead of setting the local names at the end of binary reading, eagerly
set them before parsing function bodies. This is NFC now, but will fix a
future bug once the binary reader uses IRBuilder. IRBuilder can
introduce new scratch locals, and it gives them the names `$scratch`,
`$scratch_1`, etc. If the name section includes locals with the same
names and we set those local names after parsing function bodies, then
we can end up with multiple locals with the same names. Setting the
names before parsing the function bodies ensures that IRBuilder will
generate different names for the scratch locals.

The alternative fix would be to generate fresh names when setting names
from the name section, but it is better to respect the names in the name
section and use fresh names for the newly introduced scratch locals
instead.

2 files changed, 26 insertions, 19 deletions

diff --git a/src/wasm-binary.h b/src/wasm-binary.h
index 67c1dec96..59114cdb8 100644
--- a/src/wasm-binary.h
+++ b/src/wasm-binary.h
@@ -1571,6 +1571,7 @@ public:
 
   void readFunctions();
   void readVars();
+  void setLocalNames(Function& func, Index i);
 
   void readExports();
 
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index 57a01bb1c..f5bf11206 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -1876,25 +1876,6 @@ void WasmBinaryReader::read() {
     }
   }
 
-  // Set local names for imported and declared functions.
-  for (auto& [index, locals] : localNames) {
-    if (index >= wasm.functions.size()) {
-      std::cerr << "warning: function index out of bounds in name section: "
-                   "locals at index "
-                << index << '\n';
-      continue;
-    }
-    for (auto& [local, name] : locals) {
-      if (local >= wasm.functions[index]->getNumLocals()) {
-        std::cerr << "warning: local index out of bounds in name section: "
-                  << name << " at index " << local << " in function " << index
-                  << '\n';
-        continue;
-      }
-      wasm.functions[index]->setLocalName(local, name);
-    }
-  }
-
   validateBinary();
 }
 
@@ -2626,6 +2607,7 @@ void WasmBinaryReader::readImports() {
         curr->hasExplicitName = isExplicit;
         curr->module = module;
         curr->base = base;
+        setLocalNames(*curr, wasm.functions.size());
         wasm.addFunction(std::move(curr));
         break;
       }
@@ -2728,6 +2710,20 @@ void WasmBinaryReader::requireFunctionContext(const char* error) {
   }
 }
 
+void WasmBinaryReader::setLocalNames(Function& func, Index i) {
+  if (auto it = localNames.find(i); it != localNames.end()) {
+    for (auto& [local, name] : it->second) {
+      if (local >= func.getNumLocals()) {
+        std::cerr << "warning: local index out of bounds in name section: "
+                  << name << " at index " << local << " in function " << i
+                  << '\n';
+        continue;
+      }
+      func.setLocalName(local, name);
+    }
+  }
+}
+
 void WasmBinaryReader::readFunctionSignatures() {
   size_t num = getU32LEB();
   auto numImports = wasm.functions.size();
@@ -2739,6 +2735,15 @@ void WasmBinaryReader::readFunctionSignatures() {
     }
     usedNames.insert(name);
   }
+  // Also check that the function indices in the local names subsection are
+  // in-bounds, even though we don't use them here.
+  for (auto& [index, locals] : localNames) {
+    if (index >= num + numImports) {
+      std::cerr << "warning: function index out of bounds in name section: "
+                   "locals at index "
+                << index << '\n';
+    }
+  }
   for (size_t i = 0; i < num; i++) {
     auto [name, isExplicit] =
       getOrMakeName(functionNames, numImports + i, makeName("", i), usedNames);
@@ -2810,6 +2815,7 @@ void WasmBinaryReader::readFunctions() {
     readNextDebugLocation();
 
     readVars();
+    setLocalNames(*func, numFuncImports + i);
 
     func->prologLocation = debugLocation;
     {