Add support for __builtin_va_arg_pack and __builtin_va_arg_pack_len

[efriedma-quic]

Bugzilla Link	7219
Version	unspecified
OS	Linux
CC	@berolinux,@ismail,@seanm,@stephenhines,@tbaederr

Extended Description

Per summary, it would be nice to add support for __builtin_va_arg_pack and __builtin_va_arg_pack_len at some point. Implementation steps:

1. Add builtins to Builtins.def.
2. Add Sema check that these are only called from vararg function.
3. Add Sema check that these are only called from a function whose definition is never directly generated (something like "Decl->isInlined() && !Decl->isInlineDefinitionExternallyVisible()").
4. (Optional) Add Sema check that __builtin_va_arg_pack() is only used as the last argument to a vararg call; this might be a bit tricky.
5. Add LLVM support for the intrinsics. I think this will require adding a couple new LLVM intrinsics, defining some sort of metadata for noting the number of arguments to a call, and making the inliner transform the intrinsics appropriately. Somewhat nasty, but I can't think of any obvious issues with this approach, and I can't think of any other practical approach.

[llvmbot]

This is also being track as rdar://problem/11102669

[llvmbot]

We (Android) would be very happy to see this happen. Our libc currently has a number of the __*_chk() versions of functions #ifdef'd out for clang because this isn't supported. For example: https://github.com/android/platform_bionic/blob/master/libc/include/fcntl.h#L109

[rengolin]

These builtins seem to work around va_list/va_start/va_end/va_arg. Are they really required, or are they just a lazy way of not writing your own local functions as a matter of vararg types?

AFAICS, __builtin_va_arg_pack is almost useless, since generally stdlibs provide a va_list variant (no?), and __builtin_va_arg_pack_len usage is limited to one use: know how many items are references without having to add more code to check it, which seems a bit heavy for a run-time warning.

I'm not against them, just sceptical of their value for such a big change in both LLVM and Clang.

[berolinux]

__builtin_va_arg_pack is used inside some stdlibs, e.g. in Bionic we have this construct:

#if defined(clang)
#if !defined(snprintf)
#define __wrap_snprintf(dest, size, ...) __builtin___snprintf_chk(dest, size, 0, __bos(dest), VA_ARGS)
#define snprintf(...) __wrap_snprintf(VA_ARGS)
#endif
#else
__BIONIC_FORTIFY_INLINE
__printflike(3, 4)
int snprintf(char *dest, size_t size, const char *format, ...)
{
return __builtin___snprintf_chk(dest, size, 0,
__bos(dest), format, __builtin_va_arg_pack());
}
#endif

(where the #if defined(clang) variant is rather bogus, causes e.g. Chromium build to fail when it tries to
namespace base {
int snprintf(.....)
};
)

There doesn't seem to be a good solution for this type of stuff without __builtin_va_arg_pack.

glibc actually has a similar construct for fortified snprintf -- except it conditionalizes the #define on !__cplusplus to work around stuff trying to pull snprintf into a namespace or the likes. But that results in unfortified snprintf being used...

[rengolin]

I know bionic and glibc use that, but there is a perfectly valid option to use va_list directly (which glibc also uses).

extern int vsnprintf (char *__restrict __s, size_t __maxlen,
__const char *__restrict __format, __gnuc_va_list __arg)
attribute ((format (printf, 3, 0)));

The only need for the fortified version is __builtin_va_arg_pack_len, which should actually be fixed by having specific non-vararg functions, for example this is kind of silly in a variadic function:

if (__builtin_va_arg_pack_len() > 1) {
    __creat_too_many_args();  // compile time error
}

But I guess changing the standard is needed to make them non-variadic.

My point is that, whatever savings you have on user code by implementing these builtins, you'll have in the compiler, which may look like saving (users don't need the bloating) but can also be a maintenance hell (by having to account for such builtins on future changes, optimizations, ABI compliance, etc).

[rengolin]

5. Add LLVM support for the intrinsics. I think this will require adding a
   couple new LLVM intrinsics, defining some sort of metadata for noting the
   number of arguments to a call, and making the inliner transform the
   intrinsics appropriately. Somewhat nasty, but I can't think of any obvious
   issues with this approach, and I can't think of any other practical approach.

Eli,

Do you mean add a metadata on every variadic call on the call itself? Or adding a simple metadata on the variadic function that will invoke the behaviour on the inliner to:

IF inline-vararg metadata on function:
ARGS = all varargs of the call
FOREACH inlined function's function with __builtin_va_arg_pack:
replace on the vararg metadata at the end with ARGS
FOREACH usage of __builtin_va_arg_pack_len in the inlined:
replace by number of args in ARGS
ENDIF

If LLVM already supports always_inline or gnu_inline, the out-of-line function should be removed before validation, or the builtins/metadata will fail.

[llvmbot]

This would be nice to have, to enable using clang with this stand-alone FORTIFY_SOURCE implementation: http://git.2f30.org/fortify-headers/about/

[llvmbot]

Put some patches up for this here: https://reviews.llvm.org/D57635 & https://reviews.llvm.org/D57634

[tbaederr]

Is there any movement on this? All the upstream LLVM feedback I can find on va_arg_pack() and va_arg_pack_len() seem pretty negative. Is there any value in working on this?