Mark exception handlers if they have a stacktrace specified. Do not build a stacktrace if the handl…

Mark exception handlers if they have a stacktrace specified. Do not build a stacktrace if the handler has no stacktrace.

R=asiva@google.com, hausner@google.com

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

[hausner, 2013-08-28 16:08:20]

LGTM but please also get the LGTS(iva).

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:198: uword handler_pc = NULL;
Should a uword variable not be initialized with 0? NULL is a pointer value.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/object_test.cc...
runtime/vm/object_test.cc:2389: const bool kNeedsNoStacktrace = false;
kNoStackTrace ?

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:948: bool needs_stacktrace;     // True if a stacktrace
is needed.
Due to alignment of HandlerInfo elements in the data_ array, this adds 4 bytes
for each exception handler (i.e. catch clause), 8 bytes even in 64bit mode. I
don't know how much that is in total, but we should at least add a TODO here
that we should pack the data more densely. For example, the outer_try_index does
not need to be an intptr_t, I do not think that we'll ever have to deal with
functions that contain 4 billion try statements. That field could be 16 bits,
and the bool can fit it with 8 bits to spare.

[siva, 2013-08-28 17:00:09]

One question regarding nested try..catch blocks where a stacktrace is declared
in the outer catch block.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/code_descripto...
File runtime/vm/code_descriptors.h (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/code_descripto...
runtime/vm/code_descriptors.h:104: bool needs_stacktrace;
// Handler catch block declares stacktrace.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:198: uword handler_pc = NULL;
On 2013/08/28 16:08:20, hausner wrote:
> Should a uword variable not be initialized with 0? NULL is a pointer value.

Maybe intialize it to kUwordMax.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:200: bool handler_pc_set = false;
The variable dart_handler_found is not needed anymore.

I think the variable handler_pc_set is also not needed anymore, you could just
go with handler_pc == kUwordMax;

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:273: return true;
Don't you have to keep going till you hit the entry frame to see
if a stacktrace is needed?
Consider this case:

try {
  ....
  ....
  try {
     ...
     throw TypeAException_obj;
  } on TypeBException catch (e) {
     ...
  }
  ....
} on TypeAException catch (e, stacktrace) {
  ....
}

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:289: return false;
Since you are not building a full stack trace anymore here it is not
necessary to have the outer while loop. you could just do:

while (!frame->IsEntryFrame()) {
  ....
  ....
}
ASSERT(frame->IsEntryFrame());
...
*handler_fp - frame->fp();
return false;

(The variable handler_pc_set is also not needed.)

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:948: bool needs_stacktrace;     // True if a stacktrace
is needed.
I second this comment.

On 2013/08/28 16:08:20, hausner wrote:
> Due to alignment of HandlerInfo elements in the data_ array, this adds 4 bytes
> for each exception handler (i.e. catch clause), 8 bytes even in 64bit mode. I
> don't know how much that is in total, but we should at least add a TODO here
> that we should pack the data more densely. For example, the outer_try_index
does
> not need to be an intptr_t, I do not think that we'll ever have to deal with
> functions that contain 4 billion try statements. That field could be 16 bits,
> and the bool can fit it with 8 bits to spare.

[srdjan, 2013-08-28 20:38:29]

PTAL

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:198: uword handler_pc = NULL;
On 2013/08/28 17:00:09, siva wrote:
> On 2013/08/28 16:08:20, hausner wrote:
> > Should a uword variable not be initialized with 0? NULL is a pointer value.
> 
> Maybe intialize it to kUwordMax.

Initialized to kUWordMax and moved close to frame->FindExceptionHandler call.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:198: uword handler_pc = NULL;
On 2013/08/28 16:08:20, hausner wrote:
> Should a uword variable not be initialized with 0? NULL is a pointer value.

Using kUWordMax

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:200: bool handler_pc_set = false;
On 2013/08/28 17:00:09, siva wrote:
> The variable dart_handler_found is not needed anymore.

Offline discussion: it is needed to note the catch frame.

> 
> I think the variable handler_pc_set is also not needed anymore, you could just
> go with handler_pc == kUwordMax;

Moved close to its only usage.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:273: return true;
On 2013/08/28 17:00:09, siva wrote:
> Don't you have to keep going till you hit the entry frame to see
> if a stacktrace is needed?
> Consider this case:
> 
> try {
>   ....
>   ....
>   try {
>      ...
>      throw TypeAException_obj;
>   } on TypeBException catch (e) {
>      ...
>   }
>   ....
> } on TypeAException catch (e, stacktrace) {
>   ....
> }

Good 'catch'!

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:289: return false;
On 2013/08/28 17:00:09, siva wrote:
> Since you are not building a full stack trace anymore here it is not
> necessary to have the outer while loop. you could just do:
> 
> while (!frame->IsEntryFrame()) {
>   ....
>   ....
> }
> ASSERT(frame->IsEntryFrame());
> ...
> *handler_fp - frame->fp();
> return false;
> 
> (The variable handler_pc_set is also not needed.)

Done.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/object_test.cc...
runtime/vm/object_test.cc:2389: const bool kNeedsNoStacktrace = false;
On 2013/08/28 16:08:20, hausner wrote:
> kNoStackTrace ?

Done.

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/23445012/diff/48001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:948: bool needs_stacktrace;     // True if a stacktrace
is needed.
On 2013/08/28 17:00:09, siva wrote:
> I second this comment.
> 
> On 2013/08/28 16:08:20, hausner wrote:
> > Due to alignment of HandlerInfo elements in the data_ array, this adds 4
bytes
> > for each exception handler (i.e. catch clause), 8 bytes even in 64bit mode.
I
> > don't know how much that is in total, but we should at least add a TODO here
> > that we should pack the data more densely. For example, the outer_try_index
> does
> > not need to be an intptr_t, I do not think that we'll ever have to deal with
> > functions that contain 4 billion try statements. That field could be 16
bits,
> > and the bool can fit it with 8 bits to spare.
> 

Will do that in a follow up CL.

[siva, 2013-08-28 22:07:00]

lgtm

https://codereview.chromium.org/23445012/diff/11001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/23445012/diff/11001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:270: bool is_catch_all = false;
This can be hoisted above the while (..) loop?

https://codereview.chromium.org/23445012/diff/11001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:291: }
Maybe the code could be simplified as:

uword dummy_handler_pc;
if (frame->FindExceptionHandler(&dummy_handler_pc,
                                needs_stacktrace,
                                &is_catch_all)) {
  if (!handler_pc_set) {
    handler_pc_set = true;
    *handler_pc = dummy_handler_pc;
    *handler_sp = frame->sp();
    *handler_fp = frame->fp();
  }
  if (*needs_stacktrace || is_catch_all) {
    return true;
  }
}

[srdjan, 2013-08-28 22:37:20]

https://codereview.chromium.org/23445012/diff/11001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/23445012/diff/11001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:270: bool is_catch_all = false;
On 2013/08/28 22:07:00, siva wrote:
> This can be hoisted above the while (..) loop?

Done

https://codereview.chromium.org/23445012/diff/11001/runtime/vm/exceptions.cc#...
runtime/vm/exceptions.cc:291: }
On 2013/08/28 22:07:00, siva wrote:
> Maybe the code could be simplified as:
> 
> uword dummy_handler_pc;
> if (frame->FindExceptionHandler(&dummy_handler_pc,
>                                 needs_stacktrace,
>                                 &is_catch_all)) {
>   if (!handler_pc_set) {
>     handler_pc_set = true;
>     *handler_pc = dummy_handler_pc;
>     *handler_sp = frame->sp();
>     *handler_fp = frame->fp();
>   }
>   if (*needs_stacktrace || is_catch_all) {
>     return true;
>   }
> }

Nice! Done.

[srdjan, 2013-08-28 22:42:33]

Committed patchset #10 manually as r26823 (presubmit successful).