Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Oct 12, 2021

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 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 zcbenz changed the title build: V8 pointer compression is 64bit arch only build: v8 pointer compression is 64bit arch only Oct 12, 2021
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
@gengjiawen
Copy link
Member

cc @nodejs/v8

@gengjiawen gengjiawen added the v8 engine Issues and PRs related to the V8 dependency. label Oct 12, 2021
@targos
Copy link
Member

targos commented Oct 12, 2021

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

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?

@richardlau
Copy link
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
Copy link
Contributor Author

zcbenz commented Oct 12, 2021

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.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

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 gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
@Mesteery Mesteery added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
@github-actions 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
Copy link
Collaborator

@Mesteery Mesteery removed the needs-ci PRs that need a full CI run. label Oct 12, 2021
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2021
targos pushed a commit that referenced this pull request Oct 23, 2021
PR-URL: #40418
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos
Copy link
Member

targos commented Oct 23, 2021

Landed in a536de4

@targos targos closed this Oct 23, 2021
targos pushed a commit that referenced this pull request Oct 23, 2021
PR-URL: #40418
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Nov 8, 2021
BethGriggs pushed a commit that referenced this pull request Nov 24, 2021
PR-URL: #40418
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

10 participants