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

other-reprs: Null-pointer-optimized enums are FFI safe unless repr(C) #13

Merged
merged 1 commit into from May 26, 2017

Conversation

Projects
None yet
4 participants
@geofft
Copy link
Contributor

geofft commented Apr 18, 2017

See also the improper_ctypes lint, specifically is_repr_nullable_ptr in
src/librustc_lint/types.rs.

@geofft

This comment has been minimized.

Copy link
Contributor Author

geofft commented Apr 18, 2017

Actually the more I read the code, it looks like the improper_ctypes lint considers it FFI-safe to have an enum with #[repr(C)] or #[repr(uN)], as long as all variants contain FFI-safe data. Presumably the layout is effectively struct SomeEnum {uintN_t discriminant; union { ... } data;}; or so.

But the nomicon is claiming that "tagged unions ... are never FFI safe." Should this text be fixed, or is "tagged union" intended to mean something other than an enum with data?

@ubsan

This comment has been minimized.

Copy link

ubsan commented Apr 30, 2017

They aren't yet FFI-safe, but they should probably be made so.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented May 15, 2017

Enums are tagged unions, so this feels a bit weird, I would phrase it as something like "adds the tag back in" or something, dunno.

@Gankro

This comment has been minimized.

Copy link
Collaborator

Gankro commented May 16, 2017

This is way too much information to squish into a single-sentence aside.

To be clear, OptionLike<u32> isn't FFI-safe, but OptionLike<&T> absolutely must be (same repr as *const T). It's possible OptionLike<Box<T>> could be too, but it's unclear to me whether FFI'ing boxes is a very good idea.

@ubsan

This comment has been minimized.

Copy link

ubsan commented May 17, 2017

@Gankro I have used it in the past. It's useful for passing ownership to and from C (you return a Box<YourType> from rust, then take &YourTypes in all the functions, then have a dropper function which takes Box<YourType>. From C, it all looks like YourType [const]*, where YourType is an opaque type in C)

other-reprs: `Option` is FFI-safe, even though it's an enum
See also the improper_ctypes lint, specifically is_repr_nullable_ptr in
src/librustc_lint/types.rs.

@geofft geofft force-pushed the geofft:null branch from caaea4d to 7753a3c May 20, 2017

@geofft

This comment has been minimized.

Copy link
Contributor Author

geofft commented May 20, 2017

They aren't yet FFI-safe, but they should probably be made so.

@ubsan What needs to be done to make this happen?

In particular, if they're not yet FFI-safe, then it seems to me that the improper_ctypes lint should be fixed to align with the current state of things.

To be clear, OptionLike<u32> isn't FFI-safe, but OptionLike<&T> absolutely must be (same repr as *const T). It's possible OptionLike<Box<T>> could be too, but it's unclear to me whether FFI'ing boxes is a very good idea.

FWIW the improper_ctypes lint currently warms about both Option<i32> and Option<Box<i32>>, but not about Option<&i32>.

I've pushed some slightly longer text that phrases this as Option being a specific special case. ("subject to the null pointer optimization" and "is defined like Option, including repr(rust)" are synonyms, right?) I agree that making Box FFI-safe is useful, but it seems reasonable not to promise that yet.

@ubsan

This comment has been minimized.

Copy link

ubsan commented May 21, 2017

@geofft basically, define the layout of

#[repr(INT_TYPE)]
enum Foo {
  T0(...),
  T1(...),
   ...
}

to be

struct Foo {
  enum class Tag: INT_TYPE {
    T0,
    T1,
    ...
  } tag;
  union {
    struct { ... } T0;
    struct { ... } T1;
    ...
  };
};

or something of that order

@geofft

This comment has been minimized.

Copy link
Contributor Author

geofft commented May 21, 2017

I suspect that's what its representation is right now - what needs to be done process-wise to formalize this? Would this be an RFC or something?

@ubsan

This comment has been minimized.

Copy link

ubsan commented May 22, 2017

@geofft not sure. Probably an RFC, but also I'd like to note that the representation of the specific union members isn't specified, so you'd have to figure that out as well.

@steveklabnik steveklabnik merged commit 95e43bc into rust-lang-nursery:master May 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented May 26, 2017

Thanks!

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.