Mark extern functions as nounwind + other unwinding-based improvements #21186

Aatch · 2015-01-15T08:08:38Z

This PR does a few things:

Marks functions with a non-Rust ABI as nounwind.
Catches unwinding at FFI boundary in extern fn foo() { ... } functions and aborts. This prevents us from unwinding from Rust code into C code.
Adds a #[can_unwind] attribute for extern functions. This is for when we know a non-Rust ABI function will have to unwind. Used mainly in the unwinding implementation.
Adds a #[nounwind] attribute. It is intended for when a function that won't unwind is used cross-crate, but using #[inline] is not appropriate. It is currently used for the oom function in liballoc

rust-highfive · 2015-01-15T08:08:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

alexcrichton · 2015-01-15T08:15:34Z

These seem like they might be potentially dangerous attributes to be playing around with, so could they be behind a feature gate for now? We may also want to scrutinize the naming of these attributes in the future before allowing full use of them.

Also, can you be sure to add some positive and negative tests for the cases added here? Out of curiosity, do you have statistics about code size in before/after cases as well?

alexcrichton · 2015-01-15T08:18:06Z

src/librustc_trans/trans/foreign.rs

+
+            let pad_ret_val = builder.landing_pad(pad_ty, llpersonality, 1us);
+            builder.add_clause(pad_ret_val, C_null(Type::i8p(ccx)));
+            let trap = ccx.get_intrinsic(&("llvm.trap"));


It may be a little unfortunate to call llvm.trap here as unwinding across an extern fn (while not safe) may be a common newbie mistake to make. This unfortunately doesn't really aid in debugging as well as it basically just crashes the process.

We may want to explore a new lang item here which has the chance to print something, or perhaps a different strategy.

Yeah, though there are several other places do that do similarly-helpful things, so a more general-purpose solution might be more appropriate.

Also, a trap will still be caught by a debugger and allow for a backtrace to be printed, so it's at least better than whatever happens when you do unwind into C.

Yeah I was thinking that this could take the same strategy as divide-by-zero or out-of-bounds only hardwired to an abort function instead of a panic function. I didn't know about the debugger though, that's pretty slick!

alexcrichton · 2015-01-15T08:18:40Z

Thanks for this! Seems like our FFI is about to get even better!

Aatch · 2015-01-15T22:55:00Z

Hmm, looks like we were already assuming that extern couldn't unwind and therefore the cleanups weren't be added for them.

Aatch · 2015-01-16T00:26:42Z

Updated and added a test, should be good to go now.

huonw · 2015-01-16T00:41:26Z

It seems mildly inconsistent to call them #[can_unwind] and #[nounwind]? Also, it seems that the latter should, strictly speaking, be #[unsafe_no_unwind]?

alexcrichton · 2015-01-16T04:18:24Z

Could you add a test as well for the feature gates? (make sure they're actually gating)

alexcrichton · 2015-01-16T04:18:33Z

src/librustc/lint/builtin.rs

@@ -653,6 +653,8 @@ impl LintPass for UnusedAttributes {
            "no_debug",
            "omit_gdb_pretty_printer_section",
            "unsafe_no_drop_flag",
+            "can_unwind",
+            "nounwind",


s/nounwind/unsafe_no_unwind

alexcrichton · 2015-01-16T21:20:31Z

This looks pretty great to me, nice work @Aatch! I'm basically happy with the implementation, but I would like to canvas for some more opinions specifically about:

What do others think about abort-by-default when unwinding past an extern fn?
What do others think about abort only using llvm.trap (no message printing) in that case?
What do others think about an opt-in #[can_unwind] vs an opt-out?

Aatch · 2015-01-16T21:27:54Z

Just to put my answers in (biased as they may be):

The majority of extern fns are intended for use with a non-Rust program, normally C, that will not understand unwinding. There is no sane way for a program to unwind across those boundaries.
I would like a way to print a better message, but as I mentioned before, we already need a general-purpose way to just print out a message in these cases. Notably OOM just aborts without a message too.
See the first point, it's going to be pretty rare that you want unwinding across these boundaries. Note that I only realised we needed it because I broke the unwinding code. To put this another way, the only time we expect there to be unwinding out of an extern fn is when that function is part of the unwinding machinery itself.

alexcrichton · 2015-01-16T21:35:47Z

To be clear, I totally agree with your answers to questions 1/3, just wanna make sure we're all on the same page :)

For point 2, I do think it's ok to land now, I would just much rather have a nicer message. I've spent hours tracking core dumps before only to realize it was OOM and it have been super duper helpful if a message was actually printed!

alexcrichton · 2015-01-20T22:06:42Z

Ok, we chatted about this today in the weekly meeting and here's an update. First off, thanks again for the PR! We're all pretty excited to take advantage of the optimizations LLVM provides. For now, however, we're thinking we may want step back and make an RFC for this PR before landing.

The primary change we're worried about is the difference in unwinding semantics when unwinding past an extern fn and we think that there may be some design space to explore before committing to one strategy. We had some fleeting ideas such as using a similar strategy for unwinding in destructors perhaps. Regardless, we're a little unsure that these are the semantics we'd want to commit to, so we'd just want to canvas some more feedback before moving forward in this respect.

I'd be more than willing to help out in writing up, proofing, or otherwise just reviewing an RFC for this area. Feel free to ping me on IRC about this whenever!

To move forward this this specific PR I think we can take a slightly altered strategy as well. In the past we've accepted implementation details which improved the efficiency of standard library primitives (e.g. NonZero) before we were ready to commit to a public API (it's entirely an implementation detail). Along these lines I think that at bare minimum the attributes this PR adds can be accepted pretty quickly (due to being behind a feature gate). I think we may also be able to take advantage of flagging FFI declarations (extern { ... } blocks) as nounwind for LLVM itself, due to unwinding being undefined behavior in that scenario today.

Does that make sense for moving forward? Again, thanks for the work in this area, it's definitely much appreciated!

Functions that use an ABI other than the Rust one are marked as nounwind. Rust functions that use non-Rust ABI are also marked nounwind. To allow for this, the unwinding is caught and we abort the process. Added a `#[can_unwind]` attribute that allows functions using a non-Rust ABI to unwind without aborting. This is required for the unwinding code in libstd to function.

Adds a nounwind attribute that allows functions to be marked as not unwinding. Most useful for cross-crate usage when the nounwind LLVM attribute cannot be inferred. Also fixes marking extern fns as nounwind when the function decl comes from another crate.

We were not allowing for the possibility that we might unwind out of a foreign abi function. Now that we allow for some of these functions to unwind, generate the proper code so clean-ups are performed. This also allowed me to write a test for the functionality, which I have now done.

steveklabnik · 2015-04-18T23:13:17Z

What's the status of this PR? It's quite old at this point.

alexcrichton · 2015-04-19T07:43:20Z

@steveklabnik this is blocked on rust-lang/rfcs#638

alexcrichton · 2015-05-01T17:21:27Z

I'm going to close this in favor of the associated RFC while we work that out.

rust-highfive assigned eddyb Jan 15, 2015

alexcrichton reviewed Jan 15, 2015
View reviewed changes

alexcrichton reviewed Jan 16, 2015
View reviewed changes

Aatch force-pushed the nounwind-c branch from 67e8304 to 0625d1d Compare January 16, 2015 21:12

alexcrichton mentioned this pull request Jan 16, 2015

Add an example to the FFI guide #19968

Closed

Aatch force-pushed the nounwind-c branch from 0625d1d to ac3ead0 Compare January 19, 2015 21:23

alexcrichton mentioned this pull request Jan 20, 2015

Implement a discriminant_value intrinsic #20907

Closed

Aatch added 6 commits January 21, 2015 11:14

Add a nounwind attribute

446ea89

Adds a nounwind attribute that allows functions to be marked as not unwinding. Most useful for cross-crate usage when the nounwind LLVM attribute cannot be inferred. Also fixes marking extern fns as nounwind when the function decl comes from another crate.

Put unwinding attributes behind a feature gate

099b499

Rename nounwind -> unsafe_no_unwind

63bc48b

Add test for the feature gate

e3ab8cb

Aatch force-pushed the nounwind-c branch from ac3ead0 to 729f03b Compare January 20, 2015 23:48

Disable aborting at FFI boundary in foreign-abi rust functions

8575acf

Aatch force-pushed the nounwind-c branch from 729f03b to 8575acf Compare January 21, 2015 07:44

dotdash mentioned this pull request Mar 13, 2015

Add an "allocator" attribute to mark functions as allocators #23322

Merged

emberian assigned emberian and unassigned eddyb Mar 25, 2015

alexcrichton closed this May 1, 2015

bstrie mentioned this pull request Jul 14, 2016

mark C ABI functions as nounwind #18512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark extern functions as nounwind + other unwinding-based improvements #21186

Mark extern functions as nounwind + other unwinding-based improvements #21186

Aatch commented Jan 15, 2015

rust-highfive commented Jan 15, 2015

alexcrichton commented Jan 15, 2015

alexcrichton Jan 15, 2015

Aatch Jan 15, 2015

alexcrichton Jan 15, 2015

alexcrichton commented Jan 15, 2015

Aatch commented Jan 15, 2015

Aatch commented Jan 16, 2015

huonw commented Jan 16, 2015

alexcrichton commented Jan 16, 2015

alexcrichton Jan 16, 2015

alexcrichton commented Jan 16, 2015

Aatch commented Jan 16, 2015

alexcrichton commented Jan 16, 2015

alexcrichton commented Jan 20, 2015

steveklabnik commented Apr 18, 2015

alexcrichton commented Apr 19, 2015

alexcrichton commented May 1, 2015

Mark extern functions as nounwind + other unwinding-based improvements #21186

Mark extern functions as nounwind + other unwinding-based improvements #21186

Conversation

Aatch commented Jan 15, 2015

rust-highfive commented Jan 15, 2015

alexcrichton commented Jan 15, 2015

alexcrichton Jan 15, 2015

Choose a reason for hiding this comment

Aatch Jan 15, 2015

Choose a reason for hiding this comment

alexcrichton Jan 15, 2015

Choose a reason for hiding this comment

alexcrichton commented Jan 15, 2015

Aatch commented Jan 15, 2015

Aatch commented Jan 16, 2015

huonw commented Jan 16, 2015

alexcrichton commented Jan 16, 2015

alexcrichton Jan 16, 2015

Choose a reason for hiding this comment

alexcrichton commented Jan 16, 2015

Aatch commented Jan 16, 2015

alexcrichton commented Jan 16, 2015

alexcrichton commented Jan 20, 2015

steveklabnik commented Apr 18, 2015

alexcrichton commented Apr 19, 2015

alexcrichton commented May 1, 2015