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

build: fix arm64 cross-compilation by bnoordhuis · Pull Request #45756 · nodejs/node · GitHub

/ node Public

build: fix arm64 cross-compilation - #45756

Closed
bnoordhuis wants to merge 2 commits into
nodejs:mainfrom
bnoordhuis:fix42888
ConversationCommits (2)ChecksFiles changed
Closed

build: fix arm64 cross-compilation#45756
bnoordhuis wants to merge 2 commits into
nodejs:mainfrom
bnoordhuis:fix42888

Conversation

bnoordhuis commented Dec 6, 2022

Copy link
Copy Markdown
Member

Commit 938212f added -msign-return-address=all to all cflags but that is wrong when cross-compiling, it should only be added to the target's cflags.

Fixes: #42888

cc @nxhack @josh-hemphill - can you confirm it works for you?

build: fix arm64 cross-compilation
6bdf9dc 
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

Fixes: nodejs#42888

nodejs-github-bot commented Dec 6, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

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 Dec 6, 2022
bnoordhuis mentioned this pull request Dec 6, 2022
Closed

josh-hemphill commented Dec 6, 2022

Copy link
Copy Markdown

I haven't finished building yet, but looks like I just get a whole lot of warnings that switch '-msign-return-address=all' is no longer supported
I think it will build though, so I think it's working.

targos commented Dec 6, 2022

Copy link
Copy Markdown
Member

FWIW V8 used to pass -mbranch-protection=standard (we also do it in v8_gypfiles/toolchain.gypi), but removed it in https://chromium-review.googlesource.com/c/v8/v8/+/3829068 (not sure why).

richardlau commented Dec 6, 2022

Copy link
Copy Markdown
Member

I haven't finished building yet, but looks like I just get a whole lot of warnings that switch '-msign-return-address=all' is no longer supported I think it will build though, so I think it's working.

You're on a newer version of gcc. We're currently using gcc 8 in the CI and to build the release binaries. gcc 9 introduced a replacement option, -mbranch-protection and deprecated the old one.

josh-hemphill commented Dec 7, 2022

Copy link
Copy Markdown

I ran into other dependency issues when using gcc 8, which I could probably get around, but I'm working with docker containers and want to limit to a single gcc version installed if I can to keep the image size down.
It seems to build just fine with gcc 9 if I sed replace the flag.
Anyone know if there's other gotchas using gcc 9? Just want to make sure I'm not sabotaging myself doing that.

nxhack commented Dec 8, 2022

Copy link
Copy Markdown

@bnoordhuis

My gcc cross-compiler version is 11.3.0, so I tested with
flag with branch-protection=standard.

It builds and runs without any problems.

However, I am concerned about the v8 upstream fix.

bnoordhuis commented Dec 8, 2022

Copy link
Copy Markdown
Member Author

It's part of Chromium's (and therefore V8's) push towards CFI (Control Flow Integrity.)

The design doc is here if you're interested: https://docs.google.com/document/d/1O2jwK4dxI3nRcOJuPYkonhTkNQfbmwdvxQMyXgeaRHo/edit

I'm going to guess they view arm64 branch protection as a special case that's been subsumed by the (more general and cross-platform) CFI work.

richardlau mentioned this pull request Feb 20, 2023
Merged
mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023

nodejs-github-bot commented Feb 20, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Feb 21, 2023
Open
Open
nxhack mentioned this pull request Feb 22, 2023
Closed
richardlau mentioned this pull request Feb 24, 2023
Merged
baparham added a commit to baparham/pkg-fetch that referenced this pull request Feb 24, 2023
patches: add cross compiling fix to node 19
04bf044 
based on suggested change from nodejs/node#45756
baparham added a commit to baparham/pkg-fetch that referenced this pull request Mar 8, 2023
patches: add cross compiling fix to node 19
139aa8f 
based on suggested change from nodejs/node#45756
baparham added a commit to baparham/pkg-fetch that referenced this pull request Mar 8, 2023
patches: add cross compiling fix to node 19
1c43dd6 
based on suggested change from nodejs/node#45756
baparham mentioned this pull request Mar 8, 2023
Merged
3 tasks

PeterBurner commented Mar 14, 2023

Copy link
Copy Markdown

any news on this?

baparham added a commit to vercel/pkg-fetch that referenced this pull request Mar 15, 2023
patches: add cross compiling fix to node 19
61b0b1c 
based on suggested change from nodejs/node#45756
targos mentioned this pull request Apr 21, 2023
Closed
This was referenced Apr 30, 2023
Closed
Open
theofficialgman added a commit to theofficialgman/unofficial-builds that referenced this pull request Jul 18, 2023
add patch until upstream arm64 cross compilation is fixed
87a44f5 
see nodejs/node#45756
theofficialgman added a commit to theofficialgman/unofficial-builds that referenced this pull request Aug 7, 2023
add patch until upstream arm64 cross compilation is fixed
9583ed0 
see nodejs/node#45756
theofficialgman added a commit to theofficialgman/unofficial-builds that referenced this pull request Aug 7, 2023
add patch until upstream arm64 cross compilation is fixed
c45cc02 
see nodejs/node#45756
staltz added a commit to nodejs-mobile/nodejs-mobile that referenced this pull request Sep 12, 2023
android,build: dont set a certain cflag globally
bcb194c 
This fixes "msign-return-address=all" unrecognized by g++ for obj.host
Basically copied from nodejs/node#45756
staltz added a commit to nodejs-mobile/nodejs-mobile that referenced this pull request Sep 13, 2023
android,build: dont set a certain cflag globally
9e6c250 
This fixes "msign-return-address=all" unrecognized by g++ for obj.host
Basically copied from nodejs/node#45756

aduh95 commented Sep 20, 2023

Copy link
Copy Markdown
Contributor

Ping @nodejs/build

nodejs-github-bot commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator

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

sxa commented Sep 20, 2023

Copy link
Copy Markdown
Member

Resolved the obvious merge conflict but looks like a little more remedial work will be needed before this can be progressed.

github-actions Bot mentioned this pull request Sep 21, 2023
Open
22 tasks
staltz added a commit to nodejs-mobile/nodejs-mobile that referenced this pull request Oct 3, 2023
android,build: dont set a certain cflag globally
650062b 
This fixes "msign-return-address=all" unrecognized by g++ for obj.host
Basically copied from nodejs/node#45756
staltz added a commit to nodejs-mobile/nodejs-mobile that referenced this pull request Oct 12, 2023
android,build: dont set a certain cflag globally
7d6236e 
This fixes "msign-return-address=all" unrecognized by g++ for obj.host
Basically copied from nodejs/node#45756
robertsLando mentioned this pull request Dec 4, 2023
Merged

robertsLando commented Dec 4, 2023

Copy link
Copy Markdown
Contributor

News on this?

targos mentioned this pull request Dec 22, 2023
Merged

nikolaik commented Dec 28, 2023

Copy link
Copy Markdown

This seems to have been fixed in #51256 or am I misunderstanding?

targos commented Dec 29, 2023

Copy link
Copy Markdown
Member

Correct. Superseded by #51256.

targos closed this Dec 29, 2023
robertsLando mentioned this pull request Jan 3, 2024
Merged
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

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable pointer authentication on ARM64
Back | FazBrowse Home | New Git URL