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

Announcing the FFI-unwinding Project Group #2797

Merged
merged 10 commits into from
Dec 12, 2019
Merged

Conversation

BatmanAoD
Copy link
Member

@BatmanAoD BatmanAoD commented Oct 28, 2019

Rendered

This RFC was drafted in collaboration with @nikomatsakis.

@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the RFC. label Oct 28, 2019
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

This RFC doesn't itself make any substantial decisions. It merely codifies the existence of the ffi-unwind "project group" and its basic functioning. This is something that the lang team has already basically agreed to do. Therefore, I'm going to move to merge now. (But people should feel free to leave comments nonetheless!)

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 28, 2019

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Oct 28, 2019
text/0000-project-ffi-unwind.md Outdated Show resolved Hide resolved
text/0000-project-ffi-unwind.md Outdated Show resolved Hide resolved
text/0000-project-ffi-unwind.md Outdated Show resolved Hide resolved
We would like to be able to provide features in stable Rust without a full
formal specification but with an informal statement of intent regarding the
behavior. These features would be considered "unspecified behavior" (rather
than "undefined behavior"), and their behavior would therefore be subject to
Copy link
Contributor

Choose a reason for hiding this comment

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

As worded right now, this sort of assumes that everyone agrees what "unspecified" means (that often turns out not to be the case). I assume this will be detailed and clarified as the project progresses. I hope this will not turn into "implementation defined" or some such.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have indeed written up our own definition, and I think it would be worthwhile to open an RFC to concretely define this terminology.

I think "implementation defined" would be a good addition to the spec-terminology doc.

Copy link
Contributor

@gnzlbg gnzlbg Oct 29, 2019

Choose a reason for hiding this comment

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

An alternative is for "TBD" to be an orthogonal denotation to whatever the behavior class used by the reference is (unspecified, undefined, target-dependent, ...).

One of the consequences of unspecified meaning different things to different people (and different WGs, the UCGs has its own definitions and they are controversial!), is that users might end up confused on whether "unspecified" behavior can become "undefined" in the future or not.

I don't think that's something that's worth resolving right now, and I think that it is in the best interest of the Project Group to have as much freedom as possible. By making it an orthogonal annotation, the Project Group can say that "X is UB (TBD) and Y is target-dependent (TBD)" meaning that X is UB and Y is target dependent, and that the project group is trying to improve both, but while X might remain UB, Y won't become worse than target-dependent (e.g. Y won't become UB, but it might become a well-defined thing on all platforms).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. I appreciate the suggestion, @gnzlbg, but I don't really think that helps with the problem Niko and I were trying to solve by defining/adopting "TBD", which is to have a way of saying, essentially, "this is not UB in the sense that the compiler may optimize based on the assumption that it can't happen; we have not fully specified the behavior, but you can expect it to 'work' approximately as you'd expect, and we promise to further specify the behavior later (though until that time the details of the behavior-as-implemented may change)."

I don't think it would be appropriate to define "unspecified behavior" as part of this "announcement" RFC, but if @Centril (or someone else) would prefer, I can just remove this section entirely (assuming @nikomatsakis agrees that's okay).

Copy link
Contributor

Choose a reason for hiding this comment

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

That would mean that any behavior that we might want to define in some way, and one of the alternatives being considered is defining it as "UB", would need to be UB instead of TBD. If that's the intent, then that's fine with me. Maybe add a sentence mentioning just that ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@BatmanAoD I'm just happy with the knowledge that you'll take into account that I don't think this should become implementation-defined and that the lang team should have the freedom to change the Rust unwind ABI on any platform including with shims if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gnzlbg Yes, I think that's the intent; I would say that nothing can be "TBD" unless it's got an "approximate" or "informal" behavior spec that we don't intend to change. The canonical example is "extern "C unwind" will do 'the right thing' on any platform." That's not well-defined behavior; it's not even implementation defined. But it does express a (hopefully shared) understanding of what code examples should be expected to work, and a statement of intent from the lang team that such code won't break if and when the behavior is further specified. @nikomatsakis would you agree with this explanation?

If so, I can add some verbiage to this effect.

Copy link
Contributor

@nikomatsakis nikomatsakis Oct 30, 2019

Choose a reason for hiding this comment

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

My two cents:

I think it is useful to enumerate our scope and goals and to communicate those goals to people. So I think that distinguishing TBD is very useful, as it communicates that. I think that if we can give people guidelines as to our intent (along the lines that @BatmanAoD sketched -- perhaps more specific) that's even better.

That said, I think we should leave ourselves the freedom to make decisions. I think we can define our intent here as roughly:

  • We intend to make it possible for Rust panics and foreign exceptions to propagate through a "C unwind" boundary in a well-defined and specified way.
  • As a non-exhaustive list, this means that we intend to eventually use cases like the following:
    • a C++ exception unwinding Rust frames, running destructors as it goes
    • a Rust panic unwinding through C++ frames, running destructors as it goes
    • a Rust panic unwinding into C++ and then back into Rust, and being caught via catch_unwind
  • However, there are a number of details whose resolution is not clear. As a non-exhaustive list:
    • what should happen when a C++ exception attempts to unwind through a Rust catch_unwind call?
    • how should longjmp, which is defined on some platforms via unwinding, interact with frames that contain destructors? (And is that even in scope for this group?)

Along those lines, I think we can specify that:

  • For Tier 1 platforms, there will be some translation from Rust panics into the native unwinding mechanism, but we have not yet specified how Rust panics will be described
  • Similarly, we have not specified what the behavior will be when unwinding across a "C unwind" barrier occurs, except for the case where the exception was originally a Rust panic, in which case it is translated back into a Rust panic
    • This implies that we have not specified what happens when catch_unwind is involved, since before catch_unwind gets involved, there must be a "C unwind" barrier.
  • We expect this specification work to be done on a per-platform basis

I'm going to be honest and say that the distinction between "unspecified behavior" and "undefined behavior" feels like a "distinction without a difference" to me, most of the time, unless that unspecified behavior is further qualified to some narrower range of possibilities. I guess I would mostly rather we just don't waste a lot of time (too late...) arguing back and forth on this, as I'm not sure what practical import it has. I'd rather we just spend our time deciding what the behavior ought to be.

text/0000-project-ffi-unwind.md Outdated Show resolved Hide resolved
text/0000-project-ffi-unwind.md Outdated Show resolved Hide resolved
@Centril Centril added A-ffi FFI related proposals. A-panic Panics related proposals & ideas labels Oct 28, 2019
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
BatmanAoD and others added 2 commits October 28, 2019 17:15
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
BatmanAoD and others added 3 commits October 29, 2019 16:44
Co-Authored-By: gnzlbg <gnzlbg@users.noreply.github.com>
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@Centril
Copy link
Contributor

Centril commented Oct 31, 2019

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Oct 31, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 31, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Oct 31, 2019
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 10, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 10, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@nikomatsakis nikomatsakis merged commit 1fe0929 into master Dec 12, 2019
@nikomatsakis
Copy link
Contributor

Merged. There is no tracking issue, but please join the discussion on Zulip (#project-ffi-unwind) or check out the repository https://github.com/rust-lang/project-ffi-unwind

@BatmanAoD BatmanAoD deleted the ProjectFfiUnwind branch December 13, 2019 00:18
@amosonn
Copy link

amosonn commented Jan 11, 2020

Link in description still points to the (now missing) private branch and not to the merged RFC.

@BatmanAoD
Copy link
Member Author

@amosonn Fixed, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ffi FFI related proposals. A-panic Panics related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants