[interpreter] Fix source positions for exception messages.

[interpreter] Fix source positions for exception messages.

This adds more source positions for expressions to generated bytecode,
ensuring that exception messages are in sync with full-codegen code.

R=yangguo@chromium.org
BUG=v8:4689
LOG=n

[Michael Starzinger, 2016-03-11 09:04:20]

mstarzinger@chromium.org changed reviewers:
+ mythria@chromium.org

[Michael Starzinger, 2016-03-11 09:04:21]

Hey Yang, as discussed offline: This fixes the position information in the
exception messages. Unfortunately it makes a whole slow of debugger tests fail.
I might need you help getting to the bottom of this. Is the approach in general
close to what you had in mind for the expression positions or am I completely
off here?

[rmcilroy, 2016-03-11 10:24:39]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org, vogelheim@chromium.org

[rmcilroy, 2016-03-11 10:24:40]

On 2016/03/11 09:04:21, Michael Starzinger wrote:
> Hey Yang, as discussed offline: This fixes the position information in the
> exception messages. Unfortunately it makes a whole slow of debugger tests
fail.
> I might need you help getting to the bottom of this. Is the approach in
general
> close to what you had in mind for the expression positions or am I completely
> off here?

This might be similar to something Daniel was looking into yesterday - we
currently assume that there will always be at least one bytecode between
different source positions, however certain optimizations we are doing (e.g.,
omitting the Ldar or Star bytecodes if we know the register is already the same
as the accumulator) mean that we might not necessarily emit bytecode between two
source positions.

I'm not sure what we can do about this, do you have any thoughts Yang? We really
want to keep these optimizations, and actually add more of them next quarter,
since redundant Ldar and Star instructions cost memory and a fair chunk of
performance.

[Yang, 2016-03-11 10:45:05]

On 2016/03/11 10:24:40, rmcilroy wrote:
> On 2016/03/11 09:04:21, Michael Starzinger wrote:
> > Hey Yang, as discussed offline: This fixes the position information in the
> > exception messages. Unfortunately it makes a whole slow of debugger tests
> fail.
> > I might need you help getting to the bottom of this. Is the approach in
> general
> > close to what you had in mind for the expression positions or am I
completely
> > off here?
> 
> This might be similar to something Daniel was looking into yesterday - we
> currently assume that there will always be at least one bytecode between
> different source positions, however certain optimizations we are doing (e.g.,
> omitting the Ldar or Star bytecodes if we know the register is already the
same
> as the accumulator) mean that we might not necessarily emit bytecode between
two
> source positions.
> 
> I'm not sure what we can do about this, do you have any thoughts Yang? We
really
> want to keep these optimizations, and actually add more of them next quarter,
> since redundant Ldar and Star instructions cost memory and a fair chunk of
> performance.

I'm currently changing the way we break at return. We used to break before and
after the evaluation of the returned expression. That does not map to the
interpreter if we only emit one bytecode for both, like you describe. I'm
changing FCG and tests to only after evaluation of the expression but before the
return.

We might need to do something similar for throw as well.

[rmcilroy, 2016-03-11 11:10:33]

On 2016/03/11 10:45:05, Yang wrote:
> On 2016/03/11 10:24:40, rmcilroy wrote:
> > On 2016/03/11 09:04:21, Michael Starzinger wrote:
> > > Hey Yang, as discussed offline: This fixes the position information in the
> > > exception messages. Unfortunately it makes a whole slow of debugger tests
> > fail.
> > > I might need you help getting to the bottom of this. Is the approach in
> > general
> > > close to what you had in mind for the expression positions or am I
> completely
> > > off here?
> > 
> > This might be similar to something Daniel was looking into yesterday - we
> > currently assume that there will always be at least one bytecode between
> > different source positions, however certain optimizations we are doing
(e.g.,
> > omitting the Ldar or Star bytecodes if we know the register is already the
> same
> > as the accumulator) mean that we might not necessarily emit bytecode between
> two
> > source positions.
> > 
> > I'm not sure what we can do about this, do you have any thoughts Yang? We
> really
> > want to keep these optimizations, and actually add more of them next
quarter,
> > since redundant Ldar and Star instructions cost memory and a fair chunk of
> > performance.
> 
> I'm currently changing the way we break at return. We used to break before and
> after the evaluation of the returned expression. That does not map to the
> interpreter if we only emit one bytecode for both, like you describe. I'm
> changing FCG and tests to only after evaluation of the expression but before
the
> return.
> 
> We might need to do something similar for throw as well.

My point was that with Michi's change we might end up with overlapping source
positions anywhere in the function, not just at return or throw (e.g., if the
Ldar for a VisitVariableProxy is omitted since the local is already in the
accumulator, then the VisitVariableProxy source position might be at the same
bytecode offset as the subsequent source position, e.g., the expression position
for a property store). Is this an issue for the debugger?

[Yang, 2016-03-11 11:46:40]

On 2016/03/11 11:10:33, rmcilroy wrote:
> On 2016/03/11 10:45:05, Yang wrote:
> > On 2016/03/11 10:24:40, rmcilroy wrote:
> > > On 2016/03/11 09:04:21, Michael Starzinger wrote:
> > > > Hey Yang, as discussed offline: This fixes the position information in
the
> > > > exception messages. Unfortunately it makes a whole slow of debugger
tests
> > > fail.
> > > > I might need you help getting to the bottom of this. Is the approach in
> > > general
> > > > close to what you had in mind for the expression positions or am I
> > completely
> > > > off here?
> > > 
> > > This might be similar to something Daniel was looking into yesterday - we
> > > currently assume that there will always be at least one bytecode between
> > > different source positions, however certain optimizations we are doing
> (e.g.,
> > > omitting the Ldar or Star bytecodes if we know the register is already the
> > same
> > > as the accumulator) mean that we might not necessarily emit bytecode
between
> > two
> > > source positions.
> > > 
> > > I'm not sure what we can do about this, do you have any thoughts Yang? We
> > really
> > > want to keep these optimizations, and actually add more of them next
> quarter,
> > > since redundant Ldar and Star instructions cost memory and a fair chunk of
> > > performance.
> > 
> > I'm currently changing the way we break at return. We used to break before
and
> > after the evaluation of the returned expression. That does not map to the
> > interpreter if we only emit one bytecode for both, like you describe. I'm
> > changing FCG and tests to only after evaluation of the expression but before
> the
> > return.
> > 
> > We might need to do something similar for throw as well.
> 
> My point was that with Michi's change we might end up with overlapping source
> positions anywhere in the function, not just at return or throw (e.g., if the
> Ldar for a VisitVariableProxy is omitted since the local is already in the
> accumulator, then the VisitVariableProxy source position might be at the same
> bytecode offset as the subsequent source position, e.g., the expression
position
> for a property store). Is this an issue for the debugger?

That might indeed be an issue for the debugger. Generally the debugger assumes
that each break location can be mapped to one bytecode position. If two
statements map to the same bytecode position, we cannot break twice.

[Michael Starzinger, 2016-04-04 12:06:50]

Closing. Superseded by: https://codereview.chromium.org/1855913002/

[Michael Starzinger, 2016-04-04 12:07:01]

Description was changed from

==========
[interpreter] Fix source positions for exception messages.

This adds more source positions for expressions to generated bytecode,
ensuring that exception messages are in sync with full-codegen code.

R=yangguo@chromium.org
BUG=v8:4689
LOG=n
==========

to

==========
[interpreter] Fix source positions for exception messages.

This adds more source positions for expressions to generated bytecode,
ensuring that exception messages are in sync with full-codegen code.

R=yangguo@chromium.org
BUG=v8:4689
LOG=n
==========