libcore: Add VaList and variadic arg handling intrinsics #49878
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
nagisa
reviewed
Apr 12, 2018
src/librustc_back/lib.rs
Outdated
| AArch64Abi, | ||
| PowerPcAbi, | ||
| X86_64Abi, | ||
| None |
There was a problem hiding this comment.
Is this variant just a placeholder for targets which haven’t specified their VaList ABI? Removing this variant and picking some default would would end up removing the bug branch from trans.
There was a problem hiding this comment.
The void or char pointer could be a sensible default.
There was a problem hiding this comment.
Is this variant just a placeholder for targets which haven’t specified their VaList ABI?
I don't believe we support one that doesn't specify a VaList ABI, and I can't think of one off the top of my head that hasn't specified their VaList ABI, so unless someone else can think of a reason to keep the none variant, I'll remove it and use void-ptr or char-ptr as the default.
src/librustc_trans/type_.rs
Outdated
| }, | ||
| _ => { | ||
| // TODO: Is this the right thing to do? | ||
| bug!("va_list: not supported for this target type"); |
There was a problem hiding this comment.
Yes, bug-ing out here is the right thing to do as the valid variants are filtered out earlier. Message could be made to point out the issue better, though:
unexpected va_list kind reached trans
for example.
There was a problem hiding this comment.
👍 much better. I'll do a bit more research, but I think defaulting to either a void pointer or char pointer would be reasonable.
I'll be in airports all weekend, but I can try to get the commit with |
dlrobertson
force-pushed
the
va_list_pt0
branch
from
361cdee
to
b595ab0
Compare
|
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 |
dlrobertson
force-pushed
the
va_list_pt0
branch
from
b595ab0
to
61bf322
Compare
|
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 |
dlrobertson
force-pushed
the
va_list_pt0
branch
from
61bf322
to
f0e2318
Compare
|
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 latest upstream changes (presumably #50093) made this pull request unmergeable. Please resolve the merge conflicts. |
dlrobertson
force-pushed
the
va_list_pt0
branch
from
f0e2318
to
5198f5a
Compare
|
The job 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 |
dlrobertson
force-pushed
the
va_list_pt0
branch
from
5198f5a
to
638ff83
Compare
|
☔ The latest upstream changes (presumably #50228) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage @eddyb! This PR needs your review. |
|
@pietroalbini I can push up a rebased branch shortly. @nagisa @eddyb I'm still working on the actual |
|
Feel free to push whenever you have any changes. It is easier to stay up-to-date with the state of the PR that way and it might avoid the pings from triage. |
eddyb
reviewed
Apr 30, 2018
src/librustc_back/lib.rs
Outdated
| VoidPtr, | ||
| AArch64Abi, | ||
| PowerPcAbi, | ||
| X86_64Abi, |
There was a problem hiding this comment.
I wonder why this enum was added, instead of determining this information from the architecture, in the call ABI (formerly src/librustc_trans/{abi,cabi_*}.rs, now in src/librustc_target/abi/call/*.rs) infrastructure.
There was a problem hiding this comment.
Is there value in custom target specifications overriding it? Currently we don't support overriding most of the behavior of the C call ABI for a target.
There was a problem hiding this comment.
My main motivation for using this approach was because it more closely mirrored clang's implementation. Other than that, it makes windows a bit easier. Windows may be x86_64, but the va_list implementation is the CharPtr variant. That being said, I think we could use is_windows_like and the architecture.
There was a problem hiding this comment.
Yeah, we already special-case windows in call ABI code. Also, I'm curious what it affects, in terms of the final implementation - is it just required to be something that only LLVM operates on, or would rustc_trans also have to generate code looking at its fields etc.?
There was a problem hiding this comment.
I'm honestly not entirely sure yet. I'm still a rustc noobie, and I'm still reading through much of the trans code. I was hoping that rustc_trans::Type::va_list could be the only function that needed this enum, but the VaList argument would get eliminated as an argument due to the core implementation being a ZST.
I was also hoping that we could generate better debug info for architectures like x86_64 and Aarch64, but I haven't gotten that far yet.
|
Sorry about the delay, I wish I had read through this sooner - had I said anything about #49878 (comment) sooner, conflicts with #50228 would've been avoided. |
Np. Nothing a little vimdiff can't fix |
dlrobertson
force-pushed
the
va_list_pt0
branch
from
638ff83
to
a6876b0
Compare
dlrobertson
commented
Apr 30, 2018
src/libcore/va_list.rs
Outdated
| reason = "dlrobertson is still working on this", | ||
| issue = "27745")] | ||
| #[derive(Debug)] | ||
| pub struct VaList; |
There was a problem hiding this comment.
The original RFC indicates that this should be implemented as core::intrinsics::VaList, but since everything in intrinsics is a "rust-intrinsic" I implemented it as core::va_list::VaList. Was there a reason this was going to be implemented under intrinsics?
src/librustc/ty/context.rs
Outdated
| @@ -2694,6 2695,34 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
| self.object_lifetime_defaults_map(id.owner) | |||
| .and_then(|map| map.get(&id.local_id).cloned()) | |||
| } | |||
|
|
|||
| pub fn va_list_types(&self) -> Vec<Ty<'tcx>> { | |||
There was a problem hiding this comment.
Used to get the Tys of the VaList fields. I'm still working on figuring out what to do here for the pointer variants. I'm not sure if the current implementation works for them. My first attempt only used Type::va_list, but the VaList function argument would get optimized out as a ZST, so I added extra logic here so that the LayoutDetails indicate that the VaList is not a ZST. Comments and critiques on this would be super helpful.
src/librustc/ty/layout.rs
Outdated
| let variants = tcx.va_list_types().iter() | ||
| .map(|x| self.layout_of(x).unwrap()) | ||
| .collect::<Vec<_>>(); | ||
| univariant_uninterned(&variants[..], &def.repr, kind)? |
There was a problem hiding this comment.
I don't think this is entirely correct. I think this will not be correct for the CharPtr and VoidPtr variants.
src/librustc/ty/layout.rs
Outdated
| @@ -1614,6 1624,13 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx> | |||
| this.ty.simd_type(tcx) | |||
| } | |||
|
|
|||
| ty::TyAdt(def, ..) if Some(def.did) == tcx.lang_items().va_list() => { | |||
There was a problem hiding this comment.
The VaList implementation in core is just a stub. When compiling for X86_64, any access to field will result in an out of bounds index.
src/librustc_trans/type_.rs
Outdated
| @@ -297,4 298,34 @@ impl Type { | |||
| pub fn x86_mmx(cx: &CodegenCx) -> Type { | |||
| ty!(llvm::LLVMX86MMXTypeInContext(cx.llcx)) | |||
| } | |||
|
|
|||
| pub fn va_list(cx: &CodegenCx, name: &str) -> Type { | |||
There was a problem hiding this comment.
Function used to generate the correct type.
dlrobertson
force-pushed
the
va_list_pt0
branch
from
a6876b0
to
da46325
Compare
|
@dlrobertson Looking again at the RFC, it seems to me that the logic for picking the fields of |
|
The job 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 |
- Add the llvm intrinsics used to manipulate a va_list. - Add the va_list lang item in order to allow implementing VaList in libcore.
dlrobertson
force-pushed
the
va_list_pt0
branch
from
861d84a
to
e9e084f
Compare
eddyb
approved these changes
Nov 26, 2018
|
I've tested on |
|
@bors r |
|
📌 Commit e9e084f has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 29, 2018
libcore: Add VaList and variadic arg handling intrinsics ## Summary - Add intrinsics for `va_start`, `va_end`, `va_copy`, and `va_arg`. - Add `core::va_list::VaList` to `libcore`. Part 1 of (at least) 3 for #44930 Comments and critiques are very much welcomed 😄
|
☀️ Test successful - status-appveyor, status-travis |
|
\o/ Thanks @eddyb @joshtriplett @rkruppe and everyone else who helped me work through this! |
|
Is it possible that this creates unaligned slices, thus causing #55011 (comment) ? |
After a very brief review, it looks like an issue with the test. |
This was referenced Dec 1, 2018
|
This PR merges the platform specific aspects of I ran into an example of a non-standard use-case here: https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/xmlwriter.c#L4483-L4497 My proposal would be to move the architecture-specific logic out into its own method that was used to implement the copy method. https://github.com/rust-lang/rust/blob/master/src/libcore/ffi.rs#L193-L202 I'd be curious to hear your opinions! |
|
@glguy I don't see how you could implement a similar function with the current interface. This is indeed a non-standard use case, so I don't see it having an impact on the methods implemented for |
|
@glguy That code is only awkward because it (mis-)uses a while(1) {
VA_COPY(locarg, argptr);
count = vsnprintf((char *) buf, size, format, locarg);
va_end(locarg);
if (!((count < 0) || (count == size - 1) || (count == size) || (count > size))) {
break;
}
xmlFree(buf);
size = BUFSIZ;
buf = (xmlChar *) xmlMalloc(size);
if (buf == NULL) {
xmlWriterErrMsg(NULL, XML_ERR_NO_MEMORY,
"xmlTextWriterVSprintf : out of memory!\n");
return NULL;
}
} |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
va_start,va_end,va_copy, andva_arg.core::va_list::VaListtolibcore.Part 1 of (at least) 3 for #44930
Comments and critiques are very much welcomed 😄