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.

Sign up for GitHub

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

Jump to bottom

build: fix arm64 cross-compilation #45756

Closed
wants to merge 2 commits into from
Closed

build: fix arm64 cross-compilation #45756

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
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?

@bnoordhuis
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
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@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 Dec 6, 2022
@bnoordhuis bnoordhuis mentioned this pull request Dec 6, 2022
Closed
@josh-hemphill
Copy link

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
Copy link
Member

targos commented Dec 6, 2022

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
Copy link
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
Copy link

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
Copy link

nxhack commented Dec 8, 2022

@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
Copy link
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 richardlau mentioned this pull request Feb 20, 2023
Merged
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023
@github-actions 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
Copy link
Collaborator

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

This was referenced Feb 21, 2023
Open
Open
@nxhack nxhack mentioned this pull request Feb 22, 2023
Closed
@richardlau 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
@baparham
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
@baparham
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
@baparham
patches: add cross compiling fix to node 19
1c43dd6 
based on suggested change from nodejs/node#45756
@baparham baparham mentioned this pull request Mar 8, 2023
Merged
3 tasks
@PeterBurner
Copy link

any news on this?

baparham added a commit to vercel/pkg-fetch that referenced this pull request Mar 15, 2023
@baparham
patches: add cross compiling fix to node 19
61b0b1c 
based on suggested change from nodejs/node#45756
@targos 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
@theofficialgman
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
@theofficialgman
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
@theofficialgman
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
@staltz
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
@staltz
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
Copy link
Contributor

aduh95 commented Sep 20, 2023

Ping @nodejs/build

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@sxa
Copy link
Member

sxa commented Sep 20, 2023

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

@github-actions 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
@staltz
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
@staltz
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 robertsLando mentioned this pull request Dec 4, 2023
Merged
@robertsLando
Copy link

News on this?

@targos targos mentioned this pull request Dec 22, 2023
Merged
@nikolaik
Copy link

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

@targos
Copy link
Member

targos commented Dec 29, 2023

Correct. Superseded by #51256.

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