Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcore: Implement VaList::arg in pure rust #56489

Open
dlrobertson opened this issue Dec 4, 2018 · 10 comments
Open

libcore: Implement VaList::arg in pure rust #56489

dlrobertson opened this issue Dec 4, 2018 · 10 comments
Labels
A-ffi Area: Foreign Function Interface (FFI) C-enhancement Category: An issue proposing an enhancement or a PR with one. F-c_variadic `#![feature(c_variadic)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dlrobertson
Copy link
Contributor

dlrobertson commented Dec 4, 2018

Summary

Implement VaList::arg in pure rust, similar to va_list-rs.

Details

We currently expose the va_arg intrinsic which should emit the correct LLVM for va_arg for the given architecture and OS. We currently use the LLVM va_arg instruction, but it doesn't emit the correct code for some common OSes and architectures causing us to implement the instruction manually (See [src/librustc_codegen_llvm/va_arg.rs] for details). Since we do not support calling VaList::arg on arbitrary types, we might be able to implement something similar to va_list-rs in pure rust for most architectures, falling back to the LLVM va_arg instruction only when a pure rust implementation does not exist.

Original issue

Note this issue has been changed following #56489 (comment). The original issue is as follows:

codegen: Move custom va_arg logic to librustc_codegen_ssa

The LLVM va_arg intrinsic is far from a complete implementation. As a result, we have started to manually implement va_arg (like clang does) with the Builder in src/librustc_codegen_llvm/va_arg.rs. This logic should be moved to librustc_codegen_ssa in BuilderMethods::va_arg.

BuilderMethods::va_arg needs to fall back to LLVM's va_arg intrinsic when there isn't an custom implementation available, so we'll need to add a new trait method backend_va_arg (please suggest a better name 😄) that exposes the backend specific implementation of va_arg.

@dlrobertson
Copy link
Contributor Author

Does this seam reasonable? Should be relatively simple to implement.

CC @eddyb @denismerigoux

@eddyb
Copy link
Member

eddyb commented Dec 4, 2018

I wouldn't put the manual implementation of va_arg on BuilderMethods, but keep it in a separate file as it now.
We could probably implement the entire va_arg ABI ourselves, for all platforms, if it's not too big of a deal (can we even do it in Rust code instead of generating IR?).

cc @rkruppe @sunfishcode

@dlrobertson
Copy link
Contributor Author

I wouldn't put the manual implementation of va_arg on BuilderMethods, but keep it in a separate file as it now.

Seems reasonable. So would this file exist in rustc_codegen_ssa or rustc_codegen_llvm?

We could probably implement the entire va_arg ABI ourselves, for all platforms, if it's not too big of a deal.

If we get them all implemented it should be a fairly large file, but if clang can do it, I don't see why we can't do it for the vast majority of architectures and OSes with just a very select few falling through to the LLVM implementation.

(can we even do it in Rust code instead of generating IR?).

I'm still very new to rustc, so I'm not entirely sure. As long as you:

  1. know the architecture and OS
  2. can reason about types
  3. can load/store

you should be able to implement it. clang implements it with IR, so it might be easiest to start there using clang as a reference. There is the va_list crate.

@denismerigoux
Copy link
Contributor

From a purely "traitification" point of view, I don't think there should be any problem in making these methods generic on the backend.

@eddyb
Copy link
Member

eddyb commented Dec 4, 2018

@dlrobertson I was thinking having trait impls in libcore.
If we restrict ourselves to a finite set of types, it might be possible to do without generating IR at all.

@dlrobertson
Copy link
Contributor Author

@eddyb

If we restrict ourselves to a finite set of types, it might be possible to do without generating IR at all.

This is definitely something worth looking into. We could something similar to what the va_list crate did where each T implements a trait that allows it to be fetched from a VaList. If we went this route I don't think we could support aggregate types. I also don't know if this would work with the OSes and architectures that 8 byte values to 4 byte aligned address.

I also realized we couldn't move va_arg.rs to librustc_codegen_ssa due to the use of the LlvmType trait.

@eddyb
Copy link
Member

eddyb commented Dec 7, 2018

@dlrobertson We can always switch back to something built into the compiler, if we want to support aggregates. But I feel like that's far less likely to happen.
OTOH, maybe FnType could be used to compute what we should be generating?

@dlrobertson
Copy link
Contributor Author

@eddyb

We can always switch back to something built into the compiler, if we want to support aggregates. But I feel like that's far less likely to happen.

Cool. I'll try to implement VaList purely in libcore. I think we should be able to implement it such that all of the currently supported types will work.

OTOH, maybe FnType could be used to compute what we should be generating?

I don't know enough about FnType to understand what you're getting at, but I'll look into it and ping you with some questions once I understand a bit more about FnType.

@eddyb
Copy link
Member

eddyb commented Dec 7, 2018

FnType is a weird name (maybe we should call it to CallAbi or something) for a low-level description of how Rust arguments and returns are passed through a call.

It's possible the relevant machinery could be generalized to give us the "VaList encoding" of an argument.

@dlrobertson
Copy link
Contributor Author

Got it. Yeah, that might work. I'll look into that a bit too.

If we do use libcore to generate va_arg could we add a new cfg for the VaList type? Or is there a better way than copy-pasting a multi-line cfg? I think if we don't find a clever alternative, we'll end up with a lot of messy cfgs and we could end up with some nasty bugs.

@dlrobertson dlrobertson changed the title codegen: Move custom va_arg logic to librustc_codegen_ssa libcore: Implement VaList::arg in pure rust Jan 18, 2019
@jonas-schievink jonas-schievink added the A-ffi Area: Foreign Function Interface (FFI) label Jan 27, 2019
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. F-c_variadic `#![feature(c_variadic)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ffi Area: Foreign Function Interface (FFI) C-enhancement Category: An issue proposing an enhancement or a PR with one. F-c_variadic `#![feature(c_variadic)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants