Page MenuHomePhabricator

Support __builtin_ms_va_list.
ClosedPublic

Authored by cdavis5x on Sep 6 2013, 1:59 PM.

Details

Summary

This change adds support for __builtin_ms_va_list, a GCC extension for
variadic ms_abi functions. The existing __builtin_va_list support is
inadequate for this because va_list is defined differently in the Win64
ABI vs. the System V/AMD64 ABI.

Depends on D1622.

Diff Detail

Repository
rL LLVM

Event Timeline

cdavis5x updated this revision to Unknown Object (????).Sep 9 2013, 6:54 PM
  • Nuke __builtin_ms_va_arg. GCC doesn't have it; they use the normal __builtin_va_arg instead. (Yes, this is horrifying.)
  • Add tests for serialization and templates.

This seems very weird to me. Can you explain a bit about the background for this? Is this to support writing variadic attribute((ms_abi)) functions when not targeting Win64? How is this is supposed to work when we *are* targeting Win64 directly? (Do we get a corresponding __builtin_sysv_va_list to go in the opposite direction, for instance? Are the two types the same in that case?)

include/clang/Serialization/ASTBitCodes.h
1343 ↗(On Diff #4153)

Why are you using a separate StmtCode here, rather than just serializing the flag normally?

@rsmith wrote:

This seems very weird to me. Can you explain a bit about the background for this?

Certainly.

Is this to support writing variadic attribute((ms_abi)) functions when not targeting Win64?

Yes. Some software in the wild (Wine, for instance) has variadic ms_abi functions. (I wouldn't be surprised, frankly, if this were originally implemented in GCC at the behest of Wine. I know that the ms_hook_prologue attribute was.)

How is this is supposed to work when we *are* targeting Win64 directly?

Exactly the same as a normal va_list. I even added a test for this.

(Do we get a corresponding __builtin_sysv_va_list to go in the opposite direction, for instance? Are the two types the same in that case?)

We don't. Obviously, this is something GCC overlooked--probably because it was implemented specifically so Wine could use it (see above). (To be honest, I contemplated this myself before submitting. I wanted to add an orthogonal extension for va_lists in a sysv_abi function. But I decided that was better suited for another patch--this one's big enough as it is.)

include/clang/Serialization/ASTBitCodes.h
1343 ↗(On Diff #4153)

I think it was because I didn't want to break backwards compatibility with the existing encoding of clang::VAArgExpr in a PCH/module. (That's going to become important later when modules are ready for primetime.) But, if you want me to serialize the bit normally, I can go do that.

cdavis5x updated this revision to Unknown Object (????).Sep 12 2013, 5:21 PM

Serialize the isMicrosoftABI bit normally.

ping/update? this prevents compiling Wine with Clang

error: unknown type name '__builtin_ms_va_list';
rnk edited edge metadata.Jul 27 2015, 11:18 AM

We can easily codegen __builtin_ms_va_start() and end() to llvm.vastart / llvm.vaend, because the first *has* to live in the win64 function and the second is a no-op. We should remove the FIXME about lowering __builtin_ms_va_arg() in LLVM instead of clang, because you can use it from a SysV function and it should work: consider implementing Win64 printf by calling an internal SysV vfprintf implementation. We also need to implement __builtin_ms_va_copy() in Clang for the same reason, but that's easy, since it's always a simple pointer load and store.

Charles, are you actively working on this or should I just fix up the patch and submit it?

In D1623#212706, @rnk wrote:

We can easily codegen __builtin_ms_va_start() and end() to llvm.vastart / llvm.vaend, because the first *has* to live in the win64 function and the second is a no-op. We should remove the FIXME about lowering __builtin_ms_va_arg() in LLVM instead of clang, because you can use it from a SysV function and it should work: consider implementing Win64 printf by calling an internal SysV vfprintf implementation. We also need to implement __builtin_ms_va_copy() in Clang for the same reason, but that's easy, since it's always a simple pointer load and store.

Charles, are you actively working on this or should I just fix up the patch and submit it?

Stand by. I have a new version of this patch to upload that does all those things.

cdavis5x updated this revision to Diff 30719.Jul 27 2015, 12:28 PM
cdavis5x edited edge metadata.

Big belated update:

  • Rebase to r243280.
  • Go back to lowering va_arg()/va_copy() manually.
NOTE: Still needs D1622. (I will update that shortly.)
rnk added inline comments.Jul 27 2015, 2:30 PM
include/clang/AST/Expr.h
3724–3726 ↗(On Diff #30719)

This seems unfortunate. Richard, why do we even have a VAArgExpr instead of just leaving it as a builtin CallExpr like __builtin_va_start? Just history?

cdavis5x updated this revision to Diff 33490.Aug 28 2015, 4:27 PM
  • Rebase onto rL246348.
  • Register the __builtin_ms_va_list predef decl. Fixes a nasty interaction with modules.
  • Mark __builtin_ms_va_start builtin as manually type-checked.
rsmith added inline comments.Aug 28 2015, 6:53 PM
include/clang/AST/Expr.h
3709 ↗(On Diff #33490)

Is there a reason to add a default argument for this? It looks like all calls provide an argument.

3720–3722 ↗(On Diff #33490)

The IsMS flag you added here is never used.

3728–3734 ↗(On Diff #33490)

Probably because its second argument is a type, and we couldn't preserve that and maintain AST fidelity if we represented it as a CallExpr.

lib/CodeGen/CGExprScalar.cpp
3387–3392 ↗(On Diff #33490)

Do you really need this dispatch? CodeGenFunction dispatches this to the function's ABI info class, which should do the right thing already.

lib/Sema/SemaChecking.cpp
1038–1040 ↗(On Diff #33490)

Can we do better than this? I think it'd be better to check that __builtin_ms_va_start only occurs in a function using the MS ABI's variadic calling convention, and __builtin_va_start only occurs in a function using the normal variadic C calling convention.

rsmith added inline comments.Aug 28 2015, 6:56 PM
lib/CodeGen/CGExprScalar.cpp
3387–3392 ↗(On Diff #33490)

Hmm, scratch that, it's entirely possible for an MS VA list to get passed into a non-MS-ABI function and then to have __builtin_va_arg called on it.

cdavis5x updated this revision to Diff 34012.Sep 3 2015, 9:03 PM

Address @rsmith's comments.

cdavis5x added inline comments.Sep 3 2015, 9:04 PM
include/clang/AST/Expr.h
3709 ↗(On Diff #33490)

Nope. The default arg is gone now.

3720–3722 ↗(On Diff #33490)

Removed. (I don't know what I was thinking with that...)

lib/Sema/SemaChecking.cpp
1038–1040 ↗(On Diff #33490)

Done.

cdavis5x updated this revision to Diff 34415.Sep 10 2015, 12:14 AM
  • Rebase onto rL247238.
  • Run clang-format on this patch.
  • Push va_arg emission down into the ABIInfo hierarchy.

I threaded the IsMS parameter from the VAArgExpr through the
EmitVAArg methods. I'm not entirely happy about this design; I'm open
to suggestions. One thing this does help, though, is supporting this
on other architectures (ARM, maybe?).

rnk added inline comments.Sep 10 2015, 8:51 AM
lib/CodeGen/CGCall.cpp
3598 ↗(On Diff #34415)

I think keeping the va_arg logic in TargetInfo.cpp is good, but we don't need to thread IsMS through every EmitVAArg override. Instead, we can do something like this here:

if (IsMS)
  return CGM.getTypes().getABIInfo().EmitMSVAArg(*this, VAListAddr, Ty);
return CGM.getTypes().getABIInfo().EmitVAArg(*this, VAListAddr, Ty);
rsmith added inline comments.Sep 10 2015, 8:28 PM
lib/Sema/SemaExpr.cpp
11664–11672 ↗(On Diff #34415)

If __builtin_va_list and __builtin_ms_va_list are the same type, this will set IsMS to true, which is not wrong per se but seems a bit surprising. (IsMS is the "I'm using an unnatural ABI" flag, and I think it'd be a bit better to not set it for normal va_start / va_arg / va_end, even when targeting the MS ABI. Thoughts?

rjmccall edited edge metadata.Sep 10 2015, 9:35 PM

This is a kindof weird extension conceptually. "Emulate the C ABI that Microsoft would use on the current architecture"? I guess we're lucky that they never support two different C ABIs on the same ISA. But okay, whatever, it's a thing.

A couple comments:

lib/CodeGen/CGCall.cpp
3598 ↗(On Diff #34415)

I agree, especially because TargetInfo.cpp is partially maintained by backend authors; let's not force them all to know about this weird extension.

lib/CodeGen/CGExprAgg.cpp
966 ↗(On Diff #34415)

We have exactly this same pattern of code in three different places, and it's kindof begging for somebody to add another emitter that's missing the MS ABI site. Maybe instead of having EmitVAListRef as a separate function, we should just have this:

Address CGF::EmitVAArgExpr(VAArgExpr *E)

and it can check the ABI and do the right combination of things.

cdavis5x updated this revision to Diff 34728.Sep 14 2015, 1:43 PM
cdavis5x edited edge metadata.

Address review comments.

  • Pull out va_arg emission on __builtin_ms_va_arg to its own method on ABIInfo. That way, ABI implementers don't need to care that this extension even exists.
  • Attempt to push repeated va_arg emission code in CGExpr{Scalar,Agg,Complex}.cpp down into CodeGenFunction::EmitVAArg.
  • Don't ever consider a VAArgExpr to be a Microsoft one when the native va_list happens to be the same as the MS one.

Also:

  • Rebase onto rL247603.
  • Add a test that we lower va_arg() and va_copy() correctly even with a Win64 va_list inside a System V function.
cdavis5x added inline comments.Sep 14 2015, 1:44 PM
lib/CodeGen/CGCall.cpp
3598 ↗(On Diff #34415)

Done.

lib/CodeGen/CGExprAgg.cpp
966 ↗(On Diff #34415)

Done.

I kept EmitVAListRef because it's used elsewhere. Also, I had to add an out parameter to EmitVAArg for the va_list reference, because some callers (like this one!) do something with it if EmitVAArg fails.

lib/Sema/SemaExpr.cpp
11664–11672 ↗(On Diff #34415)

I agree. Fixed.

rnk accepted this revision.Sep 17 2015, 1:40 PM
rnk edited edge metadata.

I think this is ready.

This revision is now accepted and ready to land.Sep 17 2015, 1:40 PM
cdavis5x closed this revision.Sep 17 2015, 1:58 PM
This revision was automatically updated to reflect the committed changes.