[PATCH] D57635: Add a new intrinsic and attribute to implement `__builtin_va_arg_pack{, _len}` in Clang

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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()`.



================
Comment at: llvm/docs/LangRef.rst:10475
+difference of the total number of arguments at the call site and the number of
+declared parameters.
+
----------------
erik.pilkington wrote:
> efriedma wrote:
> > 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.)
> > 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?
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.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1872
+        if (CB->isInlineVAArgPackCall()) {
+          auto *NewCB = forwardVAArgsToCall(CB);
+          NewCB->setIsInlineVAArgPackCall(false);
----------------
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57635/new/

https://reviews.llvm.org/D57635