[PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x added inline 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);
 }
----------------
rjmccall wrote:
> 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.
Done.

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

================
Comment at: lib/Sema/SemaExpr.cpp:11664-11672
@@ -11662,1 +11663,11 @@
+
+  // It might be a __builtin_ms_va_list.
+  if (!E->isTypeDependent() && Context.getTargetInfo().hasBuiltinMSVaList()) {
+    QualType MSVaListType = Context.getBuiltinMSVaListType();
+    if (Context.hasSameType(MSVaListType, E->getType())) {
+      if (CheckForModifiableLvalue(E, BuiltinLoc, *this))
+        return ExprError();
+      IsMS = true;
+    }
+  }
 
----------------
rsmith wrote:
> 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?
I agree. Fixed.


http://reviews.llvm.org/D1623