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

configure: use __ARM_ARCH to determine arm version #4123

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

joaocgreis
Copy link
Member

Before this change, configure used processor specific macro defines (like __ARM_ARCH_6M__) to detect the arm processor version. This changes configure to use __ARM_ARCH, that should be defined to the correct version.

This is useful because configure may not expect the flag for a specific processor. Currently, if trying to cross compile optimizing to the Raspberry Pi 1 processor with -mcpu=arm1176jzf-s the detected arm_version will be default (because it sets__ARM_ARCH_6ZK__), and the generated binaries useless in that processor.

@nodejs/platform-arm ARM processors currently on CI are ok with this change, but could there be some other processor or compiler that does not set __ARM_ARCH or sets it to something that we do not want? @mmalecki you authored the original function, do you know of any problem with this? (Thanks!)

cc @nodejs/build

For reference:

$ echo | arm-linux-gnueabihf-gcc -dM -E - | grep ARM_ARCH
#define __ARM_ARCH_ISA_ARM 1
#define __ARM_ARCH_6__ 1
#define __ARM_ARCH_ISA_THUMB 1
#define __ARM_ARCH 6
$ echo | arm-linux-gnueabihf-gcc -mcpu=arm1176jzf-s -dM -E - | grep ARM_ARCH
#define __ARM_ARCH_ISA_ARM 1
#define __ARM_ARCH_ISA_THUMB 1
#define __ARM_ARCH 6
#define __ARM_ARCH_6ZK__ 1
$ echo | arm-linux-gnueabihf-gcc -mcpu=cortex-a7 -dM -E - | grep ARM_ARCH                                                    
#define __ARM_ARCH_ISA_ARM 1
#define __ARM_ARCH_PROFILE 65
#define __ARM_ARCH_ISA_THUMB 2
#define __ARM_ARCH 7
#define __ARM_ARCH_7A__ 1
#define __ARM_ARCH_EXT_IDIV__ 1

@joaocgreis joaocgreis added build Issues and PRs related to build files or the CI. arm Issues and PRs related to the ARM platform. lts-watch-v4.x labels Dec 2, 2015
'__ARM_ARCH_7M__' in cc_macros_cache or
'__ARM_ARCH_7S__' in cc_macros_cache)
return ('__ARM_ARCH' in cc_macros_cache and
cc_macros_cache['__ARM_ARCH'] == '7')
Copy link
Member

Choose a reason for hiding this comment

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

You could simplify this to return cc_macros_cache.get('__ARM_ARCH') == '7'.

@bnoordhuis
Copy link
Member

LGTM with a comment.

@mmalecki
Copy link
Contributor

mmalecki commented Dec 3, 2015

@joaocgreis Not aware of such, no.

@jasnell
Copy link
Member

jasnell commented Dec 3, 2015

@joaocgreis
Copy link
Member Author

Updated, thanks for the reviews! Will land Tuesday if there are no further comments.
CI with the changes: https://ci.nodejs.org/job/node-test-pull-request/944/

@joaocgreis
Copy link
Member Author

Note: armv7-wheezy detects arm_version as 6, but that already happened before. I assume it's because the default compiler targets armv6, as this is what happens in the Raspberry Pi 2.

@rvagg
Copy link
Member

rvagg commented Dec 7, 2015

armv7-wheezy isn't Raspbian fwiw, it's proper Wheezy running on some ARMv7 machines.

Before this change, configure used processor specific macro defines
(like __ARM_ARCH_6M__) to detect the arm processor version. This
changes configure to use __ARM_ARCH, that should be defined to the
correct version.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#4123
@joaocgreis joaocgreis merged commit a7f5dfd into nodejs:master Dec 9, 2015
joaocgreis added a commit that referenced this pull request Dec 9, 2015
Before this change, configure used processor specific macro defines
(like __ARM_ARCH_6M__) to detect the arm processor version. This
changes configure to use __ARM_ARCH, that should be defined to the
correct version.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #4123
@joaocgreis
Copy link
Member Author

Landed in v4.x-staging in 88c17f8

joaocgreis added a commit that referenced this pull request Dec 15, 2015
Before this change, configure used processor specific macro defines
(like __ARM_ARCH_6M__) to detect the arm processor version. This
changes configure to use __ARM_ARCH, that should be defined to the
correct version.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #4123
@jasnell jasnell mentioned this pull request Dec 17, 2015
joaocgreis added a commit that referenced this pull request Dec 17, 2015
Before this change, configure used processor specific macro defines
(like __ARM_ARCH_6M__) to detect the arm processor version. This
changes configure to use __ARM_ARCH, that should be defined to the
correct version.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #4123
joaocgreis added a commit that referenced this pull request Dec 23, 2015
Before this change, configure used processor specific macro defines
(like __ARM_ARCH_6M__) to detect the arm processor version. This
changes configure to use __ARM_ARCH, that should be defined to the
correct version.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #4123
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Before this change, configure used processor specific macro defines
(like __ARM_ARCH_6M__) to detect the arm processor version. This
changes configure to use __ARM_ARCH, that should be defined to the
correct version.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#4123
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