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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
