Lvalue reference types appear in the signature of __builtin_va_start when compiled as C for some targets

[Quuxplusone]

Bugzilla Link	PR37122
Status	NEW
Importance	P enhancement
Reported by	Tom Honermann (thonerma@synopsys.com)
Reported on	2018-04-13 08:32:51 -0700
Last modified on	2018-04-13 12:20:44 -0700
Version	6.0
Hardware	PC Windows NT
CC	efriedma@quicinc.com, llvm-bugs@lists.llvm.org, sig-rnd-sat-clang-bugs@synopsys.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

It seems that the signature of __builtin_va_start contains a C++ lvalue
reference type when targeting Windows, even when compiling as C:

$ cat t.c
void f(int n, ...) {
  __builtin_va_list v;
  __builtin_va_start(v, n);
}

$ clang --version
clang version 6.0.0 (tags/RELEASE_600/final)
Target: x86_64-unknown-linux-gnu
...

$ clang -c -Xclang -ast-dump -target x86_64-pc-windows-gnu t.c
...
`-FunctionDecl 0x4c89578 <col:3> col:3 implicit used __builtin_va_start 'void
(__builtin_va_list &, ...)' extern
  |-ParmVarDecl 0x4c89618 <<invalid sloc>> <invalid sloc> '__builtin_va_list &'
  `-NoThrowAttr 0x4c89680 <col:3> Implicit

Note the presence of '__builtin_va_list &' as a parameter type.

The lvalue reference is a surprise for AST consumers that do not expect to
encounter C++ reference types in code compiled as C.

[Quuxplusone]

We could change __builtin_va_start to use custom type-checking, and hide the signature, I guess? I don't see how it would make much of a difference in practice, though: the reference type is an accurate representation of the signature.

[Quuxplusone]

The signature already differs based on target platform:

$ clang -c -Xclang -ast-dump -target x86_64-pc-linux-gnu t.c
...
`-FunctionDecl 0x5c13908 <col:3> col:3 implicit used __builtin_va_start 'void
(struct __va_list_tag *, ...)' extern
  |-ParmVarDecl 0x5c139a8 <<invalid sloc>> <invalid sloc> 'struct __va_list_tag *'
  `-NoThrowAttr 0x5c13a10 <col:3> Implicit

I'm not familiar enough with the calling conventions to appreciate why a
pointer is used in this case, but a "reference" is used on Windows.

The defined signature is:

include/clang/Basic/Builtins.def:
  ..
  38 //  A -> "reference" to __builtin_va_list
  ..
 426 BUILTIN(__builtin_va_start, "vA.", "nt")

I presume that the signature translation for "A" is target dependent, but I
haven't managed to identify the code that handles that yet.

[Quuxplusone]

(In reply to Eli Friedman from comment #1)
> I don't see how it would make much of a difference in
> practice, though: the reference type is an accurate representation of the
> signature.

The difference for us is that we hit an assert in our own code that is present
to ensure we aren't leaking C++ concepts into our C analysis.

[Quuxplusone]

The relevant code is DecodeTypeFromStr in ASTContext.cpp; there's a nice comment there explaining why this is target-dependent.

[Quuxplusone]

(In reply to Eli Friedman from comment #4)
> The relevant code is DecodeTypeFromStr in ASTContext.cpp; there's a nice
> comment there explaining why this is target-dependent.

Thank you.  The comment is a little imprecise since it mentions behavior for
x86 vs x86_64, but doesn't indicate corresponding target OS dependencies.  I
did some more testing and I now see that the reference type appears for x86 on
Linux as well.  It looks like the comment is consistent with Linux.

Would it not make sense for the current 'char*&' case to be 'char**'?

[Quuxplusone]

We can't change the signature of __builtin_va_start; a bunch of existing code depends on the current signature.

[Quuxplusone]

(In reply to Eli Friedman from comment #6)
> We can't change the signature of __builtin_va_start; a bunch of existing
> code depends on the current signature.

We clearly can't require source code changes.  I was thinking more from the
code gen perspective (which I would expect to be unaffected).  But yes, this
would mean adding an implicit operator& in the AST.  That's ugly.