GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — build: v8 pointer compression is 64bit arch only by zcbenz · Pull Request #40418 · nodejs/node

Conversation

@zcbenz

@zcbenz 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Oct 12, 2021
@zcbenz
zcbenz force-pushed the no-pointer-compression-x86 branch from ccecf9c to e888420 Compare October 12, 2021 04:01
@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 approved these changes Oct 12, 2021
View reviewed changes
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 12, 2021
@gengjiawen

gengjiawen commented Oct 12, 2021

Copy link
Copy Markdown
Member

cc @nodejs/v8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gengjiawen gengjiawen added the v8 engine Issues and PRs related to the V8 dependency. label Oct 12, 2021
@targos

targos commented Oct 12, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 12, 2021
sxa
sxa requested changes Oct 12, 2021
View reviewed changes

@sxa sxa left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@zcbenz
build: v8 pointer compression is 64bit arch only
dbddd86
@zcbenz
zcbenz force-pushed the no-pointer-compression-x86 branch from e888420 to dbddd86 Compare October 12, 2021 09:34
@richardlau

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zcbenz

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sxa
sxa approved these changes Oct 12, 2021
View reviewed changes

@sxa sxa left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 12, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 12, 2021
tniessen
tniessen approved these changes Oct 12, 2021
View reviewed changes
@Mesteery Mesteery added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 12, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 12, 2021
@nodejs-github-bot

nodejs-github-bot commented Oct 12, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mesteery Mesteery removed the needs-ci PRs that need a full CI run. label Oct 12, 2021
@github-actions github-actions Bot mentioned this pull request Oct 13, 2021
CI Reliability 2021-10-13 nodejs/reliability#85
Open
22 tasks
JungMinu
JungMinu approved these changes Oct 13, 2021
View reviewed changes
mhdawson
mhdawson approved these changes Oct 13, 2021
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson

mhdawson commented Oct 13, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 13, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mesteery Mesteery added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 13, 2021
@github-actions github-actions Bot mentioned this pull request Oct 14, 2021
CI Reliability 2021-10-14 nodejs/reliability#86
Open
22 tasks
This was referenced Oct 15, 2021
CI Reliability 2021-10-15 nodejs/reliability#87
Open
CI Reliability 2021-10-16 nodejs/reliability#88
Open
CI Reliability 2021-10-17 nodejs/reliability#89
Open
CI Reliability 2021-10-18 nodejs/reliability#90
Open
targos pushed a commit that referenced this pull request Oct 23, 2021
@zcbenz @targos
build: disable v8 pointer compression on 32bit archs
a536de4 
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 commented Oct 23, 2021

Copy link
Copy Markdown
Member

Landed in a536de4
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos closed this Oct 23, 2021
targos pushed a commit that referenced this pull request Oct 23, 2021
@zcbenz @targos
build: disable v8 pointer compression on 32bit archs
9f46fca 
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
v17.1.0 release proposal #40758
Merged
BethGriggs pushed a commit that referenced this pull request Nov 24, 2021
@zcbenz @BethGriggs
build: disable v8 pointer compression on 32bit archs
4581997 
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
v16.13.1 proposal #40974
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. 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.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@zcbenz @gengjiawen @targos @richardlau @nodejs-github-bot @mhdawson @tniessen @JungMinu @sxa @Mesteery
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.