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

[LLVM 4.0] Set EH personality when resuming stack unwinding #37862

Merged

Conversation

shepmaster
Copy link
Member

To resume stack unwinding, the LLVM resume instruction must be used.

In order to use this instruction, the calling function must have an
exception handling personality set.

LLVM 4.0 adds a new IR validation check to ensure a personality is
always set in these cases.

This was introduced in r277360.

To resume stack unwinding, the LLVM `resume` instruction must be used.

In order to use this instruction, the calling function must have an
exception handling personality set.

LLVM 4.0 adds a new IR validation check to ensure a personality is
always set in these cases.

This was introduced in [r277360](https://reviews.llvm.org/rL277360).
@shepmaster
Copy link
Member Author

/cc @dylanmckay

@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@shepmaster shepmaster mentioned this pull request Nov 18, 2016
23 tasks
@nrc
Copy link
Member

nrc commented Nov 18, 2016

r? @eddy

@shepmaster
Copy link
Member Author

@nrc did you mean @eddyb ?

@shepmaster
Copy link
Member Author

I feel like I've seen @alexcrichton's name on exception-handling related stuff as well.

@alexcrichton
Copy link
Member

Seems fine to me, but I'm not too familiar with MIR trans. I'd probably imagine this'd happen somewhere where the invoke instruction was created that the resume is corresponding to, but maybe that's buried elsewhere? In any case, @eddyb is probably the best to know whether this is the right place!

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned nrc Nov 18, 2016
@eddyb
Copy link
Member

eddyb commented Nov 19, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Nov 19, 2016

📌 Commit 84415ea has been approved by eddyb

@@ -116,6 +116,9 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
if let Some(cleanup_pad) = cleanup_pad {
bcx.cleanup_ret(cleanup_pad, None);
} else {
let llpersonality = bcx.fcx().eh_personality();
bcx.set_personality_fn(llpersonality);
Copy link
Member

@nagisa nagisa Nov 20, 2016

Choose a reason for hiding this comment

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

I believe I said already in #36895, this code seems dubiously workaround-ish. Can we fix this properly, by fixing (if its not enough) up in the function near line 80.

Copy link
Member

Choose a reason for hiding this comment

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

Namely, resume should not be translated in any case where there isn’t an unwind/invoke edge. And any unwind/invoke ought to already be setting the personality_fn.

Copy link
Member Author

Choose a reason for hiding this comment

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

I said already in #36895,

Apologies; I wasn't CCed on that PR, so didn't know there were already reviews :-)

@bors
Copy link
Contributor

bors commented Nov 20, 2016

⌛ Testing commit 84415ea with merge 4bc9290...

bors added a commit that referenced this pull request Nov 20, 2016
… r=eddyb

[LLVM 4.0] Set EH personality when resuming stack unwinding

To resume stack unwinding, the LLVM `resume` instruction must be used.

In order to use this instruction, the calling function must have an
exception handling personality set.

LLVM 4.0 adds a new IR validation check to ensure a personality is
always set in these cases.

This was introduced in [r277360](https://reviews.llvm.org/rL277360).
@bors bors merged commit 84415ea into rust-lang:master Nov 20, 2016
@shepmaster shepmaster deleted the llvm-4.0-always-set-eh-personality branch April 25, 2018 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants