Skip to content

Commit

Permalink
Fix emitter crash from a goto inside a try-finally
Browse files Browse the repository at this point in the history
Summary: If there was a goto inside the try block of a try-finally, with no other
region (such as a loop) around it, the emitter would crash. We didn't
need to check for m_parent's goto labels, because the loop header
already starts the search at the current region's parent.

Note that this still behaves incorrectly for a goto out of two nested
try-finally regions, e.g.:

```
try {
  try {
    goto bluh;
  } finally {
    // one
  }
} finally {
  // two
}
bluh:
...
```

In this case, `one` will get executed and then jump straight to `bluh`.
None of our active test cases test this yet (some of the zend/bad ones
do, though). I think this may require a deeper fix.

Reviewed By: @jdelong

Differential Revision: D1555786
  • Loading branch information
oyamauchi authored and hhvm-bot committed Sep 17, 2014
1 parent a29e382 commit 5906603
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 1 deletion.
2 changes: 1 addition & 1 deletion hphp/compiler/analysis/emitter.cpp
Expand Up @@ -1657,7 +1657,7 @@ void EmitterVisitor::emitGotoTrampoline(Emitter& e,
for (region = region->m_parent.get(); true; region = region->m_parent.get()) {
assert(region->m_gotoTargets.count(name));
auto t = region->m_gotoTargets[name].target;
if (region->m_parent->m_gotoLabels.count(name)) {
if (region->m_gotoLabels.count(name)) {
// If only there is the appropriate label inside the current region
// perform a jump.
Id stateLocal = getStateLocal();
Expand Down
17 changes: 17 additions & 0 deletions hphp/test/slow/finally/goto-bare.php
@@ -0,0 +1,17 @@
<?php

function foo() {
try {
goto label;
} finally {
var_dump("finally1");
}

return "wrong!";

label:
var_dump("label");
return "return2";
}

var_dump(foo());
3 changes: 3 additions & 0 deletions hphp/test/slow/finally/goto-bare.php.expect
@@ -0,0 +1,3 @@
string(8) "finally1"
string(5) "label"
string(7) "return2"

0 comments on commit 5906603

Please sign in to comment.