[v8][wasm] Handles finally in try/finally blocks.

src/wasm/ast-decoder.cc

Index: src/wasm/ast-decoder.cc
diff --git a/src/wasm/ast-decoder.cc b/src/wasm/ast-decoder.cc
index 0f192508ba7d47211f380a4a48c724693202638c..d4b2c996d3112048b2b8cadb513800bcd2b54648 100644
--- a/src/wasm/ast-decoder.cc
+++ b/src/wasm/ast-decoder.cc
@@ -68,17 +68,43 @@ struct Value {
   LocalType type;
 };
 
+struct Control;
+
+// PathToken is used by exception handling code for managing finally's.

[bradnelson, 2016/08/23 05:49:42]

Don't really care for the PathToken name.
This is really a list of incoming branches to the finally, how about
BranchesToThisFinally .. bah don't like that either...

[John, 2016/08/23 11:38:20]

The three hardest problems in CS: Cache Coherency, Multi-thread safety, Naming,
and off-by-one errors. <badjoke />

I dislike PathToken, too, but I dislike BranchToThisFinally a bit more. Leaving
as is, but glad to change.

[titzer, 2016/08/23 13:56:16]

IncomingBranches? And would that mean that it is Control* from?

[John, 2016/08/23 16:30:59]

IncomingBranch is great! Thanks.

However, it is still Control* target: each of these structs "saves" an incoming
branch's information, including the target that should be branched to once the
finally's run.

+struct PathToken {
+  int32_t token_value;
+  Control* target;
+  Value val;
+};
+
+// Auxiliary data for exception handling. Most scopes don't need any of this so
+// we group everything into a separate struct.
+struct TryInfo {

[titzer, 2016/08/23 13:56:16]

If this were a ZoneObject, you could simply new(zone) it.

It's not great that we override new, but it's better to be consistent if we ever
get a mind to fix all of them later.

[John, 2016/08/23 16:30:59]

Well, it is not great, but consistency trumps great... and overriding new for
arena allocation ain't **that** bad. :)

Done.

+  SsaEnv* catch_env;       // catch environment (only for try with catch).
+  SsaEnv* finish_try_env;  // the environment where a try with finally lives.
+  ZoneVector<PathToken> path_tokens;
+  TFNode* token;
+  bool has_handled_finally;
+
+  TryInfo(Zone* zone, SsaEnv* c, SsaEnv* f)
+      : catch_env(c),
+        finish_try_env(f),
+        path_tokens(zone),
+        token(nullptr),
+        has_handled_finally(false) {}
+};
+
 // An entry on the control stack (i.e. if, block, loop).
 struct Control {
   const byte* pc;
-  int stack_depth;         // stack height at the beginning of the construct.
-  SsaEnv* end_env;         // end environment for the construct.
-  SsaEnv* false_env;       // false environment (only for if).
-  SsaEnv* catch_env;       // catch environment (only for try with catch).
-  SsaEnv* finish_try_env;  // the environment where a try with finally lives.
-  TFNode* node;            // result node for the construct.
-  LocalType type;          // result type for the construct.
-  bool is_loop;            // true if this is the inner label of a loop.
+  int stack_depth;       // stack height at the beginning of the construct.
+  SsaEnv* end_env;       // end environment for the construct.
+  SsaEnv* false_env;     // false environment (only for if).
+  TryInfo* eh;           // exception handling stuff. See TryInfo above.
+  int32_t prev_finally;  // previous control (on stack) that has a finally.

[bradnelson, 2016/08/23 05:49:42]

The bookkeeping on the linked list of previous controls with a finally is rather
tricky.
Any reason this can't be a pointer to the control block, instead of index?

[John, 2016/08/23 11:38:20]

I was initially thinking about that. Then the problem became (in
BuildFinallyChain) figuring out whether a branch target crossed a finally or
not. With the pointers, I figured, I'd have to search the entire stack to find
that out. Then I thought about keeping these indexes. IMO, comparing pointers
for anything other than (in)equality is a strong indication of code smell (of
course, there are legitimate uses of < for pointers)

Now, after your comment, I gave this a second thought, and realized "hey, these
pointers wouldn't be *random* pointers: they are array elements! so comparing
them with < should be fine!" But then it hit me: "shoot! these are array
elements! more importantly, these are elements in an std::vector!"

The problem here would be keeping a pointer to an std::vector element. If the
vector is ever resized (and it may), we're in for a really bad time.

I'd argue that keeping the indices is more correct than keeping the pointers.

[titzer, 2016/08/23 13:56:16]

I agree on the index.

+  TFNode* node;          // result node for the construct.
+  LocalType type;        // result type for the construct.
+  bool is_loop;          // true if this is the inner label of a loop.
 
   bool is_if() const { return *pc == kExprIf; }
 
@@ -96,26 +122,35 @@ struct Control {
   }
 
   // Named constructors.
-  static Control Block(const byte* pc, int stack_depth, SsaEnv* end_env) {
-    return {pc,      stack_depth, end_env, nullptr, nullptr,
-            nullptr, nullptr,     kAstEnd, false};
+  static Control Block(const byte* pc, int stack_depth,
+                       int32_t most_recent_finally, SsaEnv* end_env) {
+    return {pc,      stack_depth, end_env,
+            nullptr, nullptr,     most_recent_finally,
+            nullptr, kAstEnd,     false};
   }
 
-  static Control If(const byte* pc, int stack_depth, SsaEnv* end_env,
+  static Control If(const byte* pc, int stack_depth,
+                    int32_t most_recent_finally, SsaEnv* end_env,
                     SsaEnv* false_env) {
-    return {pc,      stack_depth, end_env,  false_env, nullptr,
-            nullptr, nullptr,     kAstStmt, false};
+    return {pc,        stack_depth, end_env,
+            false_env, nullptr,     most_recent_finally,
+            nullptr,   kAstStmt,    false};
   }
 
-  static Control Loop(const byte* pc, int stack_depth, SsaEnv* end_env) {
-    return {pc,      stack_depth, end_env, nullptr, nullptr,
-            nullptr, nullptr,     kAstEnd, true};
+  static Control Loop(const byte* pc, int stack_depth,
+                      int32_t most_recent_finally, SsaEnv* end_env) {
+    return {pc,      stack_depth, end_env,
+            nullptr, nullptr,     most_recent_finally,
+            nullptr, kAstEnd,     true};
   }
 
-  static Control Try(const byte* pc, int stack_depth, SsaEnv* end_env,
+  static Control Try(const byte* pc, int stack_depth,
+                     int32_t most_recent_finally, Zone* zone, SsaEnv* end_env,
                      SsaEnv* catch_env, SsaEnv* finish_try_env) {
-    return {pc,      stack_depth, end_env, nullptr, catch_env, finish_try_env,
-            nullptr, kAstEnd,     false};
+    return {pc, stack_depth, end_env, nullptr,
+            new (zone->New(sizeof(TryInfo)))
+                TryInfo(zone, catch_env, finish_try_env),
+            most_recent_finally, nullptr, kAstEnd, false};
   }
 };
 
@@ -425,6 +460,10 @@ class WasmDecoder : public Decoder {
 
 // The full WASM decoder for bytecode. Both verifies bytecode and generates

[bradnelson, 2016/08/23 05:49:43]

This comment got separated from the class.

[John, 2016/08/23 11:38:20]

Done. It looks weird, but this is kinda by design: the constants are really only
used in that class. I thought that maybe keeping them between the comment and
the class would help convey that message, but I don't really love the solution.

 // a TurboFan IR graph.
+static const int32_t kFirstFinallyToken = 1000;
+static const int32_t kFallthroughToken = 100;

[bradnelson, 2016/08/23 05:49:42]

These choices of values are a little confusing, but not sure what to suggest.
I assume you picked a big gap just to make things obvious when debugging?

[John, 2016/08/23 11:38:20]

Exactly. I was going to pick 1024 (a nice round number) and 121 (a nice
palindrome, and also the 121 == 11**2) because their values don't really matter.
I can change them if you want, but I am leaving them as is for now.

[titzer, 2016/08/23 13:56:16]

Are there any problems with collisions with these values, e.g. if we have more
than 1024 nested finally's?

IIUC then we could start fallthrough with 0 and first with 1; small numbers are
good since they will likely end up being immediates in moves in the code.

[John, 2016/08/23 16:30:59]

Done.

+static const int32_t kNullFinallyToken = -1;
+
 class WasmFullDecoder : public WasmDecoder {
  public:
   WasmFullDecoder(Zone* zone, TFBuilder* builder, const FunctionBody& body)
@@ -434,7 +473,9 @@ class WasmFullDecoder : public WasmDecoder {
         base_(body.base),
         local_type_vec_(zone),
         stack_(zone),
-        control_(zone) {
+        control_(zone),
+        most_recent_finally_(-1),
+        finally_token_val_(kFirstFinallyToken) {
     local_types_ = &local_type_vec_;
   }
 
@@ -527,6 +568,16 @@ class WasmFullDecoder : public WasmDecoder {
   ZoneVector<Value> stack_;               // stack of values.
   ZoneVector<Control> control_;           // stack of blocks, loops, and ifs.
 
+  int32_t most_recent_finally_;
+  int32_t finally_token_val_;
+
+  int32_t FallthroughTokenForFinally() {
+    // Any number < kFirstFinallyToken would work.
+    return kFallthroughToken;
+  }
+
+  int32_t NewPathTokenForFinally() { return finally_token_val_++; }
+
   inline bool build() { return builder_ && ssa_env_->go(); }
 
   void InitSsaEnv() {
@@ -730,15 +781,15 @@ class WasmFullDecoder : public WasmDecoder {
               break;
             }
 
-            if (c->catch_env == nullptr) {
+            if (c->eh->catch_env == nullptr) {
               error(pc_, "catch already present for try with catch");
               break;
             }
 
             Goto(ssa_env_, c->end_env);
 
-            SsaEnv* catch_env = c->catch_env;
-            c->catch_env = nullptr;
+            SsaEnv* catch_env = c->eh->catch_env;
+            c->eh->catch_env = nullptr;
             SetEnv("catch:begin", catch_env);
 
             if (Validate(pc_, operand)) {
@@ -761,7 +812,7 @@ class WasmFullDecoder : public WasmDecoder {
             }
 
             Control* c = &control_.back();
-            if (c->has_catch() && c->catch_env != nullptr) {
+            if (c->has_catch() && c->eh->catch_env != nullptr) {
               error(pc_, "missing catch for try with catch and finally");
               break;
             }
@@ -771,7 +822,7 @@ class WasmFullDecoder : public WasmDecoder {
               break;
             }
 
-            if (c->finish_try_env == nullptr) {
+            if (c->eh->finish_try_env == nullptr) {
               error(pc_, "finally already present for try with finally");
               break;
             }
@@ -779,10 +830,14 @@ class WasmFullDecoder : public WasmDecoder {
             // ssa_env_ is either the env for either the try or the catch, but
             // it does not matter: either way we need to direct the control flow
             // to the end_env, which is the env for the finally.
-            // c->finish_try_env is the the environment enclosing the try block.
-            Goto(ssa_env_, c->end_env);
-
-            PopUpTo(c->stack_depth);
+            // c->eh->finish_try_env is the the environment enclosing the try
+            // block.
+            const bool has_fallthrough = ssa_env_->go();
+            Value val = PopUpTo(c->stack_depth);
+            if (has_fallthrough) {
+              MergeInto(c->end_env, &c->node, &c->type, val);
+              MergeFinallyToken(ssa_env_, c, FallthroughTokenForFinally());
+            }
 
             // The current environment becomes end_env, and finish_try_env
             // becomes the new end_env. This ensures that any control flow
@@ -791,10 +846,22 @@ class WasmFullDecoder : public WasmDecoder {
             // that kExprEnd below can handle the try block as it would any
             // other block construct.
             SsaEnv* finally_env = c->end_env;
-            c->end_env = c->finish_try_env;
+            c->end_env = c->eh->finish_try_env;
             SetEnv("finally:begin", finally_env);
-            c->finish_try_env = nullptr;
+            c->eh->finish_try_env = nullptr;
+
+            if (has_fallthrough) {
+              LocalType c_type = c->type;
+              if (c->node == nullptr) {
+                c_type = kAstStmt;
+              }
+              Push(c_type, c->node);
+            }
 
+            c->eh->has_handled_finally = true;
+            // There's no more need to keep the current control scope in the
+            // finally chain as no more predecessors will be added to c.
+            most_recent_finally_ = c->prev_finally;
             break;
           }
           case kExprLoop: {
@@ -847,7 +914,7 @@ class WasmFullDecoder : public WasmDecoder {
           }
           case kExprEnd: {
             if (control_.empty()) {
-              error(pc_, "end does not match any if or block");
+              error(pc_, "end does not match any if, try, or block");
               break;
             }
             const char* name = "block:end";
@@ -855,7 +922,7 @@ class WasmFullDecoder : public WasmDecoder {
             Value val = PopUpTo(c->stack_depth);
             if (c->is_loop) {
               // Loops always push control in pairs.
-              control_.pop_back();
+              PopControl();
               c = &control_.back();
               name = "loop:end";
             } else if (c->is_if()) {
@@ -871,19 +938,58 @@ class WasmFullDecoder : public WasmDecoder {
             } else if (c->is_try()) {
               name = "try:end";
 
-              // try blocks do not yield a value.
-              val = {val.pc, nullptr, kAstStmt};
-
               // validate that catch/finally were seen.
-              if (c->catch_env != nullptr) {
+              if (c->eh->catch_env != nullptr) {
                 error(pc_, "missing catch in try with catch");
                 break;
               }
 
-              if (c->finish_try_env != nullptr) {
+              if (c->eh->finish_try_env != nullptr) {
                 error(pc_, "missing finally in try with finally");
                 break;
               }
+
+              if (c->has_finally() && ssa_env_->go()) {
+                // finally -> dispatch on the path token

[bradnelson, 2016/08/23 05:49:43]

Make clear your counting inbound paths other than the current block?

[John, 2016/08/23 11:38:20]

Done.

+                const ZoneVector<PathToken>& path_tokens = c->eh->path_tokens;
+                uint32_t targets = 0;
+                for (const auto& path_token : path_tokens) {
+                  if (path_token.target != c) ++targets;
+                }
+
+                SsaEnv* break_env = ssa_env_;
+                if (targets == 0) {
+                  // Nothing to do
+                } else {
+                  TFNode* sw = BUILD(Switch, static_cast<uint32_t>(targets + 1),

[bradnelson, 2016/08/23 05:49:42]

Worth pulling the resolve of the finally into a separate method?

[John, 2016/08/23 11:38:20]

Done.

+                                     c->eh->token);
+
+                  SsaEnv* copy = Steal(break_env);
+                  for (uint32_t ii = 0; ii < path_tokens.size(); ++ii) {
+                    Control* t = path_tokens[ii].target;
+                    if (t != c) {
+                      const int32_t token_value = path_tokens[ii].token_value;
+                      ssa_env_ = Split(copy);
+                      ssa_env_->control = BUILD(IfValue, token_value, sw);
+                      MergeInto(t->end_env, &t->node, &t->type,
+                                path_tokens[ii].val);
+                      // We only need to merge the finally token if t is both a
+                      // try-with-finally and its finally hasn't yet been found
+                      // in the instruction stream. Otherwise we just need to
+                      // branch to t.
+                      if (t->has_finally() && !t->eh->has_handled_finally) {
+                        MergeFinallyToken(ssa_env_, t, token_value);
+                      }
+                    }
+                  }
+                  ssa_env_ = Split(copy);
+                  ssa_env_->control = BUILD(IfDefault, sw);
+                  Control* t = c;
+                  MergeInto(t->end_env, &t->node, &t->type, val);

[bradnelson, 2016/08/23 05:49:42]

Just use c instead of copy to t ?

[John, 2016/08/23 11:38:19]

Done.

+                  // Not a finally env; no fallthrough token.
+                }
+                ssa_env_ = break_env;
+              }
             }
 
             if (ssa_env_->go()) {
@@ -892,7 +998,7 @@ class WasmFullDecoder : public WasmDecoder {
             SetEnv(name, c->end_env);
             stack_.resize(c->stack_depth);
             Push(c->type, c->node);
-            control_.pop_back();
+            PopControl();
             break;
           }
           case kExprSelect: {
@@ -926,7 +1032,7 @@ class WasmFullDecoder : public WasmDecoder {
             Value val = {pc_, nullptr, kAstStmt};
             if (operand.arity) val = Pop();
             if (Validate(pc_, operand, control_)) {
-              BreakTo(operand.target, val);
+              BreakTo(operand, val);
             }
             len = 1 + operand.length;
             Push(kAstEnd, nullptr);
@@ -943,7 +1049,7 @@ class WasmFullDecoder : public WasmDecoder {
               fenv->SetNotMerged();
               BUILD(Branch, cond.node, &tenv->control, &fenv->control);
               ssa_env_ = tenv;
-              BreakTo(operand.target, val);
+              BreakTo(operand, val);
               ssa_env_ = fenv;
             }
             len = 1 + operand.length;
@@ -1260,23 +1366,37 @@ class WasmFullDecoder : public WasmDecoder {
 
   void PushBlock(SsaEnv* end_env) {
     const int stack_depth = static_cast<int>(stack_.size());
-    control_.emplace_back(Control::Block(pc_, stack_depth, end_env));
+    control_.emplace_back(
+        Control::Block(pc_, stack_depth, most_recent_finally_, end_env));
   }
 
   void PushLoop(SsaEnv* end_env) {
     const int stack_depth = static_cast<int>(stack_.size());
-    control_.emplace_back(Control::Loop(pc_, stack_depth, end_env));
+    control_.emplace_back(
+        Control::Loop(pc_, stack_depth, most_recent_finally_, end_env));
   }
 
   void PushIf(SsaEnv* end_env, SsaEnv* false_env) {
     const int stack_depth = static_cast<int>(stack_.size());
-    control_.emplace_back(Control::If(pc_, stack_depth, end_env, false_env));
+    control_.emplace_back(Control::If(pc_, stack_depth, most_recent_finally_,
+                                      end_env, false_env));
   }
 
   void PushTry(SsaEnv* end_env, SsaEnv* catch_env, SsaEnv* finish_try_env) {
     const int stack_depth = static_cast<int>(stack_.size());
-    control_.emplace_back(
-        Control::Try(pc_, stack_depth, end_env, catch_env, finish_try_env));
+    control_.emplace_back(Control::Try(pc_, stack_depth, most_recent_finally_,
+                                       zone_, end_env, catch_env,
+                                       finish_try_env));
+    if (control_.back().has_finally()) {
+      most_recent_finally_ = static_cast<uint32_t>(control_.size() - 1);
+    }
+  }
+
+  void PopControl() {
+    const Control& c = control_.back();
+    most_recent_finally_ = c.prev_finally;
+    control_.pop_back();
+    // No more accesses to (danging pointer) c
   }
 
   int DecodeLoadMem(LocalType type, MachineType mem_type) {
@@ -1375,7 +1495,52 @@ class WasmFullDecoder : public WasmDecoder {
 
   int startrel(const byte* ptr) { return static_cast<int>(ptr - start_); }
 
-  void BreakTo(Control* block, Value& val) {
+  Control* BuildFinallyChain(const BreakDepthOperand& operand, const Value& val,
+                             int32_t* token) {
+    const int32_t target_index =
+        static_cast<uint32_t>(control_.size() - operand.depth - 1);

[bradnelson, 2016/08/23 05:49:43]

DCHECK the operand.depth makes sense?

[John, 2016/08/23 11:38:19]

Well, the code already handles that, but I am a firm believer in asserts (plus,
you should never blindly trust your inputs anyway.) Done.

+
+    *token = kNullFinallyToken;
+    Control* first_control = nullptr;
+    Control* previous_control = nullptr;
+
+    for (int32_t ii = most_recent_finally_; ii >= target_index;
+         ii = previous_control->prev_finally) {
+      Control* current_finally = &control_[ii];
+
+      DCHECK(!current_finally->eh->has_handled_finally);
+      if (first_control == nullptr) {

[bradnelson, 2016/08/23 05:49:42]

It seems like this would read better if the empty case got handled above the for
loop?
Then you could move the first_control == nullptr stuff up, and unconditionally
allocate the new token up front?

[John, 2016/08/23 11:38:19]

Nice! done.

+        *token = NewPathTokenForFinally();
+        first_control = current_finally;
+      } else {
+        previous_control->eh->path_tokens.push_back(
+            {*token, current_finally, val});
+      }
+      previous_control = current_finally;
+    }
+
+    if (first_control == nullptr) {
+      // No finally between ssa_env_ and operand.target.
+      DCHECK(!operand.target->has_finally());
+      DCHECK_EQ(*token, kNullFinallyToken);
+      return operand.target;
+    }
+
+    if (operand.target != previous_control) {
+      DCHECK_NOT_NULL(previous_control);
+      DCHECK(previous_control->has_finally());
+      DCHECK_NE(*token, kNullFinallyToken);
+      previous_control->eh->path_tokens.push_back(
+          {*token, operand.target, val});
+    }
+
+    return first_control;
+  }
+
+  void BreakTo(const BreakDepthOperand& operand, const Value& val) {
+    int32_t finally_token;
+    Control* block = BuildFinallyChain(operand, val, &finally_token);
+
     if (block->is_loop) {
       // This is the inner loop block, which does not have a value.
       Goto(ssa_env_, block->end_env);
@@ -1383,9 +1548,14 @@ class WasmFullDecoder : public WasmDecoder {
       // Merge the value into the production for the block.
       MergeInto(block->end_env, &block->node, &block->type, val);
     }
+
+    if (finally_token != kNullFinallyToken) {
+      MergeFinallyToken(ssa_env_, block, finally_token);
+    }
   }
 
-  void MergeInto(SsaEnv* target, TFNode** node, LocalType* type, Value& val) {
+  void MergeInto(SsaEnv* target, TFNode** node, LocalType* type,
+                 const Value& val) {
     if (!ssa_env_->go()) return;
     DCHECK_NE(kAstEnd, val.type);
 
@@ -1442,6 +1612,32 @@ class WasmFullDecoder : public WasmDecoder {
     }
   }
 
+  void MergeFinallyToken(SsaEnv*, Control* to, int32_t new_token) {
+    DCHECK(to->has_finally());
+    DCHECK(!to->eh->has_handled_finally);
+    if (builder_ == nullptr) {

[bradnelson, 2016/08/23 05:49:42]

Why?

[John, 2016/08/23 11:38:20]

I honestly do not know why the builder would be nullptr, but the code seems to
handle this, so I assume it is possible.

So, assuming builder_ may be nullptr, then the switch statement below would need
extra logic in the kMerged case (I would no longer be able to
DCHECK_NOT_NULL(to->eh->token) because the token would never have been created
in the kReached case.)

[titzer, 2016/08/23 13:56:16]

It can become nullptr if you are in unreachable code or if there is an error.

+      return;
+    }
+
+    switch (to->end_env->state) {
+      case SsaEnv::kReached:
+        DCHECK(to->eh->token == nullptr);
+        to->eh->token = builder_->Int32Constant(new_token);
+        break;
+      case SsaEnv::kMerged:
+        DCHECK_NOT_NULL(to->eh->token);
+        to->eh->token =
+            CreateOrMergeIntoPhi(kAstI32, to->end_env->control, to->eh->token,
+                                 builder_->Int32Constant(new_token));
+        break;
+      case SsaEnv::kUnreachable:
+        UNREACHABLE();
+      // fallthrough intended.
+      default:
+        break;
+    }
+  }
+
   void Goto(SsaEnv* from, SsaEnv* to) {
     DCHECK_NOT_NULL(to);
     if (!from->go()) return;