Refactor C FFI variadics to more closely match their C counterparts, and add Clone implementation #59625
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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. |
|
cc @dlrobertson for API review. Also, why does this move the intrinsics to |
|
I also wonder why these can't remain (unsafe) methods on a VaList rather than separate intrinsics. |
That was @dlrobertson's suggestion, after I made the intrinsics public. The original implementation left them in
I guess they could, they would just call out to the intrinsics internally. |
|
I think there is one argument in favor of leaving the lower-level functions as intrinsics: users should generally use the higher-level |
I was not aware of that. I suggested this as I was hoping to "solve" the
Maybe. Definitely worth some experiments. |
|
On Mon, Apr 01, 2019 at 07:04:08PM -0700, Dan Robertson wrote:
> Also, why does this move the intrinsics to core::intrinsics? That was pretty extensively discussed in the RFC thread, and many people didn't care for the idea of intrinsics in core::intrinsics becoming stable at some point. (And these should become stable at some point.)
I was not aware of that. I suggested this as I was hoping to "solve" the `copy` problem by making a `va_copy` intrinsic public.
I don't feel strongly about it either way myself; the current situation
just reflects some rather involved discussions.
|
| }; | ||
| let impl_ty = match inner_ty.sty { |
There was a problem hiding this comment.
You can use ty_adt_def here.
There was a problem hiding this comment.
I considered it myself, but I thought the code winds up about the same (match against Some instead of ty::Adt). I'll change it in the next commit.
| // and store it in the the spoofed `VaList`. | ||
| let layout = self.layout_of(inner_ty); | ||
| let backing = PlaceRef::alloca(self, layout, ""); | ||
| let src = self.load(args[0].immediate(), |
There was a problem hiding this comment.
This looks flipped. In the prior version we load for non-pointer variants and don't load for pointer variants.
There was a problem hiding this comment.
impl_ty being a ty::Adt means we're in a struct variant; for pointer variants, impl_ty is a ty::Foreign. Both inner_ty and impl_ty are retrieved from va_list(), so impl_ty should always match the definition of VaListImpl.
There was a problem hiding this comment.
In other words, ty::Adt for VaListImpl => struct variant => we want a load. Otherwise, ty::Foreign => pointer variant => no load. Did I get this wrong?
There was a problem hiding this comment.
Ah right, this was flipped in the previous version, but that was due to the old signature of va_copy.
|
I'll make some changes and see how far I get, but there might be technical reasons why we can't use methods: the |
|
I added this function to #[inline(always)]
unsafe fn raw_copy(&self) -> Self {
va_copy(self)
}and it gets inlined correctly in the optimized test file ( ; <&mut core::ffi::VaListInner as core::ffi::VaListMethods>::raw_copy
; Function Attrs: alwaysinline nonlazybind uwtable
define internal align 8 dereferenceable(24) %"core::ffi::VaListInner"* @"_ZN79_$LT$$RF$mut$u20$core..ffi..VaListInner$u20$as$u20$core..ffi..VaListMethods$GT$8raw_copy17h78377ab98854a549E"(%"core::ffi::VaListInner"** noalias readonly align 8 dereferenceable(8) %self) unnamed_addr #1 {
start:
%0 = alloca %"core::ffi::VaListInner", align 8
%tmp_ret = alloca %"core::ffi::VaListInner"*, align 8
%1 = bitcast %"core::ffi::VaListInner"** %tmp_ret to i8*
call void @llvm.lifetime.start.p0i8(i64 8, i8* %1)
%2 = load %"core::ffi::VaListInner"*, %"core::ffi::VaListInner"** %self, align 8
store %"core::ffi::VaListInner"* %0, %"core::ffi::VaListInner"** %tmp_ret, align 8
%3 = bitcast %"core::ffi::VaListInner"* %0 to i8*
%4 = bitcast %"core::ffi::VaListInner"* %2 to i8*
call void @llvm.va_copy(i8* %3, i8* %4)
%5 = load %"core::ffi::VaListInner"*, %"core::ffi::VaListInner"** %tmp_ret, align 8, !nonnull !3
%6 = bitcast %"core::ffi::VaListInner"** %tmp_ret to i8*
call void @llvm.lifetime.end.p0i8(i64 8, i8* %6)
br label %bb1
bb1: ; preds = %start
ret %"core::ffi::VaListInner"* %5
}The |
| let intrinsic = self.cx().get_intrinsic(&("llvm.va_copy")); | ||
| self.call(intrinsic, &[llresult, va_list], None); | ||
| if impl_ty.ty_adt_def().is_some() { |
There was a problem hiding this comment.
I switched to ty_adt_def both here and above.
|
The inlining works as expected for optimized builds, like in |
|
@joshtriplett Moving the intrinsics back into
This is getting problematic for technical reasons, see above. Any thoughts on how to solve the |
|
The main blocker for this is to figure out if we can provide method wrappers like #[inline(always)]
unsafe fn raw_copy(&self) -> Self {
va_copy(self)
}cc @rust-lang/compiler |
|
Or we need some other way to ensure that the alloca inside Could someone from @rust-lang/compiler suggest a way to do that? |
|
|
|
Sounds like something else is going on with the inlining, I'll investigate. |
|
According to https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/inline.rs#L45, the I could add a comment to Separately, I discovered that, for some reason, the Playground always performs the inlining. |
|
The The relevant code is here. Playground shows the output of |
|
I think I found the culprit: the test file had |
|
@ahomescu Awesome! |
src/test/codegen/c-variadic-copy.rs
Outdated
| } | ||
|
|
||
| pub unsafe extern "C" fn va_copy_variadic(ap: VaList) { | ||
| let mut ap2 = ap.unsafe_copy(); |
There was a problem hiding this comment.
I added this test that checks that unsafe_copy gets inlined.
|
All tests pass on x86_64 now (including the new ones). |
|
@nagisa The current status is: the inlining always happens, unless both
Any thoughts on how and where to add this? |
|
I disagree with exposing the "raw copy", and think C2Rust should be performing the rewriting necessary (AFAIK it should always be possible EDIT: that is, when not UB). Last time I was shown an example of non-scoped |
|
📌 Commit b9ea653 has been approved by |
|
⌛ Testing commit b9ea653 with merge d3619b53f40f03030deb93c690f87ee3157569d2... |
|
💔 Test failed - checks-travis |
|
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.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 |
|
The error looks spurious. @bors retry |
Refactor C FFI variadics to more closely match their C counterparts, and add Clone implementation We had to make some changes to expose `va_copy` and `va_end` directly to users (mainly for C2Rust, but not exclusively): - redefine the Rust variadic structures to more closely correspond to C: `VaList` now matches `va_list`, and `VaListImpl` matches `__va_list_tag` - add `Clone` for `VaListImpl` - add explicit `as_va_list()` conversion function from `VaListImpl` to `VaList` - add deref coercion from `VaList` to `VaListImpl` - add support for the `asmjs` target All these changes were needed for use cases like: ```Rust let mut ap2 = va_copy(ap); vprintf(fmt, ap2); va_end(&mut ap2); ```
|
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@605ea9d. Direct link to PR: <rust-lang/rust#59625> 🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
|
Thanks everyone! |
|
I think this change broke Rust again on sparc64 although I'm not sure yet as I haven't done any bisecting: Do we need to add sparc64 to the arch exclusion lists above (and s390x etc)? |
|
This PR removed the |
|
@jrtc27 Okay, you're right. I forgot to perform a proper |
We had to make some changes to expose
va_copyandva_enddirectly to users (mainly for C2Rust, but not exclusively):VaListnow matchesva_list, andVaListImplmatches__va_list_tagCloneforVaListImplas_va_list()conversion function fromVaListImpltoVaListVaListtoVaListImplasmjstargetAll these changes were needed for use cases like: