Support defining C compatible variadic functions #57760
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
This comment has been minimized.
This comment has been minimized.
|
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 |
alexreg
suggested changes
Jan 20, 2019
There was a problem hiding this comment.
Great work on this! :-D I think you've essentially taken a sound approach to implementing this feature, though others who are probably more qualified than me will hopefully confirm that. See my suggestions about the codegen from MIR, but otherwise my comments are just nits (really minor things). Feel free to ping me again for a final review, once you've ironed out everything and added more tests. (A brief chapter in the Unstable Book would be nice too.)
|
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 |
|
I noticed some typos I was going to push before I saw this PR, you might just want to take them into this PR instead master...parched:va-args |
|
@parched 👍 good catch! I'll merge it in. |
|
@parched now that I think about it that will likely solve a aarch64 issue I saw previously, so it might be worth merging as a standalone PR. |
dlrobertson
force-pushed
the
varargs1
branch
2 times, most recently
from
9dca909
to
0322ede
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 |
|
Currently this PR causes an issue with |
|
Figured out what is causing the issue with When running the following using let fmt_str = CString::new("%d%d\n").unwrap();
printf(fmt_str.as_ptr(), 0, 42);We currently emit the following LLVM IR: When we should emit the following LLVM IR: So clearly at some point when attempting to remove the "spoofed" |
TimNN
added
the
S-waiting-on-author
|
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 |
bjorn3
requested changes
Feb 1, 2019
dlrobertson
commented
Feb 1, 2019
This comment has been minimized.
This comment has been minimized.
dlrobertson
force-pushed
the
varargs1
branch
2 times, most recently
from
36e5d87
to
285ec4e
Compare
bors
added
S-waiting-on-bors
|
⌛ Testing commit f7dd438 with merge 35046f85f921a5daadfcd7be66cd82ea27352f2b... |
|
@bors retry - prioritize stable backport |
bors
added a commit
that referenced
this pull request
Feb 28, 2019
Support defining C compatible variadic functions
## Summary
Add support for defining C compatible variadic functions in unsafe rust with
`extern "C"` according to [RFC 2137].
## Details
### Parsing
When parsing a user defined function that is `unsafe` and `extern "C"` allow
variadic signatures and inject a "spoofed" `VaList` in the new functions
signature. This allows the user to interact with the variadic arguments via a
`VaList` instead of manually using `va_start` and `va_end` (See [RFC 2137] for
details).
### Codegen
When running codegen for a variadic function, remove the "spoofed" `VaList`
from the function signature and inject `va_start` when the arg local
references are created for the function and `va_end` on return.
## TODO
- [x] Get feedback on injecting `va_start/va_end` in MIR vs codegen
- [x] Properly inject `va_end` - It seems like it should be possible to inject
`va_end` on the `TerminatorKind::Return`. I just need to figure out how
to get the `LocalRef` here.
- [x] Properly call Rust defined C variadic functions in Rust - The spoofed
`VaList` causes problems here.
Related to: #44930
r? @ghost
[RFC 2137]: https://github.com/rust-lang/rfcs/blob/master/text/2137-variadic.md
|
☀️ Test successful - checks-travis, status-appveyor |
This was referenced Feb 28, 2019
3 tasks
|
@dlrobertson I think this may have regressed macros and variadic functions slightly, I've got a test that looks like: #[foo]
extern "C" {
pub fn foo3(x: i32, ...);
} but the input to the macro extern "C" {
pub fn foo3(x: i32, ..., ...);
} Do you think this may have accidentally broken the pretty printer for variadic functions? |
|
(I've opened up #58853 to track regardless) |
It is very likely.
Thanks! |
|
@dlrobertson I appreciate your hard work on this feature. I think hitting an issue in release mode on the latest nightly ( #![feature(c_variadic)]
extern "C" {
#[no_mangle]
fn vprintf(_: *const libc::c_char, _: ...) -> libc::c_int;
}
#[no_mangle]
unsafe extern "C" fn variadic(fmt: *const libc::c_char, a: ...)
-> libc::c_int {
vprintf(fmt, a)
}
fn main() {
let fmt = std::ffi::CString::new("test %d %d %d\n").expect("new failed");
unsafe {
variadic(fmt.as_ptr(), 1, 2, 3);
}
}Advance apologies if I'm doing something that isn't expected to work. |
|
@thedataking: Can you open this example up as a new issue, so we don't lose track of it here? |
|
@thedataking Thanks for the catch. A issue would definitely be appreciated. |
|
@dlrobertson @varkor I opened up #58980 to track this issue. Thanks! |
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
Add support for defining C compatible variadic functions in unsafe rust with
extern "C"according to RFC 2137.Details
Parsing
When parsing a user defined function that is
unsafeandextern "C"allowvariadic signatures and inject a "spoofed"
VaListin the new functionssignature. This allows the user to interact with the variadic arguments via a
VaListinstead of manually usingva_startandva_end(See RFC 2137 fordetails).
Codegen
When running codegen for a variadic function, remove the "spoofed"
VaListfrom the function signature and inject
va_startwhen the arg localreferences are created for the function and
va_endon return.TODO
va_start/va_endin MIR vs codegenva_end- It seems like it should be possible to injectva_endon theTerminatorKind::Return. I just need to figure out howto get the
LocalRefhere.VaListcauses problems here.Related to: #44930
r? @ghost