Fix source position for stack traces with optimized top function.

Fix source position for stack traces with optimized top function.

Fix decoding of the deoptimization info when constructing a stack trace.

Fix stack trace for checked mode exceptions from optimized code.

BUG=dart:8058
TEST=runtime/tests/vm/dart/optimized_stacktrace_test.dart,
     tests/language/stack_overflow_stacktrace_test.dart
Committed: https://code.google.com/p/dart/source/detail?r=17755

[srdjan, 2013-01-28 18:03:58]

LGTM with comments

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
File runtime/tests/vm/dart/optimized_stacktrace_test.dart (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
runtime/tests/vm/dart/optimized_stacktrace_test.dart:16: for (var i=0; i<10000;
i++) foo(42);
You may attempt the test below for unoptimized code as well.

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
runtime/tests/vm/dart/optimized_stacktrace_test.dart:19: Expect.isTrue(false); 
// Unreachable.
Expect.fail(....);

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
runtime/tests/vm/dart/optimized_stacktrace_test.dart:29: max(x) => moritz(x);
I love the Max & Moritz theme, but max could ve confused with Math's max

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/vm.status
File runtime/tests/vm/vm.status (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/vm.statu...
runtime/tests/vm/vm.status:37: dart/optimized_stacktrace_test: Fail
Can you check with dart2js team  if it is ever supposed to match. If not, move
the test into standalone directory (VM only tests).

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.cc:582: *func |=
object_table.At(ret_after_instr->object_table_index());
You need to sync, there is no more |=

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:131: const Array& object_table,
Why not making this an instance method, implemented for appropriate instr only?

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler.h:400: friend class CheckStackOverflowSlowPath; 
// For pending_deoptimization_env_
Terminate with period.

[siva, 2013-01-28 22:46:55]

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

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.cc...
runtime/vm/stack_frame.cc:300: deopt_info_.ToInstructions(deopt_table,
&deopt_instructions_);
Is this some kind of code hoisting optimization you are trying to do in order to
avoid potential repeated
calls to deopt_info_.Instruction(..) as there are many inlined frames?

In that case it seems like a premature optimization to me, the trade off is
always between memory consumption and speed and I think in this case speed is
not critical.

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.cc...
runtime/vm/stack_frame.cc:307: Code* code) {
I try and avoid functions which have two return values one as a regular return
value and the other being returned as a parameter. Maybe we could change this to
always return the code object and then we get the function from it.

RawCode* InlinedFunctionsInDartFrameIterator::GetNextCode(uword* pc);

What do you think?

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.h
File runtime/vm/stack_frame.h (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.h#...
runtime/vm/stack_frame.h:242: GrowableArray<DeoptInstr*> deopt_instructions_;
See comment about adding this array in stack_frame.cc

[Florian Schneider, 2013-01-29 12:19:07]

Thanks for the feedback.

I could also simplify the code a bit since we can now assert that we always have
a deopt info when iterating an optimized frame.

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
File runtime/tests/vm/dart/optimized_stacktrace_test.dart (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
runtime/tests/vm/dart/optimized_stacktrace_test.dart:16: for (var i=0; i<10000;
i++) foo(42);
On 2013/01/28 18:03:58, srdjan wrote:
> You may attempt the test below for unoptimized code as well.

Done.

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
runtime/tests/vm/dart/optimized_stacktrace_test.dart:19: Expect.isTrue(false); 
// Unreachable.
On 2013/01/28 18:03:58, srdjan wrote:
> Expect.fail(....);

Done.

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/dart/opt...
runtime/tests/vm/dart/optimized_stacktrace_test.dart:29: max(x) => moritz(x);
On 2013/01/28 18:03:58, srdjan wrote:
> I love the Max & Moritz theme, but max could ve confused with Math's max

Renamed to maximus.

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/vm.status
File runtime/tests/vm/vm.status (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/tests/vm/vm.statu...
runtime/tests/vm/vm.status:37: dart/optimized_stacktrace_test: Fail
On 2013/01/28 18:03:58, srdjan wrote:
> Can you check with dart2js team  if it is ever supposed to match. If not, move
> the test into standalone directory (VM only tests).

After consulting, I'll leave the status as Fail instead of skipping: The output
does not match with dart2js currently, but it should not crash.

I'm still confused by the different tests directories: runtime/tests/ and
tests/language. Which tests should go where?

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.cc:582: *func |=
object_table.At(ret_after_instr->object_table_index());
On 2013/01/28 18:03:58, srdjan wrote:
> You need to sync, there is no more |=

Done.

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:131: const Array& object_table,
On 2013/01/28 18:03:58, srdjan wrote:
> Why not making this an instance method, implemented for appropriate instr
only?

I would have to move all concrete deopt instruction class declarations to the .h
file.

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler.h:400: friend class CheckStackOverflowSlowPath; 
// For pending_deoptimization_env_
On 2013/01/28 18:03:58, srdjan wrote:
> Terminate with period.

Done.

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

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.cc...
runtime/vm/stack_frame.cc:300: deopt_info_.ToInstructions(deopt_table,
&deopt_instructions_);
On 2013/01/28 22:46:55, siva wrote:
> Is this some kind of code hoisting optimization you are trying to do in order
to
> avoid potential repeated
> calls to deopt_info_.Instruction(..) as there are many inlined frames?
> 
> In that case it seems like a premature optimization to me, the trade off is
> always between memory consumption and speed and I think in this case speed is
> not critical.

No, this is not for optimization purposes: The raw deopt info contains
DeoptSuffixInstr instructions to compact redundant deopt infos. ToInstruction is
the helper to uncompress the DeoptSuffixInstr into individual deopt
instructions. It would be possible to do this while iterating, but this seems
unnecessarily complicated and duplicating the code we already have.

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.cc...
runtime/vm/stack_frame.cc:307: Code* code) {
On 2013/01/28 22:46:55, siva wrote:
> I try and avoid functions which have two return values one as a regular return
> value and the other being returned as a parameter. Maybe we could change this
to
> always return the code object and then we get the function from it.
> 
> RawCode* InlinedFunctionsInDartFrameIterator::GetNextCode(uword* pc);
> 
> What do you think?

I agree. I'll get rid of one output parameter and make the iterator return the
function object as before, and extract the code via func.unoptimized_code() from
it.

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.h
File runtime/vm/stack_frame.h (right):

https://codereview.chromium.org/12049039/diff/11001/runtime/vm/stack_frame.h#...
runtime/vm/stack_frame.h:242: GrowableArray<DeoptInstr*> deopt_instructions_;
On 2013/01/28 22:46:55, siva wrote:
> See comment about adding this array in stack_frame.cc

I simplified the code for the iterator a bit (see .cc file)

[regis, 2013-01-29 16:09:20]

Florian,

Do you mind fixing the arm and mips builds?

Thanks,
Regis