Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

deps: Fix V8 build in 32bit SmartOS with GCC 4.9 #25556

Closed
wants to merge 2 commits into from
Closed

deps: Fix V8 build in 32bit SmartOS with GCC 4.9 #25556

wants to merge 2 commits into from

Conversation

joaocgreis
Copy link
Member

Currently in 32bit SmartOS, node compiles and runs well with GCC 4.7 but segfaults immediately when compiled with GCC 4.9.

This fix is a cherry pick of 3 commits from V8:

  1. https://chromium.googlesource.com/external/v8.git/+/4e670fd05e439145b3f46dd2c54d332044a72954
    (only the variable rename in NonAsciiStart)
  2. https://chromium.googlesource.com/external/v8.git/+/90dc5c9e66ef1778cb8425c43c2f37b6799260c3
  3. https://chromium.googlesource.com/external/v8.git/+/7cb82a76b46ef18f8e05d40d4fe13b3410d446e4

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

These 3 commits are separate in this PR for easier review, but will be squashed if accepted.

Fixes #25281

@joaocgreis
Copy link
Member Author

@joyent/node-collaborators PTAL, thanks!

@misterdjules misterdjules added this to the 0.12.6 milestone Jun 23, 2015
@misterdjules
Copy link

Adding as a P-1 for the same reasons as highlighted previously in the issue it fixes.

@misterdjules
Copy link

@joaocgreis I would suggest changing the commit messages to make them clearer.

More specifically, I would follow the guidelines about cherry-picking changes from V8 and include the original commit message in each commit, like in the following commit: 2fc5eeb. Please note that the PR-URL and Reviewed-By fields are added by the Jenkins job, so you don't have to add them to the commit message.

I like how you mentioned the other commits' sha though in each of the three commits, so I would keep that but I would make it more concise.

Other than that the changes look good, but before approving these changes I would like to know from @alhazred if these changes fix his problems (discussion is ongoing in github.com//issues/25281).

Anyway, great work @joaocgreis 👍 and thank you!

@misterdjules
Copy link

@joaocgreis #25281 (comment) confirms that your changes fix the problem for the original reporter, so once the commit messages are fixed, LGTM.

Also, I would not squash these commits, as it's easier to identify them when we have to investigate issues if they're as close to the original changes as possible.

If you need help landing these changes, please ping me on #libuv :)

@misterdjules
Copy link

@joaocgreis When this lands, you'll also need to update the list of V8 floating patches for v0.12.

@joaocgreis
Copy link
Member Author

@misterdjules The first commit is only a variable rename needed only for the third commit to merge cleanly. It's huge, 2,221 additions and 2,548 deletions, and does not merge cleanly. Should I:

  1. Pick it partially as I did above
  2. Rename the variable with a commit of my own
  3. Not pick it and solve the conflicts when picking commit 3

Which one do you think is the best option, considering this needs to be added as a floating patch?

@misterdjules
Copy link

@joaocgreis I would choose 3) because solving the conflicts doesn't change the commit enough such as it becomes too different from the original one, and it avoids us from floating one more patch.

@joaocgreis
Copy link
Member Author

Updated. Let me know if the commit messages are what you had in mind, @misterdjules . Ok to land?

@misterdjules
Copy link

@joaocgreis I would change the title of each commit to really describe what they do, so the first one would be something like V8: remove V8_HOST_CAN_READ_UNALIGNED, and the second would be V8: do not use wide reads in CopyCharsUnsigned. Other than that LGTM, thank you very much! 👍

Fixes segfault in 32bit SmartOS when built with GCC 4.9.

This is the first of two backports from upstream v8:
1. v8/v8@90dc5c9
2. v8/v8@7cb82a7

Original commit message:

  Do not use wide reads in CopyCharsUnsigned.

  R=jkummerow@chromium.org
  BUG=chromium:412967
  LOG=Y

  Review URL: https://codereview.chromium.org/566583002

  git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23876 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

Fixes #25281
Fixes segfault in 32bit SmartOS when built with GCC 4.9.

This is the second of two backports from upstream v8:
1. v8/v8@90dc5c9
2. v8/v8@7cb82a7

Original commit message:

  Reland "Remove V8_HOST_CAN_READ_UNALIGNED and its uses."

  BUG=chromium:412967
  LOG=N
  R=jkummerow@chromium.org

  Review URL: https://codereview.chromium.org/571903002

  git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23938 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

Fixes #25281
joaocgreis pushed a commit that referenced this pull request Jul 1, 2015
Fixes segfault in 32bit SmartOS when built with GCC 4.9.

This is the first of two backports from upstream v8:
1. v8/v8@90dc5c9
2. v8/v8@7cb82a7

Original commit message:

  Do not use wide reads in CopyCharsUnsigned.

  R=jkummerow@chromium.org
  BUG=chromium:412967
  LOG=Y

  Review URL: https://codereview.chromium.org/566583002

  git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23876 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

Fixes #25281

Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
PR-URL: #25556
joaocgreis pushed a commit that referenced this pull request Jul 1, 2015
Fixes segfault in 32bit SmartOS when built with GCC 4.9.

This is the second of two backports from upstream v8:
1. v8/v8@90dc5c9
2. v8/v8@7cb82a7

Original commit message:

  Reland "Remove V8_HOST_CAN_READ_UNALIGNED and its uses."

  BUG=chromium:412967
  LOG=N
  R=jkummerow@chromium.org

  Review URL: https://codereview.chromium.org/571903002

  git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23938 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

Fixes #25281

Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
PR-URL: #25556
@joaocgreis
Copy link
Member Author

Landed in 48b0ca2 and 13ea50e

Also added the commits to the list of floating patches.

@misterdjules
Copy link

@joaocgreis 👍 Thank you!

@misterdjules misterdjules modified the milestones: 0.12.6, 0.12.7 Jul 6, 2015
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
Fixes segfault in 32bit SmartOS when built with GCC 4.9.

This is the first of two backports from upstream v8:
1. v8/v8@90dc5c9
2. v8/v8@7cb82a7

Original commit message:

  Do not use wide reads in CopyCharsUnsigned.

  R=jkummerow@chromium.org
  BUG=chromium:412967
  LOG=Y

  Review URL: https://codereview.chromium.org/566583002

  git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23876 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

Fixes nodejs#25281

Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
PR-URL: nodejs#25556
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
Fixes segfault in 32bit SmartOS when built with GCC 4.9.

This is the second of two backports from upstream v8:
1. v8/v8@90dc5c9
2. v8/v8@7cb82a7

Original commit message:

  Reland "Remove V8_HOST_CAN_READ_UNALIGNED and its uses."

  BUG=chromium:412967
  LOG=N
  R=jkummerow@chromium.org

  Review URL: https://codereview.chromium.org/571903002

  git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23938 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

V8 issue: https://code.google.com/p/chromium/issues/detail?id=412967

Fixes nodejs#25281

Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
PR-URL: nodejs#25556
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants