FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: v8 pointer compression is 64bit arch only by zcbenz · Pull Request #40418 · nodejs/node · GitHub

/ node Public

build: v8 pointer compression is 64bit arch only - #40418

Closed
zcbenz wants to merge 1 commit into
nodejs:masterfrom
zcbenz:no-pointer-compression-x86
ConversationCommits (1)ChecksFiles changed
Closed

build: v8 pointer compression is 64bit arch only#40418
zcbenz wants to merge 1 commit into
nodejs:masterfrom
zcbenz:no-pointer-compression-x86

Conversation

zcbenz commented Oct 12, 2021

Copy link
Copy Markdown
Contributor

The V8 pointer compression only works for 64bit arches, and the v8_enable_pointer_compression should be forced off when not building for them.

Why this is needed

Node.js only provides one headers tarball, and the same common.gypi and config.gypi file are used for building all architectures.

So for Node.js distributions that have v8_enable_pointer_compression=1, their headers tarball will set v8_enable_pointer_compression=1 in the config.gypi file, and building x86 native modules for those binaries will fail because v8_enable_pointer_compression=1 can not work for 32bit architectures.

The official Node.js distribution will also have this problem if it turned on V8 pointer compression in future.

Force turning off v8_enable_pointer_compression for non-64bit targets can fix it.

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Oct 12, 2021
zcbenz force-pushed the no-pointer-compression-x86 branch from ccecf9c to e888420 Compare October 12, 2021 04:01
zcbenz changed the title build: V8 pointer compression is 64bit arch only build: v8 pointer compression is 64bit arch only Oct 12, 2021
gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021

gengjiawen commented Oct 12, 2021

Copy link
Copy Markdown
Member

cc @nodejs/v8

gengjiawen added the v8 engine Issues and PRs related to the V8 dependency. label Oct 12, 2021

targos commented Oct 12, 2021

Copy link
Copy Markdown
Member

/cc @nodejs/platform-aix @nodejs/platform-ppc

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
sxa requested changes Oct 12, 2021
View reviewed changes

sxa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the group ping @targos! Yes the AIX/ppc64, Linux/ppc64le, Linux/s390x and the in development RISC-V port etc.are all also 64-bit and based on the information in the descirption this will result in the wrong behavior on those. Also since 32-bit is now the smaller set of plaforms (It's unlikely any new 32-bit ones will appear!) I'd strongly suggest that this is changed to an explicit list of the 64-bit ones as opposed to not in 64-bit ones to set the option off.

Do we have any tests that show this failure at present?

zcbenz force-pushed the no-pointer-compression-x86 branch from e888420 to dbddd86 Compare October 12, 2021 09:34

richardlau commented Oct 12, 2021

Copy link
Copy Markdown
Member

Do we have any tests that show this failure at present?

@sxa we do not build with pointer compression enabled anywhere except the unofficial builds (https://github.com/nodejs/unofficial-builds/tree/master/recipes/x64-pointer-compression) and even there we do not run any tests.

zcbenz commented Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the review, I have updated the code to test 32bit arches instead.

There is no test for this in Node.js, as compiling with v8_enable_pointer_compression=1 on 32bit arches will result in compilation error:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/common/globals.h;l=304-307

I think this is something that can be tested in node-gyp instead since it is about building native modules.

sxa approved these changes Oct 12, 2021
View reviewed changes

sxa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the updates. Assuming that architecture list is correct (do we really have ia32 and x32 in different places) I'm ok with this change.

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
Mesteery added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021

nodejs-github-bot commented Oct 12, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40254/

Mesteery removed the needs-ci PRs that need a full CI run. label Oct 12, 2021
github-actions Bot mentioned this pull request Oct 13, 2021
Open
22 tasks
mhdawson approved these changes Oct 13, 2021
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson commented Oct 13, 2021

Copy link
Copy Markdown
Member

Create new issues for 2 flaky test failures and kicked off resume

nodejs-github-bot commented Oct 13, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40291/

Mesteery added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 13, 2021
github-actions Bot mentioned this pull request Oct 14, 2021
Open
22 tasks
This was referenced Oct 15, 2021
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Oct 23, 2021
build: disable v8 pointer compression on 32bit archs
a536de4 
PR-URL: #40418
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>

targos commented Oct 23, 2021

Copy link
Copy Markdown
Member

Landed in a536de4

targos closed this Oct 23, 2021
targos pushed a commit that referenced this pull request Oct 23, 2021
build: disable v8 pointer compression on 32bit archs
9f46fca 
PR-URL: #40418
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos mentioned this pull request Nov 8, 2021
Merged
BethGriggs pushed a commit that referenced this pull request Nov 24, 2021
build: disable v8 pointer compression on 32bit archs
4581997 
PR-URL: #40418
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
BethGriggs mentioned this pull request Nov 26, 2021
Merged
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen approved these changes
gengjiawen gengjiawen approved these changes
JungMinu JungMinu approved these changes
sxa sxa approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL