Uh oh!
There was an error while loading. Please reload this page.
libcore: Add VaList and variadic arg handling intrinsics - #49878
Conversation
rust-highfive
commented
Apr 11, 2018
(rust_highfive has picked a reviewer for you, use r? to override) |
TimNN
commented
Apr 11, 2018
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 |
petrochenkov
commented
Apr 12, 2018
nagisa
left a comment
There was a problem hiding this comment.
The changes look good, but I would probably prefer to see the full implementation before landing, rather than doing it in parts.
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.
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.
dlrobertson
commented
Apr 13, 2018
I'll be in airports all weekend, but I can try to get the commit with |
rust-highfive
commented
Apr 17, 2018
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 |
rust-highfive
commented
Apr 17, 2018
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 |
rust-highfive
commented
Apr 17, 2018
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 |
bors
commented
Apr 21, 2018
☔ The latest upstream changes (presumably #50093) made this pull request unmergeable. Please resolve the merge conflicts. |
rust-highfive
commented
Apr 22, 2018
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 |
bors
commented
Apr 26, 2018
☔ The latest upstream changes (presumably #50228) made this pull request unmergeable. Please resolve the merge conflicts. |
emilyalbini
commented
Apr 30, 2018
Ping from triage @eddyb! This PR needs your review. |
dlrobertson
commented
Apr 30, 2018
@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. |
| 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.
eddyb
commented
Apr 30, 2018
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. |
dlrobertson
commented
Apr 30, 2018
Np. Nothing a little vimdiff can't fix |
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?
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.
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.
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.
There was a problem hiding this comment.
Function used to generate the correct type.
@dlrobertson Looking again at the RFC, it seems to me that the logic for picking the fields of |
TimNN
commented
Nov 20, 2018
Ping from triage @dlrobertson: What is the status of this PR? It looks like it needs to be rebased, but maybe is also ready for another review (cc @eddyb)? |
dlrobertson
commented
Nov 21, 2018
@TimNN The current code works, but I'm working on
I can omit |
eddyb
commented
Nov 21, 2018
I'd rather wait for |
rust-highfive
commented
Nov 26, 2018
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 |
rust-highfive
commented
Nov 26, 2018
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
commented
Nov 26, 2018
@eddyb updated |
There was a problem hiding this comment.
I think you want to use va_list_ty here?
There was a problem hiding this comment.
You don't need to alloca in either case, you have llresult to write the destination to (after which you use return, I believe.
There was a problem hiding this comment.
Or result.llval, I suppose. You can also access result.layout if necessary.
There was a problem hiding this comment.
Thanks! Much cleaner
There was a problem hiding this comment.
You can probably just check here whether the return type(result.layout.ty) is tcx.lang_items().va_list() or something like that.
There was a problem hiding this comment.
Yup... also much cleaner
There was a problem hiding this comment.
I think you should match on result.layout.ty.sty for ty::Adt(def, _) and compare def.did with did instead of calling type_of.
rust-highfive
commented
Nov 26, 2018
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
commented
Nov 27, 2018
I've tested on |
eddyb
commented
Nov 27, 2018
@bors r+ |
bors
commented
Nov 27, 2018
📌 Commit e9e084f has been approved by |
bors
commented
Nov 29, 2018
bors
commented
Nov 29, 2018
☀️ Test successful - status-appveyor, status-travis |
dlrobertson
commented
Nov 29, 2018
\o/ Thanks @eddyb@joshtriplett@rkruppe and everyone else who helped me work through this! |
RalfJung
commented
Nov 30, 2018
Is it possible that this creates unaligned slices, thus causing #55011 (comment) ? |
dlrobertson
commented
Nov 30, 2018
After a very brief review, it looks like an issue with the test. |
glguy
commented
Dec 4, 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! |
dlrobertson
commented
Dec 5, 2018
@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 |
eddyb
commented
Dec 6, 2018
@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");
returnNULL;
}
} |
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 😄