Preserve exception message in iterator finalization.

Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen, turbofan, and the interpreter to emit the ClearPendingMessage call
  only when the flag is set,
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n

Committed: https://crrev.com/f70b3d3b2c482e8a9918202e358c929852861e07
Cr-Commit-Position: refs/heads/master@{#35226}

[Yang, 2016-03-30 12:12:49]

Please add a test case in test/messages/ based on the one in the issue
description

https://codereview.chromium.org/1842953003/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1842953003/diff/1/src/ast/ast.h#newcode3151
src/ast/ast.h:3151: bool clear_pending_message, int pos) {
I'd prefer having an enum so the callsite clearly shows what argument this is.

[neis, 2016-03-30 12:59:42]

Patchset #3 (id:40001) has been deleted

[neis, 2016-03-30 12:59:47]

Patchset #2 (id:20001) has been deleted

[neis, 2016-03-30 13:05:50]

On 2016/03/30 12:12:49, Yang wrote:
> Please add a test case in test/messages/ based on the one in the issue
> description

Done.  I didn't know we had such tests, thanks!

> https://codereview.chromium.org/1842953003/diff/1/src/ast/ast.h
> File src/ast/ast.h (right):
> 
> https://codereview.chromium.org/1842953003/diff/1/src/ast/ast.h#newcode3151
> src/ast/ast.h:3151: bool clear_pending_message, int pos) {
> I'd prefer having an enum so the callsite clearly shows what argument this is.

I don't like having enums for booleans but I see your point.  Are you fine with
how I changed it in the new patchset?

[Yang, 2016-03-31 07:24:43]

On 2016/03/30 13:05:50, neis wrote:
> On 2016/03/30 12:12:49, Yang wrote:
> > Please add a test case in test/messages/ based on the one in the issue
> > description
> 
> Done.  I didn't know we had such tests, thanks!
> 
> > https://codereview.chromium.org/1842953003/diff/1/src/ast/ast.h
> > File src/ast/ast.h (right):
> > 
> > https://codereview.chromium.org/1842953003/diff/1/src/ast/ast.h#newcode3151
> > src/ast/ast.h:3151: bool clear_pending_message, int pos) {
> > I'd prefer having an enum so the callsite clearly shows what argument this
is.
> 
> I don't like having enums for booleans but I see your point.  Are you fine
with
> how I changed it in the new patchset?

LGTM. Thanks.

[neis, 2016-03-31 07:28:28]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-03-31 07:28:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842953003/60001

[commit-bot: I haz the power, 2016-03-31 07:35:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 07:35:03]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13030)

[neis, 2016-03-31 08:39:28]

Description was changed from

==========
Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen and the interpreter to emit the ClearPendingMessage call
  only when the flag is set (Turbofan is currently missing this call anyway),
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n
==========

to

==========
Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen, turbofan, and the interpreter to emit the
ClearPendingMessage call
  only when the flag is set,
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n
==========

[neis, 2016-03-31 08:39:53]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 08:39:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842953003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842953003/80001

[neis, 2016-03-31 08:47:12]

@littledan, please take a look at the parser changes.

[commit-bot: I haz the power, 2016-03-31 09:05:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 09:05:27]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-04-01 13:07:41]

LGTM with one nit.

https://codereview.chromium.org/1842953003/diff/80001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1842953003/diff/80001/src/ast/ast.h#newcode1185
src/ast/ast.h:1185: bool clear_pending_message() { return
clear_pending_message_; }
nit: I love the awesome comment below. Can we move the accessor to right below
the comment? That way it shows up in various GUI editors as being associated
with the accessor (improves readability and helps keeping it in sync).

[Dan Ehrenberg, 2016-04-01 14:42:58]

lgtm

Parser change LGTM.

Dumb question--could we somehow make this work for user code too, so we'd get
rethrows with better messages in like

try {
  ...
} catch (e) {
  if (!e instanceof Foo) throw e;
  ...
}

Hypothetically, maybe the message could be associated with the exception
somehow, or could be cleared at a different time. Or, do we want user-driven
rethrows to come from the rethrow location?

[Yang, 2016-04-01 14:47:30]

On 2016/04/01 14:42:58, Dan Ehrenberg wrote:
> lgtm
> 
> Parser change LGTM.
> 
> Dumb question--could we somehow make this work for user code too, so we'd get
> rethrows with better messages in like
> 
> try {
>   ...
> } catch (e) {
>   if (!e instanceof Foo) throw e;
>   ...
> }
> 
> Hypothetically, maybe the message could be associated with the exception
> somehow, or could be cleared at a different time. Or, do we want user-driven
> rethrows to come from the rethrow location?

This has been discussed several times over the past couple years already. There
is no general way to know whether a throw is a rethrow. So in user code every
throw is a new throw.

[neis, 2016-04-04 07:48:59]

The CQ bit was checked by neis@chromium.org

[neis, 2016-04-04 07:49:00]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1842953003/#ps80001
(title: "Turbofan")

[commit-bot: I haz the power, 2016-04-04 07:49:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842953003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842953003/80001

[commit-bot: I haz the power, 2016-04-04 08:15:06]

Description was changed from

==========
Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen, turbofan, and the interpreter to emit the
ClearPendingMessage call
  only when the flag is set,
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n
==========

to

==========
Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen, turbofan, and the interpreter to emit the
ClearPendingMessage call
  only when the flag is set,
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n
==========

[commit-bot: I haz the power, 2016-04-04 08:15:07]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-04-04 08:15:34]

Description was changed from

==========
Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen, turbofan, and the interpreter to emit the
ClearPendingMessage call
  only when the flag is set,
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n
==========

to

==========
Preserve exception message in iterator finalization.

The parser uses a try-catch in order to record when the client of an iterator
throws.  The exception then used to get rethrown via 'throw', which
unfortunately resulted in the original exception message object getting
overwritten.

This CL solves this as follows:
- add a clear_pending_message flag to TryCatchStatement (set to true in normal
  cases),
- set clear_pending_message to false for the TryCatchStatement used in iterator
  finalization
- change full-codegen, turbofan, and the interpreter to emit the
ClearPendingMessage call
  only when the flag is set,
- replace 'throw' with '%ReThrow' in the iterator finalization code, thus
  reusing the (not-cleared) pending message

R=littledan@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
BUG=v8:4875
LOG=n

Committed: https://crrev.com/f70b3d3b2c482e8a9918202e358c929852861e07
Cr-Commit-Position: refs/heads/master@{#35226}
==========

[commit-bot: I haz the power, 2016-04-04 08:15:35]

Patchset 3 (id:??) landed as
https://crrev.com/f70b3d3b2c482e8a9918202e358c929852861e07
Cr-Commit-Position: refs/heads/master@{#35226}