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: clean up arm build options #32704

Closed
wants to merge 3 commits into from
Closed

build: clean up arm build options #32704

wants to merge 3 commits into from

Conversation

bnoordhuis
Copy link
Member

  • default to -march=armv7-a+neon-vfp3 when targeting armv7. Any
    Cortex-A capable of running V8 supports at least that.

  • remove the --arm-fpu=... configure option. It was added to support
    Debian's armel port (armv5, for the purpose of this discussion) but
    V8 (and therefore Node.js) dropped support for armv5 in early 2016.

  • remove the --arm-float-abi=... configure option. There should never
    be a reason to build in soft or softp mode; armv6 is the bare minimum
    and supports VFP.

Fixes: #30786

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. zlib Issues and PRs related to the zlib subsystem. labels Apr 7, 2020
@bnoordhuis bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 7, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bnoordhuis
Copy link
Member Author

Marking this semver-major because it affects how add-ons are built (and that's intentional - see the discussion in #30786.)

@bnoordhuis bnoordhuis added the arm Issues and PRs related to the ARM platform. label Apr 7, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
mscdex
mscdex reviewed Apr 7, 2020
View reviewed changes
deps/zlib/zlib.gyp Outdated
@@ -64,7 +64,7 @@
'USE_FILE32API'
],
}],
['(target_arch in "ia32 x64 x32" and OS!="ios") or arm_fpu=="neon"', {
['(target_arch in "ia32 x64 x32" and OS!="ios") or arm_version==7', {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Is this a safe assumption that armv7 will have neon support?
  • What about armv8 and newer (which always have neon)? Do they get detected as 7?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Yes, see the commit log.

  • armv8 is --dest-cpu=arm64, armv7 and armv6 are --dest-cpu=arm.

mscdex
mscdex reviewed Apr 7, 2020
View reviewed changes
common.gypi Outdated
@@ -268,6 +269,10 @@
'msvs_cygwin_shell': 0, # prevent actions from trying to use cygwin

'conditions': [
[ 'arm_version==7', {
# Any Cortex-A that can run V8 supports at least armv7-a+neon-vfpv3.
'cflags': [ '-march=armv7-a+neon-vfpv3' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I did a brief search and at least Tegra 2-based (Cortex-A9 -- armv7-a) devices do not include neon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, such SoCs exist but they aren't able to run V8 in ARMv7 mode, according to V8:

node/deps/v8/src/codegen/arm/assembler-arm.cc

Lines 156 to 163 in c849f2d

// For cross compilation the preprocessor symbols such as
// CAN_USE_ARMV7_INSTRUCTIONS and CAN_USE_VFP3_INSTRUCTIONS can be used to
// enable ARMv7 and VFPv3 instructions when building the snapshot. However,
// these flags should be consistent with a supported ARM configuration:
// "armv6": ARMv6 + VFPv2
// "armv7": ARMv7 + VFPv3-D32 + NEON
// "armv7+sudiv": ARMv7 + VFPv4-D32 + NEON + SUDIV
// "armv8": ARMv8 (+ all of the above)

node/deps/v8/src/codegen/arm/assembler-arm.cc

Lines 218 to 222 in c849f2d

unsigned runtime = kArmv6;
// NEON and VFPv3 imply at least ARMv7-A.
if (cpu.has_neon() && cpu.has_vfp3_d32()) {
DCHECK(cpu.has_vfp3());
runtime |= kArmv7;

Tegra 2 specifically is pretty obsolete (we're talking 10 year old smartphones and tablets) but if someone wanted to build node for it, they can with ./configure -- -Darm_version=6.

Is there a more recent SoC where this is an issue?

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bnoordhuis
Copy link
Member Author 

22:33:50 arm-rpi-linux-gnueabihf-gcc: error: unrecognized argument in option '-mfpu=neon-vfpv3'
22:33:50 arm-rpi-linux-gnueabihf-gcc: note: valid arguments to '-mfpu=' are: crypto-neon-fp-armv8 fp-armv8 fpv4-sp-d16 fpv5-d16 fpv5-sp-d16 neon neon-fp-armv8 neon-fp16 neon-vfpv4 vfp vfp3 vfpv3 vfpv3-d16 vfpv3-d16-fp16 vfpv3-fp16 vfpv3xd vfpv3xd-fp16 vfpv4 vfpv4-d16

🤦‍- fortunately -mfpu=neon implies vfpv3.

@bnoordhuis
build: clean up arm build options
64a4910 
* default to `-march=armv7` and `-mfpu=neon` when targeting armv7.
  Any Cortex-A capable of running V8 supports at least that.

* remove the `--arm-fpu=...` configure option. It was added to support
  Debian's armel port (armv5, for the purpose of this discussion) but
  V8 (and therefore Node.js) dropped support for armv5 in early 2016.

* remove the `--arm-float-abi=...` configure option. There should never
  be a reason to build in soft or softp mode; armv6 is the bare minimum
  nowadays and supports VFP.

Fixes: nodejs#30786
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@bnoordhuis
Copy link
Member Author

I'm running out of patience with this PR. Test failures because of weird floating-point issues...

14:09:44     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
14:09:44     + actual - expected
14:09:44     
14:09:44     + 3.000000953208655
14:09:44     - 3
14:09:44         at assertCursorRowsAndCols (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-readline-interface.js:62:10)

(That's just one example.)

If someone wants to pick this up again, be my guest.

@bnoordhuis bnoordhuis closed this Apr 10, 2020
@bnoordhuis bnoordhuis mentioned this pull request Apr 17, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

Assignees
No one assigned
Labels
arm Issues and PRs related to the ARM platform. build Issues and PRs related to build files or the CI. semver-major PRs that contain breaking changes and should be released in the next major version. tools Issues and PRs related to the tools directory. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

addons/openssl-binding/test very flaky on ARM
3 participants
@bnoordhuis @nodejs-github-bot @mscdex