Skip to content

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