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

Allow null-pointer-optimized enums in FFI if their underlying representation is FFI safe #60300

Merged
merged 1 commit into from May 23, 2019

Conversation

@mjbshaw
Copy link
Contributor

commented Apr 26, 2019

I'm not sure if this requires an RFC. I attempted to start a discussion on internals.rust-lang.org and when no one really objected I figured I'd go ahead and try implementing this.

This allows types like Option<NonZeroU8> to be used in FFI without triggering the improper_ctypes lint. This works by changing the is_repr_nullable_ptr function to consider an enum E to be FFI-safe if:

  • E has no explicit #[repr(...)].
  • It only has two variants.
  • One of those variants is empty (meaning it has no fields).
  • The other variant has only one field.
  • That field is one of the following:
    • &T
    • &mut T
    • extern "C" fn
    • core::num::NonZero*
    • core::ptr::NonNull<T>
    • #[repr(transparent)] struct wrapper around one of the types in this list.
  • The size of E and its field are both known and are both the same size (implying E is participating in the nonnull optimization).

This logic seems consistent with the Rust nomicon.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

r? @estebank

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

@Centril

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Show resolved Hide resolved src/librustc_lint/types.rs Outdated
Show resolved Hide resolved src/librustc_lint/types.rs Outdated

@Centril Centril added the I-nominated label Apr 26, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@rkruppe

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

I do think we should change the lint to accepting types that are already effectively guaranteed to participate in Option layout optimizations (at minimum, the NonZero* types and repr(transparent) newtypes). However, there are also situations where rustc does apply layout optimizations but we don't obviously (i.e., without an RFC) want to commit to those optimizations.

Relatedly, there may be repr(C) library-defined types which happen to have a niche in a private field but whose author didn't intend to guarantee that Option<Foo> is FFI-safe. For example, a library author may at first use bool for a private field but later decide to use uint8_t to be defensive against the C side storing a byte other than 0 or 1, without expecting this to break clients.

So I think the check "Option<T> is the same size as T" that this PR currently implements is too broad. I would be more comfortable to have us start out with something more restrictive. I am not sure what that should be, though -- that uncertainty might be why this generalization hasn't happened yet.

@mjbshaw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@Centril Thanks; I've made the changes you recommended. Let me know if there are others.

@rkruppe I agree. I've revised the code to now only allow:

  • &T and &mut T
  • extern "C" fn
  • core::num::NonZero*
  • core::ptr::NonNull<T>
  • #[repr(transparent)] struct wrapper around one of the types in this list.

I'll update the top-level comment to reflect this change.

Show resolved Hide resolved src/librustc_lint/types.rs
Show resolved Hide resolved src/librustc_lint/types.rs
Show resolved Hide resolved src/librustc_lint/types.rs Outdated
Show resolved Hide resolved src/librustc_lint/types.rs Outdated
@bors

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

☔️ The latest upstream changes (presumably #60435) made this pull request unmergeable. Please resolve the merge conflicts.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented May 2, 2019

The idea of this change looks great to me. We want to define this behavior, carefully, and not leave it undefined. And this allows people to define safer types for FFI functions.

We do need further review on the code itself, but let's get consensus on the concept:

@rfcbot merge

@rfcbot

This comment has been minimized.

Copy link

commented May 2, 2019

Team member @joshtriplett 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.

@scottmcm

This comment has been minimized.

Copy link
Member

commented May 2, 2019

👍 now that this is the "careful" version (following #60300 (comment))

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 3, 2019

... why isn't there the standard list of checkboxes for all members of labelled teams (T-lang in this case) on the rfcbot comment

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0b529774:start=1557164028516611654,finish=1557164116066617906,duration=87550006252
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:14:49]    Compiling rustc_mir v0.0.0 (/checkout/src/librustc_mir)
[00:14:49]    Compiling rustc_typeck v0.0.0 (/checkout/src/librustc_typeck)
[00:14:49]    Compiling rustc_allocator v0.0.0 (/checkout/src/librustc_allocator)
[00:20:20]    Compiling rustc_lint v0.0.0 (/checkout/src/librustc_lint)
[00:20:20] error: expected one of `::`, `=`, or `|`, found `!=`
[00:20:20]     |
[00:20:20]     |
[00:20:20] 535 |             if let AdtKind::Struct != field_def.adt_kind() {
[00:20:20]     |                                    ^^ expected one of `::`, `=`, or `|` here
[00:20:21]    Compiling rustc_traits v0.0.0 (/checkout/src/librustc_traits)
[00:20:22] error: aborting due to previous error
[00:20:22] 
[00:20:22] error: Could not compile `rustc_lint`.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@pnkfelix perhaps it was slow -- the list exists now

@rfcbot

This comment has been minimized.

Copy link

commented May 9, 2019

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

@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

☔️ The latest upstream changes (presumably #60630) made this pull request unmergeable. Please resolve the merge conflicts.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

One of those variants is empty.

Here empty means that the variant has no fields, right? That is, variants with zero-sized fields are not empty, and, for example, Result<(), NonZeroU32> still is an improper ctype, right? If so, is there a test covering this?


EDIT: many C APIs return an error code of type int that is zero on success and non-zero on error, so the Result<(), NonZeroCInt> case is something that users might attempt to use in C FFI directly. However, we currently do not guarantee any niche optimizations for Result-like types, so it makes IMO sense to add an ui test that checks that the improper ctypes lint fails for these types until we start guaranteeing them.

@mjbshaw

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@gnzlbg Good idea. I added one. And yes, "empty" means "no fields". I'll clarify the top post to mention that.

@rkruppe I appreciate your review from earlier, but there are a couple items left I'm not sure how to address. Should I just try changing tcx.normalize_erasing_regions(...) to field.ty(cx, substs) and test if it works, and if so, assume it's enough?

Show resolved Hide resolved src/librustc_lint/types.rs Outdated
@rkruppe

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Should I just try changing tcx.normalize_erasing_regions(...) to field.ty(cx, substs) and test if it works, and if so, assume it's enough?

I think that changing the reveal mode would be good (dropping the normalization probably isn't), but it would need more thorough vetting (e.g., should have a reviewer who actually understands normalization and reveal modes) and I don't want to hold up this PR that long. I'll file an issue for it, and if you fix my last nit then r=me when FCP ends.

@rfcbot

This comment has been minimized.

Copy link

commented May 19, 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.

@rkruppe

This comment has been minimized.

Copy link
Member

commented May 19, 2019

if you fix my last nit then r=me when FCP ends.

Sorry to only notice now, but before merging there are some procedural issues to resolve:

  • this branch has merge commits, that's generally discouraged, please rebase (and while at it, you could squash some back-and-forth in the history, but no big deal)
  • the new rustc_* attribute doesn't have a feature gate test, please add one
@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 22, 2019

switching status to S-waiting-on-author to reflect the need to address the changes noted by @rkruppe above.

@mjbshaw mjbshaw force-pushed the mjbshaw:ffi_types branch from fb23b1d to 52348db May 22, 2019

Allow null-pointer-optimized enums in FFI if their underlying represe…
…ntation is FFI safe

This allows types like Option<NonZeroU8> to be used in FFI without triggering the improper_ctypes lint. This works by changing the is_repr_nullable_ptr function to consider an enum E to be FFI-safe if:

- E has no explicit #[repr(...)].
- It only has two variants.
- One of those variants is empty (meaning it has no fields).
- The other variant has only one field.
- That field is one of the following:
  - &T
  - &mut T
  - extern "C" fn
  - core::num::NonZero*
  - core::ptr::NonNull<T>
  - #[repr(transparent)] struct wrapper around one of the types in this list.
- The size of E and its field are both known and are both the same size (implying E is participating in the nonnull optimization).

@mjbshaw mjbshaw force-pushed the mjbshaw:ffi_types branch from 52348db to a31dc8e May 22, 2019

@mjbshaw

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

this branch has merge commits, that's generally discouraged, please rebase (and while at it, you could squash some back-and-forth in the history, but no big deal)

Thanks, I wasn't sure what Rust's policy was on this. I'll keep that in mind going forward (also: done).

the new rustc_* attribute doesn't have a feature gate test, please add one

Done.

@rkruppe

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Thank you for your work and patience!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

📌 Commit a31dc8e has been approved by rkruppe

@Centril Centril added this to the 1.37 milestone May 22, 2019

@Centril Centril added the relnotes label May 22, 2019

Centril added a commit to Centril/rust that referenced this pull request May 22, 2019

Rollup merge of rust-lang#60300 - mjbshaw:ffi_types, r=rkruppe
Allow null-pointer-optimized enums in FFI if their underlying representation is FFI safe

I'm not sure if this requires an RFC. I attempted to start [a discussion on internals.rust-lang.org](https://internals.rust-lang.org/t/options-ffi-safety-and-guarantees-for-abi-compatibility-with-nonnull-optimizations/9784) and when no one really objected I figured I'd go ahead and try implementing this.

This allows types like `Option<NonZeroU8>` to be used in FFI without triggering the `improper_ctypes` lint. This works by changing the `is_repr_nullable_ptr` function to consider an enum `E` to be FFI-safe if:

- `E` has no explicit `#[repr(...)]`.
- It only has two variants.
- One of those variants is empty (meaning it has no fields).
- The other variant has only one field.
- That field is one of the following:
  - `&T`
  - `&mut T`
  - `extern "C" fn`
  - `core::num::NonZero*`
  - `core::ptr::NonNull<T>`
  - `#[repr(transparent)] struct` wrapper around one of the types in this list.
- The size of `E` and its field are both known and are both the same size (implying `E` is participating in the nonnull optimization).

This logic seems consistent with [the Rust nomicon](https://doc.rust-lang.org/nomicon/repr-rust.html).

bors added a commit that referenced this pull request May 22, 2019

Auto merge of #61044 - Centril:rollup-ztsgb9p, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #60300 (Allow null-pointer-optimized enums in FFI if their underlying representation is FFI safe)
 - #60773 (Always try to project predicates when finding auto traits in rustdoc)
 - #60809 (Add FAQ for NLL migration)
 - #61023 (Migrate from recursion to iterate on qualify consts visitor impl)
 - #61029 (Simplify RefCell minimum_spanning_tree example)
 - #61030 (Make maybe_codegen_consume_direct iterate instead of doing recursion)
 - #61034 (rustc_metadata: parametrize schema::CrateRoot by 'tcx and rip out old unused incremental infra.)
 - #61037 (Update clippy submodule)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request May 22, 2019

Auto merge of #61044 - Centril:rollup-ztsgb9p, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #60300 (Allow null-pointer-optimized enums in FFI if their underlying representation is FFI safe)
 - #60773 (Always try to project predicates when finding auto traits in rustdoc)
 - #60809 (Add FAQ for NLL migration)
 - #61023 (Migrate from recursion to iterate on qualify consts visitor impl)
 - #61029 (Simplify RefCell minimum_spanning_tree example)
 - #61030 (Make maybe_codegen_consume_direct iterate instead of doing recursion)
 - #61034 (rustc_metadata: parametrize schema::CrateRoot by 'tcx and rip out old unused incremental infra.)
 - #61037 (Update clippy submodule)

Failed merges:

r? @ghost

@bors bors merged commit a31dc8e into rust-lang:master May 23, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.