-
Notifications
You must be signed in to change notification settings - Fork 7.3k
deps: Fix V8 build in 32bit SmartOS with GCC 4.9 #25556
Conversation
@joyent/node-collaborators PTAL, thanks! |
Adding as a P-1 for the same reasons as highlighted previously in the issue it fixes. |
@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 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! |
@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 :) |
@joaocgreis When this lands, you'll also need to update the list of V8 floating patches for v0.12. |
@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:
Which one do you think is the best option, considering this needs to be added as a floating patch? |
@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. |
Updated. Let me know if the commit messages are what you had in mind, @misterdjules . Ok to land? |
@joaocgreis I would change the title of each commit to really describe what they do, so the first one would be something like |
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
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
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 👍 Thank you! |
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
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
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:
(only the variable rename in NonAsciiStart)
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