[PATCH] D1623: Support __builtin_ms_va_list.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/Expr.h:3709
@@ -3708,3 +3708,3 @@
   VAArgExpr(SourceLocation BLoc, Expr* e, TypeSourceInfo *TInfo,
-            SourceLocation RPLoc, QualType t)
+            SourceLocation RPLoc, QualType t, bool IsMS = false)
     : Expr(VAArgExprClass, t, VK_RValue, OK_Ordinary,
----------------
Is there a reason to add a default argument for this? It looks like all calls provide an argument.

================
Comment at: include/clang/AST/Expr.h:3720-3722
@@ -3719,4 +3719,5 @@
 
-  /// \brief Create an empty __builtin_va_arg expression.
-  explicit VAArgExpr(EmptyShell Empty) : Expr(VAArgExprClass, Empty) { }
+  /// Create an empty __builtin_va_arg expression.
+  explicit VAArgExpr(EmptyShell Empty, bool IsMS = false)
+    : Expr(VAArgExprClass, Empty), Val(0), TInfo(0, IsMS) { }
 
----------------
The `IsMS` flag you added here is never used.

================
Comment at: include/clang/AST/Expr.h:3728-3734
@@ -3726,5 +3727,9 @@
 
-  TypeSourceInfo *getWrittenTypeInfo() const { return TInfo; }
-  void setWrittenTypeInfo(TypeSourceInfo *TI) { TInfo = TI; }
+  /// Returns whether this is really a Win64 ABI va_arg expression.
+  bool isMicrosoftABI() const { return TInfo.getInt(); }
+  void setIsMicrosoftABI(bool IsMS) { TInfo.setInt(IsMS); }
+
+  TypeSourceInfo *getWrittenTypeInfo() const { return TInfo.getPointer(); }
+  void setWrittenTypeInfo(TypeSourceInfo *TI) { TInfo.setPointer(TI); }
 
   SourceLocation getBuiltinLoc() const { return BuiltinLoc; }
----------------
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`.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3387-3392
@@ -3386,4 +3386,8 @@
 
-  llvm::Value *ArgValue = CGF.EmitVAListRef(VE->getSubExpr());
-  llvm::Value *ArgPtr = CGF.EmitVAArg(ArgValue, VE->getType());
+  llvm::Value *ArgValue = VE->isMicrosoftABI() ?
+    CGF.EmitMSVAListRef(VE->getSubExpr()) :
+    CGF.EmitVAListRef(VE->getSubExpr());
+  llvm::Value *ArgPtr = VE->isMicrosoftABI() ?
+    CGF.EmitMSVAArg(ArgValue, VE->getType()) :
+    CGF.EmitVAArg(ArgValue, VE->getType());
   llvm::Type *ArgTy = ConvertType(VE->getType());
----------------
Do you really need this dispatch? `CodeGenFunction` dispatches this to the function's ABI info class, which should do the right thing already.

================
Comment at: lib/Sema/SemaChecking.cpp:1038-1040
@@ -1037,1 +1037,5 @@
+  case X86::BI__builtin_ms_va_start:
+    if (Context.getTargetInfo().getTriple().getArch() != llvm::Triple::x86_64)
+      return false;
+    return SemaBuiltinVAStart(TheCall);
   case X86::BI_mm_prefetch: i = 1; l = 0; u = 3; break;
----------------
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.


http://reviews.llvm.org/D1623