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

Fix swift error throwing on non-darwin ARM platforms #55

Closed
wants to merge 1 commit into
base: stable
from

Conversation

Projects
None yet
3 participants
@johnno1962

johnno1962 commented Jul 23, 2017

A minor change to get Swift Error throwing to work on non-darwin ARM32 platforms (e.g. Android.) Without this change, code generated saves and restores ARM register r8 at the entry and returns of a function that throws. As r8 is used as a virtual return value for the object being thrown this gets overwritten by the restore and calling code is unable to catch the error. This was causing do/try/catch not to work on Android as it is an ARM32 platform that does not satisfy "STI.isTargetDarwin()"

Addresses SR-5438

@johnno1962 johnno1962 changed the title from Fix swift error throwing on non-darwin platforms to Fix swift error throwing on non-darwin ARM platforms Jul 23, 2017

@johnno1962 johnno1962 changed the title from Fix swift error throwing on non-darwin ARM platforms to do not merge - Fix swift error throwing on non-darwin ARM platforms Jul 23, 2017

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Jul 23, 2017

There was an issue but it’s been resolved by completely clearing out the build directory and restarting. This looks good to go.

johnno1962 commented Jul 23, 2017

There was an issue but it’s been resolved by completely clearing out the build directory and restarting. This looks good to go.

@johnno1962 johnno1962 changed the title from do not merge - Fix swift error throwing on non-darwin ARM platforms to Fix swift error throwing on non-darwin ARM platforms Jul 23, 2017

@jrose-apple jrose-apple requested a review from rjmccall Jul 24, 2017

@rjmccall

This comment has been minimized.

Show comment
Hide comment
@rjmccall

rjmccall Jul 24, 2017

Member

Yes, this looks right to me.

Member

rjmccall commented Jul 24, 2017

Yes, this looks right to me.

@rjmccall

This comment has been minimized.

Show comment
Hide comment
@rjmccall

rjmccall Jul 24, 2017

Member

Ideally Manman Ren would review this, but she's no longer working on this project.

Member

rjmccall commented Jul 24, 2017

Ideally Manman Ren would review this, but she's no longer working on this project.

@rjmccall

This comment has been minimized.

Show comment
Hide comment
@rjmccall

rjmccall Jul 25, 2017

Member

This patch should go into the main LLVM repository, though.

Member

rjmccall commented Jul 25, 2017

This patch should go into the main LLVM repository, though.

@rjmccall

This comment has been minimized.

Show comment
Hide comment
@rjmccall

rjmccall Jul 25, 2017

Member

@aschwaighofer Please review and help this land in LLVM.

Member

rjmccall commented Jul 25, 2017

@aschwaighofer Please review and help this land in LLVM.

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Jul 25, 2017

Thanks @rjmccall, @aschwaighofer, I think I have found someone to propose this patch to llvm - @modocache

johnno1962 commented Jul 25, 2017

Thanks @rjmccall, @aschwaighofer, I think I have found someone to propose this patch to llvm - @modocache

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 commented Jul 25, 2017

Awaiting review on llvm: https://reviews.llvm.org/D35835

@aschwaighofer

This comment has been minimized.

Show comment
Hide comment
@aschwaighofer

aschwaighofer Jul 25, 2017

Member

@modocache @johnno1962 The change looks good but needs a test case. I have left a review at the llvm's review site.

Thank you!

Member

aschwaighofer commented Jul 25, 2017

@modocache @johnno1962 The change looks good but needs a test case. I have left a review at the llvm's review site.

Thank you!

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Aug 30, 2017

@aschwaighofer , @rjmccall. I understand this patch has landed in llvm trunk if you want to cherry-pick it up. Thanks @modocache.

johnno1962 commented Aug 30, 2017

@aschwaighofer , @rjmccall. I understand this patch has landed in llvm trunk if you want to cherry-pick it up. Thanks @modocache.

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Oct 2, 2017

Seems like you’ve picked this up. I’ll close the PR now - Thanks!

johnno1962 commented Oct 2, 2017

Seems like you’ve picked this up. I’ll close the PR now - Thanks!

@johnno1962 johnno1962 closed this Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment