[WIP] Enable va_arg for raw pointers to unsized types

[ahomescu]

This patch implements VaArgSafe for raw pointers to unsized types, which lets users call VaList::arg for such pointers. Everything else in libcore uses T: ?Sized for raw pointers, so VaList should too, and we already ran into a case where we could use this for C2Rust.

r? @joshtriplett
cc @dlrobertson

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (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. Due to 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 the contribution instructions for more information.

[dlrobertson]

Good catch. Could you add a ui test for this?

[ahomescu]

Could you add a ui test for this?

Would src/test/ui/c-variadic be the place for that? Edit: most of the files in that directory seem to check for errors.

[ahomescu]

I added a "pointer to foreign type" test to checkrust.rs.

[ghost]

Wouldn't this would make ap.arg::<*const [u8]>() legal? Isn't that bad?

[joshtriplett]

On Sat, May 25, 2019 at 02:34:33AM -0700, whataloadofwhat wrote: Wouldn't this would make `ap.arg::<*const [u8]>()` legal? Isn't that bad?

That's fine; you're just taking a pointer from the VaArgs, and that pointer can point to anything.

[ghost]

*const [u8] isn't just a pointer though, it's a pointer and a length.

[ahomescu]

Wouldn't this would make ap.arg::<*const [u8]>() legal? Isn't that bad?

Right, *const [u8] is a fat pointer, which shouldn't be legal for arg. I'm not sure how to implement this additional restriction, if it can even be done.

Pointers-to-slices and pointers-to-traits are currently the only 2 kinds of fat pointers in Rust, afaik.

[ahomescu]

My latest update is that I have an idea how to fix this using auto traits (basically making VaArgSafe an auto trait), but I have to do some experiments to see if it works.

[ahomescu]

After looking into auto traits for a bit, I came to the conclusion that VaArgSafe should probably be an auto trait, with the following consequences:

• Support for negative implementations, which would let us disable it for slices and/or pointers to slices, e.g., impl<T> !VaArgSafe for *const [T]
• Implicit support for calling VaList::arg on structures. I originally thought C doesn't support this either, but it turns out that both gcc and clang do support this.
• Support for calling VaList::arg on unions, enums, Rust tuples, closures and other Rust-specific data types. These ideally wouldn't be allowed, but I haven't been able to figure out how to disable them.

The other kind of fat pointers in Rust are pointers-to-traits, i.e., *const dyn Foo, and I'm not sure how to disable the auto trait for those.

[dlrobertson]

@ahomescu does this enable the use of VaList::arg with aggregate types? Also how would this work with #62207?

[ahomescu]

does this enable the use of VaList::arg with aggregate types?

It actually does, recursively (if all members of the aggregate implement the trait, then the aggregate does too). However, it still doesn't the original problem of allowing fat pointers.

[ahomescu]

Also how would this work with #62207?

Adding a function to VaArgSafe precludes making it an auto-trait, but maybe we could solve that by splitting it into 2 traits: the auto VaArgSafe and the actual VaArgImpl.

[dlrobertson]

Adding a function to VaArgSafe precludes making it an auto-trait, but maybe we could solve that by splitting it into 2 traits: the auto VaArgSafe and the actual VaArgImpl.

that seems reasonable

It actually does, recursively

AFAIK I hit some strange errors on windows with aggregate types. If we use auto trait we'd want to start testing aggregate types, not sure if it works now or not.

[rholderfield]

ping from triage... @ahomescu thank you for your time, can you please provide a status?

[ahomescu]

@rholderfield This is blocked until I figure out a way to disable the VaArgSafe trait for pointers to DSTs, e.g., *const [u8] mentioned above by @whataloadofwhat. AFAIK, the language doesn't currently have a way to disable marker traits for fat pointers and references.

The change from a regular trait to an auto trait also enables VaArgSafe automatically for structures. If that's desirable, then the PR could go in as is, and we solve the DST issue later.
I'll defer to @joshtriplett and @dlrobertson on this.

[ahomescu]

This RFC could help us, since it would let us implement VaArgSafe only on ?Sized + Thin pointers.

[hdhoang]

ping from triage @joshtriplett and @dlrobertson, could you comment on the author's concern above?

[JohnCSimon]

pinging agan from triage @joshtriplett and @dlrobertson @ahomescu

could you comment on the author's concern above?

[dlrobertson]

This RFC could help us, since it would let us implement VaArgSafe only on ?Sized + Thin pointers.

Yeah RFC 2580 would be perfect for resolving this issue.

[ahomescu]

I added a source code comment on RFC 2580. In the meantime, we could either wait for that to be implemented, or keep working on this PR as-is.

[ahomescu]

I decided to submit a separate PR for the auto-trait along with more tests, so I removed it from this one.

The fix for the *const [u8] issue is blocked on RFC 2580, but everything else works.

@joshtriplett Do you want to approve this PR for now (and fix the issue later), or wait?

[joshtriplett]

@ahomescu So the current state is that you can't have pointers to unsized types at all (and thus can't accidentally treat a non-thin pointer as FFI-safe/va_arg-safe), and this PR would change that so that you can have pointers to unsized types (which would allow valid things as well as invalid things)? Or have I misunderstood?

[ahomescu]

@ahomescu So the current state is that you can't have pointers to unsized types at all (and thus can't accidentally treat a non-thin pointer as FFI-safe/va_arg-safe), and this PR would change that so that you can have pointers to unsized types (which would allow valid things as well as invalid things)? Or have I misunderstood?

That basically covers it. The original motivation was to enable VaArgSafe for pointers to opaque FFI (extern) types, which are thin pointers. Once RFC 2580 is implemented, we can actually do that.

[joshtriplett]

@ahomescu It's not clear to me that we need all of RFC 2580; don't we just want to say that thin pointers are FFI-safe and non-thin pointers aren't?

Also, I don't think we should enable something that makes it easy for people to do the wrong thing here, if we have a means of distinguishing the cases instead and only allowing thin pointers.

[ahomescu]

don't we just want to say that thin pointers are FFI-safe and non-thin pointers aren't?

Do you mean for improper_ctypes? That's an interesting idea, I'm not sure how that interacts with VaListImpl::arg(). The context here is that we want to allow someone to do ap.arg::<*const Foo>() for an opaque type Foo. Would improper_ctypes even check this?

Edit: Alternatively, it would be great to somehow say "VaArgSafe is only implemented for FFI-safe types" or something similar for VaListImpl::arg.

[joshtriplett]

On Mon, Sep 09, 2019 at 01:33:26PM -0700, ahomescu wrote: > don't we just want to say that thin pointers are FFI-safe and non-thin pointers aren't? Do you mean for `improper_ctypes`? That's an interesting idea, I'm not sure how that interacts with `VaListImpl::arg()`. The context here is that we want to allow someone to do `ap.arg::<*const Foo>()` for an opaque type `Foo`. Would `improper_ctypes` even check this?

I'm not sure.

[ahomescu]

@joshtriplett I checked the implementation for improper_ctypes, and it's just a lint that checks that the input and output types for each foreign function are FFI-safe, but with its own implementation of whether each type is FFI-safe.

What I could try to do is replace VaArgSafe with a FfiSafe trait that the compiler auto-magically implements for all FFI-safe types (kinda like Sized), using the implementation from the lint. VaListImpl::arg() would then be implemented for FfiSafe, which would cover the opaque type case and many more.
However, this would turn FfiSafe into a magic trait, is that desirable? Also, is this new trait something that would be useful for other users?

[joshtriplett]

On Mon, Sep 09, 2019 at 02:47:29PM -0700, ahomescu wrote: @joshtriplett I checked the implementation for `improper_ctypes`, and it's just a lint that checks that the input and output types for each foreign function are FFI-safe, but with its own [implementation](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/types.rs#L600) of whether each type is FFI-safe. What I could try to do is replace `VaArgSafe` with a `FfiSafe` trait that the compiler auto-magically implements for all FFI-safe types (kinda like `Sized`), using the implementation from the lint. `VaListImpl::arg()` would then be implemented for `FfiSafe`, which would cover the opaque type case and many more. However, this would turn `FfiSafe` into a magic trait, is that desirable? Also, is this new trait something that would be useful for other users?

Seems like it'd be quite useful, yes. That probably needs some broader discussion, but I'd like to use *exactly* the same list of types.

[ahomescu]

@joshtriplett @dlrobertson I discovered something interesting: some of the FFI-unsafe types are not supported by the backend anyway, I added a new UI test with the following:

pub unsafe extern "C" fn vararg_unsafe_types(_: usize, mut ap: ...) {
    let _ = ap.arg::<*const str>();
    let _ = ap.arg::<*const [u8]>();
}

and got an ICE when running the test:

error: internal compiler error: src/librustc_codegen_llvm/intrinsic.rs:178: the va_arg intrinsic does not work with non-scalar types

I've also been looking into the implementation of the improper_ctypes lint, I'm starting to think I might be able to modify it to check the self argument of VaListImpl::arg() for FFI-safety.

[dlrobertson]

That is an interesting ICE. You should post an issue for it. I wonder what the values Abi is if it isn't Scalar?

[bjorn3]

@dlrobertson It should be ScalarPair, as they are fat pointers.

[hdhoang]

ping from triage @ahomescu, any update on this PR or the ICE above?

[ahomescu]

@dlrobertson @bjorn3 That ICE only occurs if I enable VaArgSafe for fat pointers, and it makes sense because va_arg isn't implemented in the backend for ScalarPair. rustc on master doesn't get that ICE because you can't read a fat pointer through VaListImpl::arg().

@hdhoang I've looked into possible ways to implement VaArgSafe exclusively for FFI-safe types, and can't see a simple and clean way to do it. I'd be OK with closing this PR for now. I might revisit it later when RFC 2580 goes in.

[JohnCSimon]

Ping from triage:
@ahomescu Sorry, I can't tell if the status of RFC 2580... what would you like to do with this PR?
Thanks

[ahomescu]

@JohnCSimon I'm fine with closing it for now.

[Dylan-DPC-zz]

Closing this due to inactivity. Thanks for making the effort to contribute and when you want to work on this, you can make a new PR.