Skip to content

bpo-35758: Fix building on ARM + MSVC #11531

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
Merged
merged 5 commits into from
Jan 21, 2019
Merged

bpo-35758: Fix building on ARM + MSVC #11531

merged 5 commits into from
Jan 21, 2019

Conversation

gongminmin
Copy link
Contributor

@gongminmin gongminmin commented Jan 12, 2019 •
edited by bedevere-bot
Loading

Msvc defines _M_ARM for arm target, but it doesn't have x87 control word. Need to disable it to prevent some compiling problems.

https://bugs.python.org/issue14802

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

eamanu
eamanu suggested changes Jan 12, 2019
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

You must sign CLA and add NEWS. Maybe this PR need more discuss. I recommend open a new bpo or send some comments to the issue to activate it.

@gongminmin
Copy link
Contributor Author

I'll file a new bug about this. Thanks.

@gongminmin gongminmin changed the title bpo-14802: Disable x87 control word for ARM bpo-35758 : Disable x87 control word for ARM Jan 17, 2019
@gongminmin
Copy link
Contributor Author

CLA signed, and a new bug 35758 is filed for disabling x87 on ARM.

pitrou
pitrou requested changes Jan 19, 2019
View reviewed changes
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@gongminmin
Disable x87 control word for non-x86 targets
b8bcddd 
On msvc, x87 control word is only available on x86 target. Need to disable it for other targets to prevent compiling problems.
@gongminmin
Copy link
Contributor Author

I found another problem about arm. In Include\internal\pycore_atomic.h, immintrin.h should only be included on x86 and x64. Should I fix it in this PR or open another one? Thanks.

@gongminmin
Copy link
Contributor Author

I have made the requested changes; please review again. And also add an entry in Misc/News.d/next.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

@pitrou
Copy link
Member

pitrou commented Jan 21, 2019

Should I fix it in this PR or open another one?

You can fix it in this PR.

@gongminmin
Include immintrin.h on x86 and x64 only
b180407 
Immintrin.h is only available on x86 and x64. Need to disable it for other targets to prevent compiling problems.
@pitrou
Copy link
Member

pitrou commented Jan 21, 2019

Thanks @gongminmin. Is this sufficient to get Python to compile and run on ARM + MVSC? If so, I think we can merge soon.

@pitrou pitrou changed the title bpo-35758 : Disable x87 control word for ARM bpo-35758 : Fix building on ARM + MSVC Jan 21, 2019
@pitrou pitrou changed the title bpo-35758 : Fix building on ARM + MSVC bpo-35758: Fix building on ARM + MSVC Jan 21, 2019
@gongminmin
Copy link
Contributor Author

Yes, with this change, I can build arm and arm64 pythoncore and most modules.

@pitrou
Copy link
Member

pitrou commented Jan 21, 2019

Perfect, will merge then ;-)

@pitrou pitrou merged commit 7a23680 into python:master Jan 21, 2019
@gongminmin
Copy link
Contributor Author

Thanks very much!

@gongminmin gongminmin deleted the FixForArm branch January 21, 2019 20:51
gongminmin added a commit to gongminmin/cpython that referenced this pull request Jan 22, 2019
@gongminmin
bpo-35758: Fix building on ARM + MSVC (pythongh-11531)
d72e264 
* Disable x87 control word for non-x86 targets

On msvc, x87 control word is only available on x86 target. Need to disable it for other targets to prevent compiling problems.

* Include immintrin.h on x86 and x64 only

Immintrin.h is only available on x86 and x64. Need to disable it for other targets to prevent compiling problems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou approved these changes

+1 more reviewer

@eamanu eamanu eamanu requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gongminmin @the-knights-who-say-ni @bedevere-bot @pitrou @eamanu