Replaces LongJump with LongJumpScope.

Replaces LongJump with LongJumpScope.

LongJump required explicitly restoring the old
long jump base before function return. This was
sometimes forgotten, leading to crashes. This
change puts the base restoration into the
destructor so that it happens automatically on
return.

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31463

[zra, 2014-01-03 19:43:16]

Didn't have any problems building on Mac, or on Linux with any of the
cross-compilers.

[zra, 2014-01-03 20:55:34]

Also builds and tests pass on Windows.

[siva, 2014-01-03 21:18:35]

I have one question regarding how this will work in the case where
ThrowException is called in one of the paths
(the destructor will not run as it is not a StackResource object).

https://codereview.chromium.org/108383007/diff/1/runtime/vm/longjump.cc
File runtime/vm/longjump.cc (right):

https://codereview.chromium.org/108383007/diff/1/runtime/vm/longjump.cc#newco...
runtime/vm/longjump.cc:20: return &environment_;
We need an assertion here to ensure that Set is called only once within a
LongJumpScope.

The code below for example is illegal now:

{
  LongJumpScope ls;
  ...
  if (setjmp(*ls.Set()) == 0) {
    ...
  } else {
    ...
  }
  ...
  ...
  if (setjmp(*ls.Set() == 0) {
    ...
  } else {
    ...
  }
}

It was ok before because of the manual reset.

https://codereview.chromium.org/108383007/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/108383007/diff/1/runtime/vm/snapshot.cc#newco...
runtime/vm/snapshot.cc:1508: ThrowException(exception_type(), exception_msg());
How does this work now for the ThrowException case,
The destructor for LongJumpScope will not run as it is not a
StackResource.

[zra, 2014-01-03 21:52:59]

https://codereview.chromium.org/108383007/diff/1/runtime/vm/longjump.cc
File runtime/vm/longjump.cc (right):

https://codereview.chromium.org/108383007/diff/1/runtime/vm/longjump.cc#newco...
runtime/vm/longjump.cc:20: return &environment_;
On 2014/01/03 21:18:35, siva wrote:
> We need an assertion here to ensure that Set is called only once within a
> LongJumpScope.
> 
> The code below for example is illegal now:
> 
> {
>   LongJumpScope ls;
>   ...
>   if (setjmp(*ls.Set()) == 0) {
>     ...
>   } else {
>     ...
>   }
>   ...
>   ...
>   if (setjmp(*ls.Set() == 0) {
>     ...
>   } else {
>     ...
>   }
> }
> 
> It was ok before because of the manual reset.
> 

Done.

https://codereview.chromium.org/108383007/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/108383007/diff/1/runtime/vm/snapshot.cc#newco...
runtime/vm/snapshot.cc:1508: ThrowException(exception_type(), exception_msg());
On 2014/01/03 21:18:35, siva wrote:
> How does this work now for the ThrowException case,
> The destructor for LongJumpScope will not run as it is not a
> StackResource.

I see two options:
1. Treat this as a special case and say something like:
{
  LongJumpScope jump;
  if (setjmp(*jump.Set()) == 0) {
    ...
    return;
  }
}
ThrowException();

Or

2. Split LongJumpScope into two classes: one that is a StackResource and only
maintains the long_jump_base, and one that is as before and only tracks the
other StackResources.

What sounds good?

[siva, 2014-01-04 00:04:22]

https://codereview.chromium.org/108383007/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/108383007/diff/1/runtime/vm/snapshot.cc#newco...
runtime/vm/snapshot.cc:1508: ThrowException(exception_type(), exception_msg());
I prefer the first.

We can add an assertion in ThrowException to ensure that people
don't throw inside a current LongJumpScope.

On 2014/01/03 21:52:59, zra wrote:
> On 2014/01/03 21:18:35, siva wrote:
> > How does this work now for the ThrowException case,
> > The destructor for LongJumpScope will not run as it is not a
> > StackResource.
> 
> I see two options:
> 1. Treat this as a special case and say something like:
> {
>   LongJumpScope jump;
>   if (setjmp(*jump.Set()) == 0) {
>     ...
>     return;
>   }
> }
> ThrowException();
> 
> Or
> 
> 2. Split LongJumpScope into two classes: one that is a StackResource and only
> maintains the long_jump_base, and one that is as before and only tracks the
> other StackResources.
> 
> What sounds good?

[Ivan Posva, 2014-01-04 00:37:05]

LGTM with the fix for exception throwing we discussed in person.

-Ivan

[zra, 2014-01-04 01:23:50]

Committed patchset #3 manually as r31463 (presubmit successful).