Add crash instrumentation for crbug.com/621147

Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=ishell@chromium.org,cbruni@chromium.org

Committed: https://crrev.com/5ff508a82299f20a0d9828cf73072a4f4772fab8
Cr-Commit-Position: refs/heads/master@{#37328}

[Michael Lippautz, 2016-06-27 20:46:23]

Description was changed from

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
==========

to

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=verwaest@chromium.org,cbruni@chromium.org
==========

[Michael Lippautz, 2016-06-27 20:46:23]

mlippautz@chromium.org changed reviewers:
+ cbruni@chromium.org, verwaest@chromium.org

[Michael Lippautz, 2016-06-27 20:48:07]

ptal, any further suggestions?

[adamk, 2016-06-27 22:43:56]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-06-27 22:43:56]

Just a heads-up, if we want this in M52 it needs to land before 5PM Pacific time
on Tuesday.

[sshruthi, 2016-06-27 23:11:27]

On 2016/06/27 22:43:56, adamk wrote:
> Just a heads-up, if we want this in M52 it needs to land before 5PM Pacific
time
> on Tuesday.

To be clear, it needs to be merged into M52 before 5PM PST on Tuesday. Ideally,
this should be in tonight's canary so we have more confidence in the change
before merging it in. It seems very late for the author and two of the
reviewers, and close to EOD in Pacific time as well, but any chance this can be
landed before tonight's canary cut (8PM PST)?

[adamk, 2016-06-27 23:20:52]

On 2016/06/27 23:11:27, sshruthi wrote:
> On 2016/06/27 22:43:56, adamk wrote:
> > Just a heads-up, if we want this in M52 it needs to land before 5PM Pacific
> time
> > on Tuesday.
> 
> To be clear, it needs to be merged into M52 before 5PM PST on Tuesday.
Ideally,
> this should be in tonight's canary so we have more confidence in the change
> before merging it in. It seems very late for the author and two of the
> reviewers, and close to EOD in Pacific time as well, but any chance this can
be
> landed before tonight's canary cut (8PM PST)?

I don't feel particularly comfortable reviewing this (nor do I have OWNERS in
the proper directory), so I don't think landing this today is much safer than
waiting for it to be properly reviewed in Munich and landed there.

[sshruthi1, 2016-06-28 03:08:31]

On 2016/06/27 23:20:52, adamk wrote:
> On 2016/06/27 23:11:27, sshruthi wrote:
> > On 2016/06/27 22:43:56, adamk wrote:
> > > Just a heads-up, if we want this in M52 it needs to land before 5PM
Pacific
> > time
> > > on Tuesday.
> > 
> > To be clear, it needs to be merged into M52 before 5PM PST on Tuesday.
> Ideally,
> > this should be in tonight's canary so we have more confidence in the change
> > before merging it in. It seems very late for the author and two of the
> > reviewers, and close to EOD in Pacific time as well, but any chance this can
> be
> > landed before tonight's canary cut (8PM PST)?
> 
> I don't feel particularly comfortable reviewing this (nor do I have OWNERS in
> the proper directory), so I don't think landing this today is much safer than
> waiting for it to be properly reviewed in Munich and landed there.

Sure, if you don't feel comfortable, then let's definitely wait. Once this
lands, please work with govind@ to figure out what's the safest way to get this
into the beta build for the week. We should definitely get this into this week's
beta build, if it is safe and everything looks good on canary.

[Igor Sheludko, 2016-06-28 07:07:19]

ishell@chromium.org changed reviewers:
+ ishell@chromium.org

[Igor Sheludko, 2016-06-28 07:07:20]

https://codereview.chromium.org/2100313002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2100313002/diff/20001/src/ic/ic.cc#newcode2542
src/ic/ic.cc:2542: uintptr_t left_raw =
HeapObject::cast(*left)->map_word().ToRawValue();
If left is a broken pointer it will already crash in attempt to load a map. We
should compare the lower part of the left value with the hole value.

[Michael Lippautz, 2016-06-28 08:39:33]

https://codereview.chromium.org/2100313002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2100313002/diff/20001/src/ic/ic.cc#newcode2542
src/ic/ic.cc:2542: uintptr_t left_raw =
HeapObject::cast(*left)->map_word().ToRawValue();
On 2016/06/28 07:07:20, Igor Sheludko wrote:
> If left is a broken pointer it will already crash in attempt to load a map. We
> should compare the lower part of the left value with the hole value.

Oops, thought that it was the map. Reading the values now.

[Igor Sheludko, 2016-06-28 09:06:28]

https://codereview.chromium.org/2100313002/diff/40001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2100313002/diff/40001/src/ic/ic.cc#newcode2552
src/ic/ic.cc:2552: isolate()->PushStackTraceAndDie(0xBAAAAAAD, pc(), fp(), 0u);
I have a fear that the instructions array could be eliminated in release build
since it is not used. I would put the fp() value to the beginning of the
instructions buffer and pass the buffer pointer to the PushStackTraceAndDie
instead of fp.

[Michael Lippautz, 2016-06-28 09:45:13]

https://codereview.chromium.org/2100313002/diff/40001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2100313002/diff/40001/src/ic/ic.cc#newcode2552
src/ic/ic.cc:2552: isolate()->PushStackTraceAndDie(0xBAAAAAAD, pc(), fp(), 0u);
On 2016/06/28 09:06:28, Igor Sheludko wrote:
> I have a fear that the instructions array could be eliminated in release build
> since it is not used. I would put the fp() value to the beginning of the
> instructions buffer and pass the buffer pointer to the PushStackTraceAndDie
> instead of fp.

Done.

[Igor Sheludko, 2016-06-28 09:46:55]

lgtm from my side. 
Maybe Camillo will add comments.

[Michael Lippautz, 2016-06-28 10:37:46]

Description was changed from

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=verwaest@chromium.org,cbruni@chromium.org
==========

to

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=ishell@chromium.org,cbruni@chromium.org
==========

[Camillo Bruni, 2016-06-28 11:49:34]

On 2016/06/28 at 09:46:55, ishell wrote:
> lgtm from my side. 
> Maybe Camillo will add comments.

Just tried out locally and seems to be fine...

[Camillo Bruni, 2016-06-28 11:49:40]

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

[commit-bot: I haz the power, 2016-06-28 11:49:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 11:57:41]

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

[commit-bot: I haz the power, 2016-06-28 11:57:43]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/9618)

[Michael Lippautz, 2016-06-28 12:22:04]

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

[commit-bot: I haz the power, 2016-06-28 12:22:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 12:48:28]

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

[commit-bot: I haz the power, 2016-06-28 12:48:32]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2016-06-28 12:49:22]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-06-28 12:49:22]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2100313002/#ps80001
(title: "ul -> ull")

[commit-bot: I haz the power, 2016-06-28 12:49:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 12:51:32]

Description was changed from

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=ishell@chromium.org,cbruni@chromium.org
==========

to

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=ishell@chromium.org,cbruni@chromium.org
==========

[commit-bot: I haz the power, 2016-06-28 12:51:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-28 12:54:45]

Description was changed from

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=ishell@chromium.org,cbruni@chromium.org
==========

to

==========
Add crash instrumentation for crbug.com/621147

BUG=chromium:621147
LOG=N
R=ishell@chromium.org,cbruni@chromium.org

Committed: https://crrev.com/5ff508a82299f20a0d9828cf73072a4f4772fab8
Cr-Commit-Position: refs/heads/master@{#37328}
==========

[commit-bot: I haz the power, 2016-06-28 12:54:46]

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

[adamk, 2016-06-30 16:33:26]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2118493002/ by adamk@chromium.org.

The reason for reverting is: Instrumentation not needed on master branch.