Page MenuHomePhabricator

Add a new intrinsic and attribute to implement `__builtin_va_arg_pack{,_len}` in Clang
AbandonedPublic

Authored by erik.pilkington on Feb 1 2019, 7:31 PM.

Details

Summary

This patch adds an attribute inline_va_arg_pack which applies to calls/invokes and is the result of lowering __builtin_va_arg_pack(), as well as the intrinsic @llvm.va_arg_pack_len, which is the result of lowering __builtin_va_arg_pack_len(). These can only appear in available_externally functions, and are themselves lowered during inlining. You can read more about them here: https://gcc.gnu.org/onlinedocs/gcc-4.4.2/gcc/Constructing-Calls.html, I explained the motivation for them on the clang patch.

Fixes llvm.org/PR7219 & rdar://11102669

Thanks for taking a look!
Erik

Diff Detail

Event Timeline

erik.pilkington created this revision.Feb 1 2019, 7:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 7:31 PM

Maybe a silly question, but is this actually worth implementing, at this point? The new IR additions have unusual semantics that transforms are continually going to trip over; I'm not convinced you found all the places that currently need checks. And there isn't any existing code we need to be compatible with, I think. If we were designing a frontend extension from scratch, we would probably take a different approach.

llvm/docs/LangRef.rst
1408

I'm not really a fan of using an attribute like this. Currently, there are basically two ways to refer to a varargs list: va_start, and a musttail call. This is adding a third, in a way that's very easy to miss.

Not sure what it should look like, though. Maybe instead of calling the function directly, we should call an intrinsic? I guess any approach has odd semantic implications.

10475

You're computing the difference here using the number of IR arguments? That's not going to return the correct result in general; you need the frontend to compute this.


I wonder if we should add an attribute to denote "weird" intrinsics that can't be treated as equivalent to a function call in general. (For example, I think you have to forbid function merging on any function that calls this intrinsic.)

erik.pilkington marked 2 inline comments as done.Feb 2 2019, 3:36 PM

Maybe a silly question, but is this actually worth implementing, at this point? The new IR additions have unusual semantics that transforms are continually going to trip over; I'm not convinced you found all the places that currently need checks. And there isn't any existing code we need to be compatible with, I think. If we were designing a frontend extension from scratch, we would probably take a different approach.

What kind of approach are you thinking? For us, this isn't about being compatible with GCC as much as it is about getting the feature. Doing something different here would be fine as long as its "better enough" to justify deviating from GCC compatibility. Even though no existing code depends on it, compatibility with GCC would still be nice to have, so existing libraries that conditionally use the __builtin_va_arg_pack will just work with clang, whereas if we went with a different solution they would have to keep it disabled or write two copies of all their wrapper functions.

llvm/docs/LangRef.rst
1408

I tried this out first with an intrinsic, but it seemed like pretending that the va args were a value produced by a call that are passed as an argument in a different instruction was a more fragile and odd way of modelling this. Either way though, there will still be 3 ways of referencing va args, and I'm not sure that using an intrinsic instead of an attribute would make this corner case any more visible.

10475

You're computing the difference here using the number of IR arguments? That's not going to return the correct result in general; you need the frontend to compute this.

Oh right, good point. I'll update this on monday.

I wonder if we should add an attribute to denote "weird" intrinsics that can't be treated as equivalent to a function call in general. (For example, I think you have to forbid function merging on any function that calls this intrinsic.)

What other cases do you think are "weird" enough?

In terms of the general approach, what problem are we really trying to solve? The ability to forward a variadic argument list on to another variadic function doesn't grant any semantic power if you control the implementation; you can always just use a va_list instead (e.g. convert printf to vprintf). In C++, you can also just use a variadic template. And if the goal is just to allow the compiler to emit fortified calls to known library functions, it would be more straightforward to add a flag to implicitly instrument code.

I guess if you specifically want to add a feature to C which allows you to write, in C, a function (not a macro) which wraps an interface you don't control that takes a variadic argument list, you can't do much better than __builtin_va_arg_pack().

llvm/docs/LangRef.rst
10475

Basically there's a general class of intrinsics which do something different if you transform a direct call into a call to a function which calls the intrinsic. (This is excluding intrinsics which expect certain arguments to be integer constants; those should also be marked somehow, but it's mostly orthogonal.)

Going through the list of target-independent intrinsics, I think the following fall into that category: llvm.va_start, llvm.va_end, llvm.stackprotector, llvm.returnaddress, llvm.addressofreturnaddress, llvm.sponentry, llvm.frameaddress, llvm.localescape, llvm.stacksave, llvm.stackrestore, llvm.get.dynamic.area.offset, llvm.lifetime.start, llvm.lifetime.end, llvm.invariant.start, llvm.invariant.end, some llvm.eh.* intrinsics, and some llvm.dbg.* intrinsics.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1872

Forwarding like this doesn't actually work in general, if the fixed arguments of the caller don't match the fixed arguments of the callee. For certain calling conventions (like x86-64), the IR types change depending on the number of available registers.

Not sure what the correct solution is here.

erik.pilkington abandoned this revision.Feb 3 2019, 11:24 AM

In terms of the general approach, what problem are we really trying to solve? The ability to forward a variadic argument list on to another variadic function doesn't grant any semantic power if you control the implementation; you can always just use a va_list instead (e.g. convert printf to vprintf). In C++, you can also just use a variadic template. And if the goal is just to allow the compiler to emit fortified calls to known library functions, it would be more straightforward to add a flag to implicitly instrument code.

I guess if you specifically want to add a feature to C which allows you to write, in C, a function (not a macro) which wraps an interface you don't control that takes a variadic argument list, you can't do much better than __builtin_va_arg_pack().

Yeah, this is just about getting C++-compatible fortified wrappers for libc functions. I thought that delegating to a va_list variant was no good since it looks like we don't have the ability to inline a function that calls va_start, so any fortified stuff in the body would just be discarded. I think I'm convinced that handling this in the frontend would be a simpler way of solving this though, maybe with an attribute or something. I'm going to tentatively mark this as abandoned, I'll revive it if the frontend stuff doesn't pan out.

Thanks for your help!