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

varargs support #1030

Open
sunfishcode opened this issue Jan 5, 2018 · 27 comments
Open

varargs support #1030

sunfishcode opened this issue Jan 5, 2018 · 27 comments
Labels
cranelift:goal:native-ABI Focus area: Interoperate with native platform ABIs and calling conventions. cranelift Issues related to the Cranelift code generator

Comments

@sunfishcode
Copy link
Member

In order to compile C code that calls printf in libc, we need to implement the caller side of varargs. And in order to compile printf itself, we need to implement the callee side.

@stoklund
Copy link
Contributor

stoklund commented Jan 8, 2018

I am not 100% sure that we actually want too support this. It's not really a goal of Cretonne to be a C compiler nor to support all of LLVM's semantics. I don't think varargs support is needed to be a code generator for the Rust compiler, and WebAssembly doesn't require it either.

@sunfishcode
Copy link
Member Author

What if we didn't do all of the va_arg, llvm.va_start, etc. stuff in Cretonne, and just had some way to specify how much varargs argument space is needed for a caller, and ways to get a pointer to the varargs area for the caller and callee. Producers would do the rest of the lowering manually. This doesn't actually seem like it would be drastically different from constructs Cretonne already has.

Also, for completeness, it looks like varargs will eventually be needed for Rust; see rust-lang/rfcs#2137. Though it's not yet implemented even for the LLVM backend, so we won't have to worry about it for a while.

@stoklund
Copy link
Contributor

stoklund commented Jan 8, 2018

Hmm, the Rust varargs callee use case seems even more obscure than inline assembly. I don't think we should commit to a design for this until we have a more clear need for it.

It's not simple to do varargs correctly. Some ABIs require the caller to place arguments in multiple locations. SPARC even requires some floating point values to be passed in three different locations under some circumstances. Intel avoids that by instead requiring crazy code in the callee for dealing with floating point arguments in registers.

If we have to do it, so be it, but a half-hearted attempt is just going to be buggy like LLVM's va_arg instruction.

@sunfishcode
Copy link
Member Author

I agree. I believe all of your stated concerns above could be addressed by the design I sketched here. I also agree that we don't have to do anything about this right now.

@sunfishcode
Copy link
Member Author

Unfortunately for us, rustc is making progress on implementing variadic functions: rust-lang/rust#49878.

@dlrobertson
Copy link
Contributor

If we have to do it, so be it, but a half-hearted attempt is just going to be buggy like LLVM's va_arg instruction.

The implementation proposed that I'm working on uses the LLVM va_arg instruction under the hood, so things like aggregate types will not be supported.

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 13, 2018

Calling variadic functions is necessary for libstd: rust-lang/rustc_codegen_cranelift#146 (not very urgent for me, might be able to just panic at runtime)

@dlrobertson
Copy link
Contributor

Which architectures and OSes would need to be supported?

@sunfishcode
Copy link
Member Author

sunfishcode commented Nov 13, 2018

Calling variadic functions is certainly easier than the callee side. On many platforms it's the same as non-vararg. On x86-64 System V for example, everything is the same as non-vararg, except that you also need to set the %al register to the number of floating-point/SIMD arguments being passed. In theory, you can do that in Cranelift today by adding an extra function argument and setting the location field of its AbiParam to something like this:

    let rax = isa.register_info().parse_regunit("rax");
    ArgumentLoc::Reg(rax)

So what we need to do is to make sure that works, and package it into a friendlier interface. My rough idea is to put it into something like make_variadic_signature and make_variadic_call functions in cranelift-frontend, which would append the extra needed arguements when needed, so that we limit the spread of C vararg semantics in the rest of the codebase.

On the callee side, Rust is still working on implementing this using LLVM, so there's not a lot of Rust code using it yet :-).

@sunfishcode
Copy link
Member Author

Oh, one other difference between vararg and non-vararg on the caller side in C is that float is promoted to double. I assume we can let frontends take care of that.

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 14, 2018

In theory, you can do that in Cranelift today by adding an extra function argument and setting the location field of its AbiParam to something like this:

How would that work with cranelift-module? It would throw IncompatibleDeclaration, when calling a vararg function with different argument counts/types.

@sunfishcode
Copy link
Member Author

You'd declare the function with the extra argument too; that's the make_variadic_signature part.

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 28, 2019

Does rust-lang/rustc_codegen_cranelift@53e51ac seem like a good way to support this? (I know I forgot setting %al)

@dlrobertson
Copy link
Contributor

@bjorn3 I don't know much about rustc_codegen_cranelift, but I would think that support for calling variadic functions would need to be added to cranelift itself instead of rustc_codegen_cranelift.

IMO there are three things that need solving (one I have still yet to do in rustc for LLVM):

  • caller side
    • Support calling C defined variadic functions
  • callee side
    • Support core::ffi::VaList (va_end, va_copy, and va_arg)
    • Support defining C variadic functions

The last one is still WIP see rust-lang/rust#57760

Is this issue only focused on a subset of the above three? Also note, depending on how the last two are supported in rustc_codegen_llvm and rustc_codegen_ssa, the effort needed to support cranelift for the callee side may be minimal.

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 29, 2019

but I would think that support for calling variadic functions would need to be added to cranelift itself instead of rustc_codegen_cranelift.

Cranelift is more low level than LLVM. Eg it doesn't support structs, but expects you to implement them yourself. Implementing the call side of variadic functions can be done as a producer of cranelift ir. It is however possible to implement it in cranelift in the future.

Is this issue only focused on a subset of the above three?

This issue is about both.

Also note, depending on how the last two are supported in rustc_codegen_llvm and rustc_codegen_ssa, the effort needed to support cranelift for the callee side may be minimal.

rustc_codegen_llvm depends on LLVM to implement it and rustc_codegen_ssa just delegates to the codegen backend using it.

@dlrobertson
Copy link
Contributor

rustc_codegen_ssa just delegates to the codegen backend using it

Exactly, I'm specifically referring to the implementation of va_arg here. The current manual implementation of va_arg for pointer variants found here just uses the BuilderMethods trait which the backend provides. So in theory va_arg.rs could be moved to rustc_codegen_ssa meaning the implementations of va_arg found there could be used by both back-ends. In addition, there is rust-lang/rust#56489 which would also obviously implement va_arg for either backend.

@jyn514
Copy link
Contributor

jyn514 commented Oct 10, 2019

I'm not sure I see how a frontend could implement varargs manually. How would I implement a calling convention without access to registers? e.g. for floating point %al needs to be set: https://stackoverflow.com/a/2538212/7669110

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 10, 2019

  1. for sysv you can ignore varargsness when calling it (except for %al) just pretend that it always has the exact types and number of values you pass.
  2. you can manually assign a value to a register (not sure if legalization or something else overwrites it/breaks though)

@jyn514
Copy link
Contributor

jyn514 commented Nov 3, 2019

for sysv you can ignore varargsness when calling it (except for %al) just pretend that it always has the exact types and number of values you pass.

That doesn't work if you pass different numbers of args in the same program, e.g.

int printf(const char *format, ...);

int main() {
    printf("hello world\n");
    printf("some int here: %d", 2);
}

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 3, 2019

What I did for cg_clif is dont tell cranelift_module about the varargs and then use declare_func_in_func once per vararg call. That will give a unique FuncRef every time. I then changed the associates Signature (which is also unique) to match the varargs.

@jyn514
Copy link
Contributor

jyn514 commented Nov 3, 2019

and Cranelift allows that?? I can try it out, I'm surprised that cranelift allows you to modify the signature though haha

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 3, 2019

Yes, the Function you build is competely mutable. You can rewrite any part you want.

@jyn514
Copy link
Contributor

jyn514 commented Nov 3, 2019

How do I set registers?

I found RegDiversions::reg to get the current register of a Value and InstBuilder::regmove to move from one register to another, but I don't know how to get a RegDiversions instance (RegDiversions::new looks like it won't have any of the existing registers). My code so far looks like this:

// float_variadic is an integer, builder is a FunctionBuilder
let float_ir = builder.ins().iconst(types::I8, float_variadic);
let target = self.module.isa();
if target.name() != "x86" {
    unimplemented!("variadic args for architectures other than x86");
}
let reginfo = target.register_info();
let al = reginfo.parse_regunit("al").expect("al register should exist on x86");
let diversions: RegDiversions = unimplemented!(); // how do I get this?
let locations: ValueLocations = unimplemented!(); // how do I get this?
let current_reg = diversions.reg(float_ir, &locations);
builder.ins().regmove(float_ir, current_reg, al);

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 3, 2019

RegDiversions is only used when regalloc is running or has already ran. For your usecase you will want to use AbiParam::special_reg in the Signature. You can get the RegUnit by using isa.register_info().parse_regunit("rax") for x86_64. I dont think passing al to parse_regunit will work, so I chose rax in this example.

@alexcrichton alexcrichton transferred this issue from bytecodealliance/cranelift Feb 28, 2020
@alexcrichton alexcrichton added cranelift Issues related to the Cranelift code generator cranelift:goal:native-ABI Focus area: Interoperate with native platform ABIs and calling conventions. labels Feb 28, 2020
@Jakersnell
Copy link
Contributor

Are there any updates with this? Implementing varargs as a frontend with Cranelift is extremely cumbersome.

@bjorn3
Copy link
Contributor

bjorn3 commented Apr 17, 2024

There hasn't been any change since the last comment here. Also do you need just support for calling variadic functions or also defining them? The former is a lot easier to support than the latter.

@Jakersnell
Copy link
Contributor

Just calling them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:goal:native-ABI Focus area: Interoperate with native platform ABIs and calling conventions. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

7 participants