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: fix ARM vfp naming in configure script for older ARM processors #2751

Closed
wants to merge 1 commit into from
Closed

build: fix ARM vfp naming in configure script for older ARM processors #2751

wants to merge 1 commit into from

Conversation

mnkhouri
Copy link

@mnkhouri mnkhouri commented Sep 9, 2015

This commit changes the default vfp name from 'vfpv2' to 'vfp', affecting
builds for ARM architectures older than ARMv7.

Valid options for 'arm_fpu' in gcc do NOT include 'vfpv2', causing gcc to
quit with error: unrecognized argument in option ‘-mfpu=vfpv2’ when
using the configure script with --dest-cpu=arm.

Gyp also expects only vfp, vfpv3-d16, vfpv3, neon.

GCC reference for the -mfpu= option:
https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
Gyp makefile reference:
https://github.com/nodejs/node/blob/v4.0.0/Makefile.build#L173

@mnkhouri
Copy link
Author

mnkhouri commented Sep 9, 2015

To reproduce:
./configure --dest-cpu=arm --dest-os=linux --with-arm-float-abi softfp && make

My toolchain:
export AR=arm-linux-gnueabi-ar CC=arm-linux-gnueabi-gcc-4.9 CXX=arm-linux-gnueabi-g++-4.9 LINK=arm-linux-gnueabi-g++-4.9

@mscdex mscdex added build Issues and PRs related to build files or the CI. arm Issues and PRs related to the ARM platform. labels Sep 9, 2015
@rvagg
Copy link
Member

rvagg commented Sep 9, 2015

/ @nodejs/build

@bnoordhuis
Copy link
Member

LGTM apart from the commit log: the first line should be <= 50 columns, ones after that <= 72.

arm_fpu is only used by V8's gyp files. I speculate that the way they use it has changed in recent versions, resulting in this breakage.

This commit changes the default vfp name from 'vfpv2' to 'vfp',
affecting builds for ARM architectures older than ARMv7.

Valid options for 'arm_fpu' in gcc do NOT include 'vfpv2', causing gcc
to quit with error: unrecognized argument in option ‘-mfpu=vfpv2’ when
using the configure script with --dest-cpu=arm.

Gyp also expects only vfp, vfpv3-d16, vfpv3, neon.

GCC reference for the -mfpu= option:
https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
Gyp makefile reference:
https://github.com/nodejs/node/blob/v4.0.0/Makefile.build#L173
@mnkhouri
Copy link
Author

mnkhouri commented Sep 9, 2015

Amended commit message to match column guidelines

@Fishrock123
Copy link
Contributor

pinging @nodejs/build again

@bnoordhuis
Copy link
Member

I think this has been obsoleted by 17665af.

@Fishrock123
Copy link
Contributor

Hmm, maybe? It's possible that the default should still be changed? 17665af#diff-e2d5a00791bce9a01f99bc6fd613a39dR618

@bnoordhuis
Copy link
Member

Maybe not obsoleted then but it certainly needs rebasing.

@ronkorving
Copy link
Contributor

Is this still relevant? Does it still need rebasing? Ping @mnkhouri

@mnkhouri
Copy link
Author

Someone beat me to the punch: 84dea1b

Closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Issues and PRs related to the ARM platform. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants