[PATCH] D1623: Support __builtin_ms_va_list.

[John McCall via cfe-commits]

rjmccall added a comment.

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:


================
Comment at: lib/CodeGen/CGCall.cpp:3598
@@ -3599,1 +3597,3 @@
+Address CodeGenFunction::EmitVAArg(Address VAListAddr, QualType Ty, bool IsMS) {
+  return CGM.getTypes().getABIInfo().EmitVAArg(*this, VAListAddr, Ty, IsMS);
 }
----------------
rnk wrote:
> 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);
I agree, especially because TargetInfo.cpp is partially maintained by backend authors; let's not force them all to know about this weird extension.

================
Comment at: lib/CodeGen/CGExprAgg.cpp:966
@@ -965,1 +965,3 @@
+                         : CGF.EmitVAListRef(VE->getSubExpr());
+  Address ArgPtr = CGF.EmitVAArg(ArgValue, VE->getType(), VE->isMicrosoftABI());
 
----------------
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.


http://reviews.llvm.org/D1623