From 38c084ee386e257389d44c6200a403f74432e1af Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Mon, 22 Aug 2022 13:53:47 -0700
Subject: Avoid adding new unneeded names to blocks in text roundtripping 
 (#4943)

Previously the wat parser would turn this input:

(block
  (nop)
)

into something like this:

(block $block17
  (nop)
)

It just added a name all the time, in case the block is referred to by an index
later even though it doesn't have a name.

This PR makes us rountrip more precisely by not adding such names: if there
was no name before, and there is no break by index, then do not add a name.

In addition, this will be useful for non-nullable locals since whether a block has
a name or not matters there. Like #4912, this makes us more regular in our
usage of block names.
---
 src/wasm/wasm-s-parser.cpp | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

(limited to 'src/wasm/wasm-s-parser.cpp')

diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index 86d7b27e9..362ba25f9 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -1503,23 +1503,33 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
   // incredibly deep
   auto curr = allocator.alloc<Block>();
   auto* sp = &s;
-  std::vector<std::pair<Element*, Block*>> stack;
+  // The information we need for the stack of blocks here is the element we are
+  // converting, the block we are converting it to, and whether it originally
+  // had a name or not (which will be useful later).
+  struct Info {
+    Element* element;
+    Block* block;
+    bool hadName;
+  };
+  std::vector<Info> stack;
   while (1) {
-    stack.emplace_back(sp, curr);
     auto& s = *sp;
     Index i = 1;
     Name sName;
+    bool hadName = false;
     if (i < s.size() && s[i]->isStr()) {
       // could be a name or a type
       if (s[i]->dollared() ||
           stringToType(s[i]->str(), true /* allowError */) == Type::none) {
         sName = s[i++]->str();
+        hadName = true;
       } else {
         sName = "block";
       }
     } else {
       sName = "block";
     }
+    stack.emplace_back(Info{sp, curr, hadName});
     curr->name = nameMapper.pushLabelName(sName);
     // block signature
     curr->type = parseOptionalResultType(s, i);
@@ -1540,8 +1550,9 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
   }
   // we now have a stack of Blocks, with their labels, but no contents yet
   for (int t = int(stack.size()) - 1; t >= 0; t--) {
-    auto* sp = stack[t].first;
-    auto* curr = stack[t].second;
+    auto* sp = stack[t].element;
+    auto* curr = stack[t].block;
+    auto hadName = stack[t].hadName;
     auto& s = *sp;
     size_t i = 1;
     if (i < s.size()) {
@@ -1553,7 +1564,7 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
       }
       if (t < int(stack.size()) - 1) {
         // first child is one of our recursions
-        curr->list.push_back(stack[t + 1].second);
+        curr->list.push_back(stack[t + 1].block);
         i++;
       }
       for (; i < s.size(); i++) {
@@ -1562,8 +1573,17 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
     }
     nameMapper.popLabelName(curr->name);
     curr->finalize(curr->type);
+    // If the block never had a name, and one was not needed in practice (even
+    // if one did not exist, perhaps a break targeted it by index), then we can
+    // remove the name. Note that we only do this if it never had a name: if it
+    // did, we don't want to change anything; we just want to be the same as
+    // the code we are loading - if there was no name before, we don't want one
+    // now, so that we roundtrip text precisely.
+    if (!hadName && !BranchUtils::BranchSeeker::has(curr, curr->name)) {
+      curr->name = Name();
+    }
   }
-  return stack[0].second;
+  return stack[0].block;
 }
 
 // Similar to block, but the label is handled by the enclosing if (since there
-- 
cgit v1.2.3