[llvm-commits] [PATCH] Add va_argpack intrinsics

[Eli Friedman]

On Tue, May 15, 2012 at 12:10 PM, Benjamin Kramer
<benny.kra at googlemail.com> wrote:
>
> On 15.05.2012, at 20:47, Eli Friedman wrote:
>
>> On Tue, May 15, 2012 at 11:03 AM, Chandler Carruth <chandlerc at google.com> wrote:
>>> On Tue, May 15, 2012 at 11:36 AM, Eli Friedman <eli.friedman at gmail.com>
>>> wrote:
>>>>
>>>> On Tue, May 15, 2012 at 10:03 AM, Benjamin Kramer
>>>> <benny.kra at googlemail.com> wrote:
>>>>>
>>>>> On 15.05.2012, at 18:16, Chandler Carruth wrote:
>>>>>
>>>>>> On Tue, May 15, 2012 at 9:08 AM, Benjamin Kramer
>>>>>> <benny.kra at googlemail.com> wrote:
>>>>>> This will be used to implement __builtin_va_arg_pack[len] in clang
>>>>>> (PR7219).
>>>>>>
>>>>>> Awesome, thanks for working on this...
>>>>>>
>>>>>> Since this is used to model a pretty crazy gcc extension (used by
>>>>>> glibc) we have
>>>>>> to expand the intrinsics in the inliner. When it replaces a vararg call
>>>>>> site with
>>>>>> a copy of the function another pass over all instructions is performed
>>>>>> which
>>>>>> replaces any pack intrinsics with the values from the call site. There
>>>>>> are some
>>>>>> rather narrow requirements where the intrinsics can occur, documented
>>>>>> in LangRef
>>>>>> and enforced by the verifier.
>>>>>>
>>>>>> This patch has two other side effects:
>>>>>> - DAE is not allowed to remove varargs if a va_pack intrinsic is used.
>>>>>> - Inliner can now inline vararg functions iff they don't use va_start.
>>>>>> This can
>>>>>>  happen if DAE doesn't remove the varargs because there is a va_pack
>>>>>> intrinsic
>>>>>>  or if the AlwaysInliner tries to inline the function.
>>>>>>
>>>>>>
>>>>>> I'm pretty OK wit this strategy. It actually has some other interesting
>>>>>> artifacts I'd like to explore later. Specifically, I think we can generalize
>>>>>> this to allow inlining of varargs functions when the va_start is in dead
>>>>>> code...
>>>>
>>>> I'm actually mildly concerned about this strategy: the LLVM IR inliner
>>>> can't actually reason correctly about the ABI rules for a varargs
>>>> call, and will therefore screw up if you try to pass anything
>>>> complicated (_Complex, structs) through the varargs list.  This might
>>>> not matter in practice if it isn't used for anything other than the
>>>> printf and scanf families of functions, but it's still worth thinking
>>>> about.
>>>
>>>
>>> I share your concerns here regarding the pack and pack_len builtins...
>>> However, I think in the extremely limited context they are used, this is
>>> safe.
>>>
>>> The idea is that these builtins can only be used inside of some other vararg
>>> function, and the only arguments we move from the outer call to the inner
>>> are ones that would already have gone through Clang's vararg lowering. The
>>> goal seems specifically to re-use the way that the outer call is lowered
>>> when setting up the inlined inner call.
>>>
>>> I've been trying to think of a construct that breaks this, but so far I
>>> can't come up with any...
>>
>> Consider the following on x86-64:
>> void f(int x, int y, ...);
>> struct A { long long x,y; };
>> void g(int a, int b, int c, int d, ...) {
>>  //f(a,b,__builtin_va_pack());
>>  f(a,b,(struct A){},(struct A){});
>> }
>> void h() {
>>  struct A x;
>>  g(0,0,0,0,x,x);
>> }
>
> This is pretty insane and I'm afraid there is no solution without putting an inliner into clang :(
>
> Are you okay with "Passing aggregate types to a function using llvm.va_pack is undefined behavior." in the LangRef?

It's "anything which doesn't map to a single LLVM scalar type".  I
really have two concerns here: one, we don't want to silently
miscompile code, and two, we need to make sure this limitation doesn't
actually bite in the cases we care about.

-Eli