New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SI-10032 Fix code gen with returns in nested try-finally blocks #5509

Merged
merged 2 commits into from Nov 10, 2016

Conversation

Projects
None yet
3 participants
@lrytz
Member

lrytz commented Nov 8, 2016

Return statements within try or catch blocks need special treatement
if there's also a finally

try { return 1 } finally { println() }

For the return, the code generator emits a store to a local and a jump
to a "cleanup" version of the finally block. There will be 3 versions
of the finally block:

  • One reached through a handler, if the code in the try block
    throws; re-throws at the end
  • A "cleanup" version reached from returns within the try; reads the
    local and returns the value at the end
  • One reached for ordinary control flow, if there's no return and no
    exception within the try

If there are multiple enclosing finally blocks, a "cleanup" version is
emitted for each of them. The nested ones jump to the enclosing ones,
the outermost one reads the local and returns.

A global variable shouldEmitCleanup stores whether cleanup versions
are required for the curren finally blocks. By mistake, this variable
was not reset to false when emitting a try-finally nested within a
finally:

try {
  try { return 1 }
  finally { println() } // need cleanup version
} finally {             // need cleanup version
  try { println() }
  finally { println() } // no cleanup version needed!
}

In this commit we ensure that the variable is reset when emitting
nested try-finally blocks.

SI-10032 Fix code gen with returns in nested try-finally blocks
Return statements within `try` or `catch` blocks need special treatement
if there's also a `finally`

    try { return 1 } finally { println() }

For the return, the code generator emits a store to a local and a jump
to a "cleanup" version of the finally block. There will be 3 versions
of the finally block:

  - One reached through a handler, if the code in the try block
    throws; re-throws at the end
  - A "cleanup" version reached from returns within the try; reads the
    local and returns the value at the end
  - One reached for ordinary control flow, if there's no return and no
    exception within the try

If there are multiple enclosing finally blocks, a "cleanup" version is
emitted for each of them. The nested ones jump to the enclosing ones,
the outermost one reads the local and returns.

A global variable `shouldEmitCleanup` stores whether cleanup versions
are required for the curren finally blocks. By mistake, this variable
was not reset to `false` when emitting a `try-finally` nested within a
`finally`:

    try {
      try { return 1 }
      finally { println() } // need cleanup version
    } finally {             // need cleanup version
      try { println() }
      finally { println() } // no cleanup version needed!
    }

In this commit we ensure that the variable is reset when emitting
nested `try-finally` blocks.

@scala-jenkins scala-jenkins added this to the 2.12.1 milestone Nov 8, 2016

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Nov 8, 2016

Member

review by @retronym

Member

lrytz commented Nov 8, 2016

review by @retronym

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Nov 8, 2016

Member

Not sure if related to this change, but can you shed some light on:

class Test {
    public static void main(String[] args) {
        System.out.println(test()); // prints 2
    }
    static int test() {
        try { return 1; } finally { try { return 2; } finally {} } 
    }
}
object Test {
  def main(args: Array[String]): Unit = {
    // Scala 2.11: 2
    // Scala 2.12: "sandbox/test.scala:7: warning: Return statement found in finally-clause, discarding its return-value in favor of that of a more deeply nested return.", prints 1
    println(test);
  }
  def test: Int = {
    try { return 1 } finally { try { return 2 } finally {} }
  }
}
Member

retronym commented Nov 8, 2016

Not sure if related to this change, but can you shed some light on:

class Test {
    public static void main(String[] args) {
        System.out.println(test()); // prints 2
    }
    static int test() {
        try { return 1; } finally { try { return 2; } finally {} } 
    }
}
object Test {
  def main(args: Array[String]): Unit = {
    // Scala 2.11: 2
    // Scala 2.12: "sandbox/test.scala:7: warning: Return statement found in finally-clause, discarding its return-value in favor of that of a more deeply nested return.", prints 1
    println(test);
  }
  def test: Int = {
    try { return 1 } finally { try { return 2 } finally {} }
  }
}
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Nov 9, 2016

Member

Uh, very interesting. I should have caught that. It's most likely a difference between GenASM and GenBCode. Need to read the spec.

Member

lrytz commented Nov 9, 2016

Uh, very interesting. I should have caught that. It's most likely a difference between GenASM and GenBCode. Need to read the spec.

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Nov 9, 2016

Member

I'd say the scala spec is incomplete about the fact that finally blocks are executed if a return occurs in a protected area: it just says "The evaluation of any statements or expressions following the return expression is omitted".

So we should follow what java does.

The java spec says that a "return completes abruptly", it "attempts to transfer control to the invoker", but that attempt may fail: finally statements of enclosing try are executed first, and "Abrupt completion of a finally clause can disrupt the transfer of control initiated by a return statement".

Abrupt completion is basically returning or throwing.

The spec of completion of a try-finally statement is a bit complicated, but basically it says: if the finally blocks completes abruptly for reason S, then the entire try statement completes abruptly with reason S. An abrupt termination of the try block for a different reason R is discarded.

So the behavior after this PR is wrong for your example. Given that your example doesn't compile with 2.12.0 (crashes the compiler), it should be safe to fix it for 2.12.1.

Member

lrytz commented Nov 9, 2016

I'd say the scala spec is incomplete about the fact that finally blocks are executed if a return occurs in a protected area: it just says "The evaluation of any statements or expressions following the return expression is omitted".

So we should follow what java does.

The java spec says that a "return completes abruptly", it "attempts to transfer control to the invoker", but that attempt may fail: finally statements of enclosing try are executed first, and "Abrupt completion of a finally clause can disrupt the transfer of control initiated by a return statement".

Abrupt completion is basically returning or throwing.

The spec of completion of a try-finally statement is a bit complicated, but basically it says: if the finally blocks completes abruptly for reason S, then the entire try statement completes abruptly with reason S. An abrupt termination of the try block for a different reason R is discarded.

So the behavior after this PR is wrong for your example. Given that your example doesn't compile with 2.12.0 (crashes the compiler), it should be safe to fix it for 2.12.1.

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Nov 9, 2016

Member

OK, here's an example that will change semantics in 2.12.1, compared to 2.12.0:

def t: Int = try {
  try { return 1 }
  finally { return 2 }
} finally { println("hi") }

In 2.11.8, the result is 2, while in 2.12.0 it's 1, which is wrong. It's also wrong in 2.11.8 with -Ybackend:GenBCode. I'm pretty sure we should fix this for 2.12.1, and point it out in the release notes.

Member

lrytz commented Nov 9, 2016

OK, here's an example that will change semantics in 2.12.1, compared to 2.12.0:

def t: Int = try {
  try { return 1 }
  finally { return 2 }
} finally { println("hi") }

In 2.11.8, the result is 2, while in 2.12.0 it's 1, which is wrong. It's also wrong in 2.11.8 with -Ybackend:GenBCode. I'm pretty sure we should fix this for 2.12.1, and point it out in the release notes.

Fix returns from within finalizers
When a return in a finalizer was reached through a return within the try
block, the backend ignored the return in the finalizer:

    try {
      try { return 1 }
      finally { return 2 }
    } finally { println() }

This expression should evaluate to 2 (it does in 2.11.8), but in 2.12.0
it the result is 1.

The Scala spec is currently incomplete, it does not say that a finalizer
should be exectuted if a return occurs within a try block, and it does
not specify what happens if also the finally block has a return.

So we follow the Java spec, which basically says: if the finally blocks
completes abruptly for reason S, then the entire try statement completes
abruptly with reason S. An abrupt termination of the try block for a
different reason R is discarded.

Abrupt completion is basically returning or throwing.
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Nov 9, 2016

Member

/rebuild

Member

retronym commented Nov 9, 2016

/rebuild

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Nov 9, 2016

Member

LGTM. Would have been a good candidate bug to find with a fuzzer that compares behaviour to equivalent Java.

Member

retronym commented Nov 9, 2016

LGTM. Would have been a good candidate bug to find with a fuzzer that compares behaviour to equivalent Java.

@lrytz lrytz added the release-notes label Nov 10, 2016

@lrytz lrytz merged commit 9ca14a5 into scala:2.12.x Nov 10, 2016

6 checks passed

cla @lrytz signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [3489] SUCCESS. Took 14 s.
Details
validate-main [4020] SUCCESS. Took 53 min.
Details
validate-publish-core [3890] SUCCESS. Took 46 s.
Details
validate-test [3397] SUCCESS. Took 48 min.
Details

@lrytz lrytz deleted the lrytz:t10032 branch Apr 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment