Issue 10963027: Use the inner deoptimization frame when setting incoming arguments.

Issue 10963027: Use the inner deoptimization frame when setting incoming arguments. (Closed)

Created:
8 years, 3 months ago by zerny-google

Modified:
8 years, 3 months ago

Reviewers:
Vyacheslav Egorov (Google)

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Use the inner deoptimization frame when setting incoming arguments. This fixes issue 5318 where deoptimization of an inlined call resulted in an incorrect value being set in the deoptimized frames. R=vegorov@google.com BUG=5318 Committed: https://code.google.com/p/dart/source/detail?r=12715

Patch Set 1 #

Patch Set 2 : Enabled inlining #

Total comments: 4

Patch Set 3 : Added test #

Patch Set 4 : Names #

Total comments: 2

Patch Set 5 : Review update #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+82 lines, -25 lines)			Patch
M	runtime/vm/compiler.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	runtime/vm/flow_graph_compiler.cc	View	1 2 3 4	1 chunk	+47 lines, -24 lines	0 comments	Download
A	tests/language/deopt_inlined_function_test.dart	View	1 2	1 chunk	+34 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Google)

lgtm https://codereview.chromium.org/10963027/diff/3001/runtime/vm/flow_graph_compiler.cc File runtime/vm/flow_graph_compiler.cc (right): https://codereview.chromium.org/10963027/diff/3001/runtime/vm/flow_graph_compiler.cc#newcode102 runtime/vm/flow_graph_compiler.cc:102: // Set incoming arguments from the inner frame. ...

8 years, 3 months ago (2012-09-21 14:38:30 UTC) #2

zerny-google

8 years, 3 months ago (2012-09-21 14:55:47 UTC) #3

Thanks for the comments. Updated and dcommit'ed.

https://codereview.chromium.org/10963027/diff/3001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/10963027/diff/3001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler.cc:102: // Set incoming arguments from the inner
frame.
On 2012/09/21 14:38:30, Vyacheslav Egorov (Google) wrote:
> Please add a comment that clarifies that values of incoming arguments can be
> changed by the inner function and thus they have to be taken from the inner
> frame.

Done.

https://codereview.chromium.org/10963027/diff/3001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler.cc:107: // Set the locals, not including the
outgoing arguments.
On 2012/09/21 14:38:30, Vyacheslav Egorov (Google) wrote:
> I would add an assertion here that outer->Length() is bigger than
> inner->fixed_parameter_count().

Done.

https://codereview.chromium.org/10963027/diff/1002/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/10963027/diff/1002/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler.cc:124: ASSERT(inner != NULL);  // The inner
pointer is not the outermost environment.
On 2012/09/21 14:38:30, Vyacheslav Egorov (Google) wrote:
> actually at this point is has to be the outermost environment.

Typo. That should have been "is now the outermost".

Expand Messages | Collapse Messages