Changed StackZone and ApiZone to be containers for Zone.

Changed StackZone and ApiZone to be containers for Zone.


BUG=

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

[siva, 2012-10-12 20:40:21]

LGTM with one question that we discussed offline regarding the test that caused
you to make the zone links a doubly linked list.

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/code_descr...
File runtime/vm/code_descriptors_test.cc (right):

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/code_descr...
runtime/vm/code_descriptors_test.cc:285: (void) kNoArgumentNames;
we have a macro USE for silencing compiler warnings if that is what you are
trying to get rid of here.

Why was this not a problem before.

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/dart_api_i...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/dart_api_i...
runtime/vm/dart_api_impl.cc:203: char* buffer = zone.GetZone()->Alloc<char>(len
+ 1);
This would be isolate->current_zone()->Alloc ?

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/handles.h
File runtime/vm/handles.h (right):

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/handles.h#...
runtime/vm/handles.h:180: static const uword kZapUninitializedWord = (uword)
0xabababababababab;
We should probably move this to vm/globals.h and do
a conditional initialization of this based on
ARCH_IS_32_BIT

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/zone.h#new...
runtime/vm/zone.h:142: Zone* next_;
As discussed offline, I wanted to understand the test failure that caused the
need for this to be a doubly linked list.

[Ivan Posva, 2012-10-12 21:46:33]

Some nits.

As Siva said the need for doubly-linked list pointed to some issue. It sounds
like we have good plan forward now though.

-Ivan

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/dart_api_s...
File runtime/vm/dart_api_state.h (right):

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/dart_api_s...
runtime/vm/dart_api_state.h:452: previous_(previous),
stack_marker_(stack_marker) {
Emptr constructors on one line.

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/handles.h
File runtime/vm/handles.h (right):

https://chromiumcodereview.appspot.com/11028145/diff/22/runtime/vm/handles.h#...
runtime/vm/handles.h:180: static const uword kZapUninitializedWord = (uword)
0xabababababababab;
Please use C++ style casts.

[Tom Ball, 2012-10-12 22:56:51]

PTAL:

. The zone list is no longer a double-linked list;
. Dart_PropagateError, Dart_ThrowException, and Dart_ReThrowException
  now use raw handles to bridge scope unwinding; and
. code review feedback incorporated.

http://codereview.chromium.org/11028145/diff/22/runtime/vm/code_descriptors_t...
File runtime/vm/code_descriptors_test.cc (right):

http://codereview.chromium.org/11028145/diff/22/runtime/vm/code_descriptors_t...
runtime/vm/code_descriptors_test.cc:285: (void) kNoArgumentNames;
It wasn't a problem, just debugging cruft that's now removed.

http://codereview.chromium.org/11028145/diff/22/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/22/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:203: char* buffer = zone.GetZone()->Alloc<char>(len
+ 1);
On 2012/10/12 20:40:21, siva wrote:
> This would be isolate->current_zone()->Alloc ?

Done.

http://codereview.chromium.org/11028145/diff/22/runtime/vm/dart_api_state.h
File runtime/vm/dart_api_state.h (right):

http://codereview.chromium.org/11028145/diff/22/runtime/vm/dart_api_state.h#n...
runtime/vm/dart_api_state.h:452: previous_(previous),
stack_marker_(stack_marker) {
On 2012/10/12 21:46:33, Ivan Posva wrote:
> Emptr constructors on one line.

Done.

http://codereview.chromium.org/11028145/diff/22/runtime/vm/handles.h
File runtime/vm/handles.h (right):

http://codereview.chromium.org/11028145/diff/22/runtime/vm/handles.h#newcode180
runtime/vm/handles.h:180: static const uword kZapUninitializedWord = (uword)
0xabababababababab;
On 2012/10/12 20:40:21, siva wrote:
> We should probably move this to vm/globals.h and do
> a conditional initialization of this based on
> ARCH_IS_32_BIT

Done.

http://codereview.chromium.org/11028145/diff/22/runtime/vm/handles.h#newcode180
runtime/vm/handles.h:180: static const uword kZapUninitializedWord = (uword)
0xabababababababab;
Removed cast.

http://codereview.chromium.org/11028145/diff/22/runtime/vm/zone.h
File runtime/vm/zone.h (right):

http://codereview.chromium.org/11028145/diff/22/runtime/vm/zone.h#newcode142
runtime/vm/zone.h:142: Zone* next_;
The double-linked list is no longer necessary, now that the exception routines
were updated to survive ApiZone unwinding.

[Ivan Posva, 2012-10-12 23:04:27]

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:460: RawObject* raw_obj = Api::UnwrapHandle(handle);
Please wrap the area where you keep the raw object in a "NoGCScope no_gc;".

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:464: return Api::NewError(
Which will make you fail here. You are allocating at this point, so the raw_obj
could be invalid afterwards because the GC has the potential to move it in
memory.

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:479:
state->UnwindScopes(isolate->top_exit_frame_info());
Get the raw_obj out of the handle just before unwinding scopes here.

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3921: RawObject* raw_excp =
Api::UnwrapHandle(exception);
NoGCScope while you hold on to raw objects.

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3952: RawObject* raw_excp =
Api::UnwrapHandle(exception);
ditto: NoGCScope is needed.

[Tom Ball, 2012-10-15 17:17:40]

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:460: RawObject* raw_obj = Api::UnwrapHandle(handle);
On 2012/10/12 23:04:27, Ivan Posva wrote:
> Please wrap the area where you keep the raw object in a "NoGCScope no_gc;".

Done.

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:464: return Api::NewError(
Moved allocation to where you suggested.

http://codereview.chromium.org/11028145/diff/10001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:479:
state->UnwindScopes(isolate->top_exit_frame_info());
On 2012/10/12 23:04:27, Ivan Posva wrote:
> Get the raw_obj out of the handle just before unwinding scopes here.

Done.

[siva, 2012-10-15 17:50:35]

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:459: DART_EXPORT Dart_Handle
UnwindApiScopes(Isolate* isolate, Dart_Handle handle) {
Why not make this a static method?

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:465: return Api::NewHandle(isolate, raw_obj);
These new handles are created in the outer API scope so one could potentially
write a dart loop which calls a native method that throws an exception and ends
up with a bunch of handles in the outer API scope and run out of memory.

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3938: Dart_Handle* stacktrace) {
Ditto, why not make this a static method.

[Ivan Posva, 2012-10-15 18:26:01]

https://chromiumcodereview.appspot.com/11028145/diff/20003/runtime/vm/dart_ap...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/11028145/diff/20003/runtime/vm/dart_ap...
runtime/vm/dart_api_impl.cc:459: DART_EXPORT Dart_Handle
UnwindApiScopes(Isolate* isolate, Dart_Handle handle) {
There should be no need to return a Dart_Handle here. It leads to the problems
Siva describes.

https://chromiumcodereview.appspot.com/11028145/diff/20003/runtime/vm/dart_ap...
runtime/vm/dart_api_impl.cc:487: handle = UnwindApiScopes(isolate, handle);
This avoids Siva's concern:

  ASSERT(state != NULL);
  Object* error;
  {
    NoGCScope no_gc;
    RawObject* raw_obj = Api::UnwrapHandle(handle);
    state->UnwindScopes(isolate->top_exit_frame_info());
    error = &Object::Handle(raw_obj);
  }
  Exceptions::PropagateError(Error::Cast(error));

https://chromiumcodereview.appspot.com/11028145/diff/20003/runtime/vm/dart_ap...
runtime/vm/dart_api_impl.cc:3945: *exception = Api::NewHandle(isolate,
raw_excp);
No Dart_Handles as they can leak out.

[Tom Ball, 2012-10-15 19:51:06]

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:459: DART_EXPORT Dart_Handle
UnwindApiScopes(Isolate* isolate, Dart_Handle handle) {
On 2012/10/15 17:50:35, siva wrote:
> Why not make this a static method?

Done.

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:459: DART_EXPORT Dart_Handle
UnwindApiScopes(Isolate* isolate, Dart_Handle handle) {
Inlined function.

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:465: return Api::NewHandle(isolate, raw_obj);
Removed function, as Ivan described.

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:487: handle = UnwindApiScopes(isolate, handle);
Integrated recommended change.

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3938: Dart_Handle* stacktrace) {
Inlined code and removed function.

http://codereview.chromium.org/11028145/diff/20003/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3945: *exception = Api::NewHandle(isolate,
raw_excp);
Removed, followed pattern you recommended for PropagateError in
Dart_ReThrowException below.

[Ivan Posva, 2012-10-15 20:32:17]

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:461: const Object& obj = Object::Handle(isolate,
Api::UnwrapHandle(handle));
How about adding a comment explaining that we are preventing this handle to live
across the destruction of zones below. Otherwise the motivation of this extra
block is unclear.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:478: Object* error;
How about Error*?Then you can avoid the cast below and you already know it is an
error.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:480: NoGCScope no_gc;
// We need to preserve the error object across the destruction of zones. Ensure
// that no GCs can be triggered and preserve the raw object. Before creating a
// valid handle in the surviving zone.

Or something similar.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3929: Exceptions::PropagateError(Error::Cast(*exc));
?

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3930: const Instance& excp =
Api::UnwrapInstanceHandle(isolate, exception);
The exception handle is very likely invalid at this point. As it was probably
allocated in the zone that was just removed.

[Tom Ball, 2012-10-15 21:48:46]

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:461: const Object& obj = Object::Handle(isolate,
Api::UnwrapHandle(handle));
On 2012/10/15 20:32:17, Ivan Posva wrote:
> How about adding a comment explaining that we are preventing this handle to
live
> across the destruction of zones below. Otherwise the motivation of this extra
> block is unclear.

Done.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:478: Object* error;
On 2012/10/15 20:32:17, Ivan Posva wrote:
> How about Error*?Then you can avoid the cast below and you already know it is
an
> error.

Done.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:480: NoGCScope no_gc;
On 2012/10/15 20:32:17, Ivan Posva wrote:
> // We need to preserve the error object across the destruction of zones.
Ensure
> // that no GCs can be triggered and preserve the raw object. Before creating a
> // valid handle in the surviving zone.
> 
> Or something similar.

Done.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3929: Exceptions::PropagateError(Error::Cast(*exc));
Removed.

http://codereview.chromium.org/11028145/diff/18004/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:3930: const Instance& excp =
Api::UnwrapInstanceHandle(isolate, exception);
Fixed reference.

[Tom Ball, 2012-10-16 16:46:40]

The latest changes are now uploaded -- the upload had been stuck overnight.

[siva, 2012-10-16 21:50:51]

lgtm