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

Nullable-pointer Options aren't FFI-compatible with the base pointer types. #11303

Closed
jld opened this issue Jan 4, 2014 · 8 comments
Closed
Labels
A-codegen Area: Code generation A-FFI Area: Foreign function interface (FFI) O-x86_32 Target: x86 processors, 32 bit (like i686-*) P-medium Medium priority

Comments

@jld
Copy link
Contributor

jld commented Jan 4, 2014

#[inline(never)]
extern "C" fn foo(x: ~int) -> Option<~int> { Some(x) }

pub fn main() {
  unsafe {
    let f: extern "C" fn(~int) -> ~int = ::std::cast::transmute(foo);
    assert_eq!(*f(~0xDEADBEE), 0xDEADBEE);
  }
}

Works on x86_64; crashes on i686. The Rust struct or enum gets an LLVM struct wrapped around it, which changes the ABI on 32-bit x86 if the type is used as a return type. This is an unpleasant surprise for projects using the FFI to represent nullable pointers from C, and there isn't an immediately obvious workaround.

But also:

struct AlmostInt(int);

#[inline(never)]
extern "C" fn foo(x: int) -> AlmostInt { AlmostInt(x) }

pub fn main() {
  unsafe {
    let f: extern "C" fn(int) -> int = ::std::cast::transmute(foo);
    assert_eq!(f(0xDEADBEE), 0xDEADBEE);
  }
}

Same thing. But we probably don't want one-field structs to deviate from the ABI for the equivalent C struct, and the newtype case can (I think?) just use a wrapper to do the conversion instead. This yields the slightly counterintuitive result that adjoining a value to a type makes its representation slightly more efficient (option returned in register vs. newtype returned on stack), but we might be willing to live with that.

See also #10570; cc @bjz @rlane

@brson
Copy link
Contributor

brson commented Jan 8, 2014

I am a bit worried about what promises we can make regarding Option's compatibility with C. Option's representation is so dependent upon it's contents, and I'm seeing FFI user's want to deal with various other different Option types from C. Personally, I do not think it's a best practice to use Rust pointers in the FFI and always convert to unsafe pointers. I would not be opposed to keeping Option a struct and telling people to leave it out of the FFI.

@jld
Copy link
Contributor Author

jld commented Jan 10, 2014

One problem is that the ctypes lint currently allows all non-C-like enums, which was basically a compromise to get the enum discriminant size patches to finally land. If we can fix that — require an FFI-safe discriminant, or being a single-field nullable of an otherwise FFI-safe type, and I think that's it — then that would flag using Option<i32> or whatever in FFI.

@jld
Copy link
Contributor Author

jld commented Jan 10, 2014

I have a patch that almost works, but there's this, from src/test/bench/task-perf-alloc-unwind.rs:

enum UniqueList {
    ULNil, ULCons(~UniqueList)
}

That's a nullable pointer to itself, which sends type_of into infinite recursion trying to represent it without a struct. (It's also isomorphic to uint, but ignore that.) I think I can fix this, though — general enums have to be PointerCast to access anything besides the discriminant, and this isn't much different.

@brson
Copy link
Contributor

brson commented Feb 17, 2014

Nominating. Although we aren't trying to make any promises about Rust's ABI, this is still pretty important to get right.

@nikomatsakis
Copy link
Contributor

@brson this seems to have more to do with the treatment of non-C types in extern "C" fn signatures than Rust's ABI per se, right? That is, we should certainly define what it means to put such types in C function signatures (or forbid it, which is of course an option).

@pnkfelix
Copy link
Member

This is an important issue. We need to be a little concerned about over-committing too many guarantees about our FFI and the ABI and data representations / compatibility in 1.0.

In any case, right now we are not going to guarantee that we will solve this particular problem for 1.0.

P-high, but not 1.0.

@alexcrichton
Copy link
Member

Questions about whether this is valid today or whether we want to support this in the future may be better directed at #12608 now.

bors added a commit that referenced this issue May 18, 2014
This slightly adjusts the NullablePointer representation for some enums in the case where the non-nullable variant has a single field (the ptr field) to be just that, the pointer. This is in contrast to the current behaviour where we'd wrap that single pointer in a LLVM struct.

Fixes #11040 & #11303.
@luqmana
Copy link
Member

luqmana commented May 23, 2014

Fixed by #14121.

@luqmana luqmana closed this as completed May 23, 2014
@Noratrieb Noratrieb added O-x86_32 Target: x86 processors, 32 bit (like i686-*) and removed O-x86-all labels Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-FFI Area: Foreign function interface (FFI) O-x86_32 Target: x86 processors, 32 bit (like i686-*) P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

7 participants