[wasm-gc][bug] Fix interaction between 'let' and Goto

Invoking Goto in graph-builder-interface from inside a 'let' can cause
the number of locals between source and target ssa environment to be
different. This CL addresses this bug and adds a few unit tests.
Unfortunately, after this change we have to resort to always using
copy-constructors for SsaEnv, which might cause slowdown in decoding.

Bug: v8:9495
Change-Id: Idf5ace6c7563eff9d774d402f3a81e77959556ef
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2614062Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71993}

src/wasm/graph-builder-interface.cc

@@ -39,9 +39,10 @@ struct SsaEnv : public ZoneObject {
 
   SsaEnv(Zone* zone, State state, TFNode* control, TFNode* effect,
          uint32_t locals_size)
-      : state(state), control(control), effect(effect), locals(zone) {
-    if (locals_size > 0) locals.resize(locals_size);
-  }
+      : state(state),
+        control(control),
+        effect(effect),
+        locals(locals_size, zone) {}
 
   SsaEnv(const SsaEnv& other) V8_NOEXCEPT = default;
   SsaEnv(SsaEnv&& other) V8_NOEXCEPT : state(other.state),
@@ -54,7 +55,9 @@ struct SsaEnv : public ZoneObject {
 
   void Kill(State new_state = kControlEnd) {
     state = new_state;
-    locals.clear();
+    for (TFNode*& local : locals) {
+      local = nullptr;
+    }
     control = nullptr;
     effect = nullptr;
     instance_cache = {};
@@ -1054,7 +1057,14 @@ class WasmGraphBuildingInterface {
     switch (to->state) {
       case SsaEnv::kUnreachable: {  // Overwrite destination.
         to->state = SsaEnv::kReached;
+        // There might be an offset in the locals due to a 'let'.
+        DCHECK_EQ(ssa_env_->locals.size(), decoder->num_locals());
+        DCHECK_GE(ssa_env_->locals.size(), to->locals.size());
+        uint32_t local_count_diff =
+            static_cast<uint32_t>(ssa_env_->locals.size() - to->locals.size());
         to->locals = ssa_env_->locals;
+        to->locals.erase(to->locals.begin(),
+                         to->locals.begin() + local_count_diff);
         to->control = control();
         to->effect = effect();
         to->instance_cache = ssa_env_->instance_cache;
@@ -1072,13 +1082,19 @@ class WasmGraphBuildingInterface {
           TFNode* inputs[] = {to->effect, old_effect, merge};
           to->effect = builder_->EffectPhi(2, inputs);
         }
-        // Merge SSA values.
-        for (int i = decoder->num_locals() - 1; i >= 0; i--) {
+        // Merge locals.
+        // There might be an offset in the locals due to a 'let'.
+        DCHECK_EQ(ssa_env_->locals.size(), decoder->num_locals());
+        DCHECK_GE(ssa_env_->locals.size(), to->locals.size());
+        uint32_t local_count_diff =
+            static_cast<uint32_t>(ssa_env_->locals.size() - to->locals.size());
+        for (uint32_t i = 0; i < to->locals.size(); i++) {
           TFNode* a = to->locals[i];
-          TFNode* b = ssa_env_->locals[i];
+          TFNode* b = ssa_env_->locals[i + local_count_diff];
           if (a != b) {
             TFNode* inputs[] = {a, b, merge};
-            to->locals[i] = builder_->Phi(decoder->local_type(i), 2, inputs);
+            to->locals[i] = builder_->Phi(
+                decoder->local_type(i + local_count_diff), 2, inputs);
           }
         }
         // Start a new merge from the instance cache.
@@ -1094,10 +1110,16 @@ class WasmGraphBuildingInterface {
         to->effect =
             builder_->CreateOrMergeIntoEffectPhi(merge, to->effect, effect());
         // Merge locals.
-        for (int i = decoder->num_locals() - 1; i >= 0; i--) {
+        // There might be an offset in the locals due to a 'let'.
+        DCHECK_EQ(ssa_env_->locals.size(), decoder->num_locals());
+        DCHECK_GE(ssa_env_->locals.size(), to->locals.size());
+        uint32_t local_count_diff =
+            static_cast<uint32_t>(ssa_env_->locals.size() - to->locals.size());
+        for (uint32_t i = 0; i < to->locals.size(); i++) {
           to->locals[i] = builder_->CreateOrMergeIntoPhi(
-              decoder->local_type(i).machine_representation(), merge,
-              to->locals[i], ssa_env_->locals[i]);
+              decoder->local_type(i + local_count_diff)
+                  .machine_representation(),
+              merge, to->locals[i], ssa_env_->locals[i + local_count_diff]);
         }
         // Merge the instance caches.
         builder_->MergeInstanceCacheInto(&to->instance_cache,
@@ -1160,6 +1182,8 @@ class WasmGraphBuildingInterface {
       ssa_env_->effect = effect();
     }
     SsaEnv* result = zone->New<SsaEnv>(std::move(*from));
+    // Restore the length of {from->locals} after applying move-constructor.
+    from->locals = ZoneVector<TFNode*>(result->locals.size(), zone);
     result->state = SsaEnv::kReached;
     return result;
   }

test/cctest/wasm/test-gc.cc

@@ -591,12 +591,32 @@ TEST(WasmLetInstruction) {
   // The result should be 0 and not 1, as local_get(0) refers to the original
   // local.
 
+  const byte kLetInLoop = tester.DefineFunction(
+      tester.sigs.i_i(), {},
+      {WASM_LOOP(WASM_LET_1_V(
+           kI32Code, WASM_I32V(10),  // --
+           WASM_LOCAL_SET(1, WASM_I32_SUB(WASM_LOCAL_GET(1), WASM_I32V(10))),
+           WASM_BR_IF(1, WASM_I32_GES(WASM_LOCAL_GET(1), WASM_LOCAL_GET(0))))),
+       WASM_LOCAL_GET(0), WASM_END});
+
+  const byte kLetInBlock = tester.DefineFunction(
+      tester.sigs.i_i(), {},
+      {WASM_BLOCK(WASM_LET_1_V(
+           kI32Code, WASM_I32V(10),  // --
+           WASM_BR_IF(1, WASM_I32_GES(WASM_LOCAL_GET(1), WASM_LOCAL_GET(0))),
+           WASM_LOCAL_SET(1, WASM_I32V(30)))),
+       WASM_LOCAL_GET(0), WASM_END});
+
   tester.CompileModule();
 
   tester.CheckResult(kLetTest1, 42);
   tester.CheckResult(kLetTest2, 420);
   tester.CheckResult(kLetTestLocals, 891, 1000);
   tester.CheckResult(kLetTestErase, 0);
+  tester.CheckResult(kLetInLoop, 2, 52);
+  tester.CheckResult(kLetInLoop, -11, -1);
+  tester.CheckResult(kLetInBlock, 15, 15);
+  tester.CheckResult(kLetInBlock, 30, 5);
 }
 
 WASM_COMPILED_EXEC_TEST(WasmBasicArray) {