New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: backport 0353a1e from V8 upstream #15287

Closed
wants to merge 1 commit into
base: v6.x-staging
from

Conversation

Projects
None yet
8 participants
@jBarz
Contributor

jBarz commented Sep 8, 2017

Original commit message:

Avoid disassembling Interpreted Regexp code

I found that v8 will crash when --print-code is turned on while Regexp
is interpreted. It crashes when trying to print Relocation info during
Disassembly. It should probably avoid printing out disassembly when the
Code object is a bytecode regexp.

Bug:
Change-Id: I35b531cb03996a303248652871452266c78fee38
Reviewed-on: https://chromium-review.googlesource.com/642127
Reviewed-by: Yang Guo yangguo@chromium.org

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
deps: backport 0353a1e from V8 upstream
Original commit message:

  Avoid disassembling Interpreted Regexp code

  I found that v8 will crash when --print-code is turned on while Regexp
  is interpreted. It crashes when trying to print Relocation info during
  Disassembly. It should probably avoid printing out disassembly when the
  Code object is a bytecode regexp.

  Bug:
  Change-Id: I35b531cb03996a303248652871452266c78fee38
  Reviewed-on: https://chromium-review.googlesource.com/642127
  Reviewed-by: Yang Guo <yangguo@chromium.org>
@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 13, 2017

Member

@jBarz was it meant to backport this to 6.x and not to open a PR against master?

Member

BridgeAR commented Sep 13, 2017

@jBarz was it meant to backport this to 6.x and not to open a PR against master?

@jBarz

This comment has been minimized.

Show comment
Hide comment
@jBarz

jBarz Sep 13, 2017

Contributor

I meant to backport to v6.x only.

Contributor

jBarz commented Sep 13, 2017

I meant to backport to v6.x only.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 13, 2017

Member

@jBarz is this not relevant for 8.x and above or is it already included there?

Member

BridgeAR commented Sep 13, 2017

@jBarz is this not relevant for 8.x and above or is it already included there?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 13, 2017

Member

It is not included in 8.x and above.

Member

targos commented Sep 13, 2017

It is not included in 8.x and above.

@jBarz

This comment has been minimized.

Show comment
Hide comment
@jBarz

jBarz Sep 13, 2017

Contributor

Oh right, it is relevant for v8.x and above.
I can submit a v8.x backport PR.
Won't master eventually get this change though? Because it keeps up with the latest v8?

Contributor

jBarz commented Sep 13, 2017

Oh right, it is relevant for v8.x and above.
I can submit a v8.x backport PR.
Won't master eventually get this change though? Because it keeps up with the latest v8?

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 13, 2017

Member

@jBarz this will not land on 8.x without a explicit backport. And also master is not kept to the latest v8 version. Newer versions are included in semver-major versions. Master is limited to 6.1 right now because it refers to v.9.0.0-pre.

Member

BridgeAR commented Sep 13, 2017

@jBarz this will not land on 8.x without a explicit backport. And also master is not kept to the latest v8 version. Newer versions are included in semver-major versions. Master is limited to 6.1 right now because it refers to v.9.0.0-pre.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 13, 2017

Member

We can do a merge request on V8 side for V8 6.2 and 6.1 (master has 6.1) and backport to v8.x

Member

targos commented Sep 13, 2017

We can do a merge request on V8 side for V8 6.2 and 6.1 (master has 6.1) and backport to v8.x

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 16, 2017

Member

Did somebody open the merge request on the V8 side already?

Member

fhinkel commented Sep 16, 2017

Did somebody open the merge request on the V8 side already?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 16, 2017

Member

I didn't

Member

targos commented Sep 16, 2017

I didn't

@jBarz

This comment has been minimized.

Show comment
Hide comment
@jBarz

jBarz Sep 16, 2017

Contributor

Sorry, will create the merge request on the V8 side today.

Contributor

jBarz commented Sep 16, 2017

Sorry, will create the merge request on the V8 side today.

@targos

This comment has been minimized.

Show comment
Hide comment
Member

targos commented Sep 18, 2017

MylesBorins added a commit that referenced this pull request Sep 19, 2017

deps: backport 0353a1e from V8 upstream
Original commit message:

  Avoid disassembling Interpreted Regexp code

  I found that v8 will crash when --print-code is turned on while Regexp
  is interpreted. It crashes when trying to print Relocation info during
  Disassembly. It should probably avoid printing out disassembly when the
  Code object is a bytecode regexp.

  Bug:
  Change-Id: I35b531cb03996a303248652871452266c78fee38
  Reviewed-on: https://chromium-review.googlesource.com/642127
  Reviewed-by: Yang Guo <yangguo@chromium.org>

PR-URL: #15287
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 19, 2017

Member

quick V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/915/

@addaleax do you want to float this on your 6.1 PR until it is updated upstream?

Member

MylesBorins commented Sep 19, 2017

quick V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/915/

@addaleax do you want to float this on your 6.1 PR until it is updated upstream?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 19, 2017

Member

@MylesBorins sorry, which PR? If you’re talking about #15393 that’s @targos’s PR, and I don’t really know how to keep track of what needs to lands in which branch :)

Member

addaleax commented Sep 19, 2017

@MylesBorins sorry, which PR? If you’re talking about #15393 that’s @targos’s PR, and I don’t really know how to keep track of what needs to lands in which branch :)

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 19, 2017

Member

landed in 4e1a50a

Member

MylesBorins commented Sep 19, 2017

landed in 4e1a50a

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

@targos targos referenced this pull request Sep 25, 2017

Merged

deps: cherry-pick 0353a1e from upstream V8 #15599

2 of 2 tasks complete
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 25, 2017

Member

The merge was accepted for 6.2 but rejected for 6.1. Cherry-pick: #15599

Member

targos commented Sep 25, 2017

The merge was accepted for 6.2 but rejected for 6.1. Cherry-pick: #15599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment