Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x updated this revision to Diff 34415.
cdavis5x added a comment.

- Rebase onto http://reviews.llvm.org/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?).


http://reviews.llvm.org/D1623

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Expr.h
  include/clang/Basic/BuiltinsX86.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/TargetInfo.h
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDiagnostic.cpp
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/ABIInfo.h
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprComplex.cpp
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/CodeGen/ms_abi.c
  test/PCH/Inputs/va_arg.h
  test/PCH/va_arg.c
  test/PCH/va_arg.cpp
  test/PCH/va_arg.h
  test/Sema/varargs-win64.c
  test/Sema/varargs-x86-32.c
  test/Sema/varargs-x86-64.c
  test/SemaTemplate/instantiate-expr-3.cpp

Index: test/SemaTemplate/instantiate-expr-3.cpp
===
--- test/SemaTemplate/instantiate-expr-3.cpp
+++ test/SemaTemplate/instantiate-expr-3.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
 
 // -
 // Imaginary literals
@@ -108,12 +108,41 @@
 struct VaArg1 {
   void f(int n, ...) {
 VaList va;
-__builtin_va_start(va, n); // expected-error{{int}}
+__builtin_va_start(va, n); // expected-error{{int}} expected-error{{char *}}
 for (int i = 0; i != n; ++i)
   (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
-__builtin_va_end(va); // expected-error{{int}}
+__builtin_va_end(va);  // expected-error{{int}} expected-error{{char *}}
   }
 };
 
 template struct VaArg1<__builtin_va_list, int>;
+template struct VaArg1<__builtin_ms_va_list, int>; // expected-note{{instantiation}}
 template struct VaArg1; // expected-note{{instantiation}}
+
+template 
+struct VaArg2 {
+  void __attribute__((ms_abi)) f(int n, ...) {
+__builtin_ms_va_list va;
+__builtin_ms_va_start(va, n);
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType);
+__builtin_ms_va_end(va);
+  }
+};
+
+template struct VaArg2;
+
+template 
+struct VaArg3 {
+  void __attribute__((ms_abi)) f(int n, ...) {
+VaList va;
+__builtin_ms_va_start(va, n); // expected-error{{int}} expected-error{{__va_list_tag}}
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
+__builtin_ms_va_end(va);   // expected-error{{int}} expected-error{{__va_list_tag}}
+  }
+};
+
+template struct VaArg3<__builtin_ms_va_list, int>;
+template struct VaArg3<__builtin_va_list, int>; // expected-note{{instantiation}}
+template struct VaArg3;   // expected-note{{instantiation}}
Index: test/Sema/varargs-x86-64.c
===
--- test/Sema/varargs-x86-64.c
+++ test/Sema/varargs-x86-64.c
@@ -6,3 +6,77 @@
   (void)__builtin_va_arg(args2, int); // expected-error {{first argument to 'va_arg' is of type 'const __builtin_va_list' and not 'va_list'}}
 }
 
+void f2(int a, ...) {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap, a); // expected-error {{'__builtin_ms_va_start' used in System V ABI function}}
+}
+
+void __attribute__((ms_abi)) g1(int a) {
+  __builtin_ms_va_list ap;
+
+  __builtin_ms_va_start(ap, a, a); // expected-error {{too many arguments to function}}
+  __builtin_ms_va_start(ap, a);// expected-error {{'va_start' used in function with fixed args}}
+}
+
+void __attribute__((ms_abi)) g2(int a, int b, ...) {
+  __builtin_ms_va_list ap;
+
+  __builtin_ms_va_start(ap, 10); // expected-warning {{second parameter of 'va_start' not last named argument}}
+  __builtin_ms_va_start(ap, a);  // expected-warning {{second parameter of 'va_start' not last named argument}}
+  __builtin_ms_va_start(ap, b);
+}
+
+void __attribute__((ms_abi)) g3(float a, ...) {
+  __builtin_ms_va_list ap;
+
+  __builtin_ms_va_start(ap, a);
+  __builtin_ms_va_start(ap, (a));
+}
+
+void __attribute__((ms_abi)) g5() {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap, ap); // expected-error {{'va_start' used in function wi

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Reid Kleckner via cfe-commits]

rnk 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);
 }

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);


http://reviews.llvm.org/D1623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Richard Smith via cfe-commits]

rsmith added inline comments.


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;
+}
+  }
 

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?


http://reviews.llvm.org/D1623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x updated this revision to Diff 34728.
cdavis5x added a comment.

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 http://reviews.llvm.org/rL247603.
- Add a test that we lower `va_arg()` and `va_copy()` correctly even with a 
Win64 `va_list` inside a System V function.


http://reviews.llvm.org/D1623

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Expr.h
  include/clang/Basic/BuiltinsX86.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/TargetInfo.h
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDiagnostic.cpp
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/ABIInfo.h
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprComplex.cpp
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/CodeGen/ms_abi.c
  test/PCH/Inputs/va_arg.h
  test/PCH/va_arg.c
  test/PCH/va_arg.cpp
  test/PCH/va_arg.h
  test/Sema/varargs-win64.c
  test/Sema/varargs-x86-32.c
  test/Sema/varargs-x86-64.c
  test/SemaTemplate/instantiate-expr-3.cpp

Index: test/SemaTemplate/instantiate-expr-3.cpp
===
--- test/SemaTemplate/instantiate-expr-3.cpp
+++ test/SemaTemplate/instantiate-expr-3.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
 
 // -
 // Imaginary literals
@@ -108,12 +108,41 @@
 struct VaArg1 {
   void f(int n, ...) {
 VaList va;
-__builtin_va_start(va, n); // expected-error{{int}}
+__builtin_va_start(va, n); // expected-error{{int}} expected-error{{char *}}
 for (int i = 0; i != n; ++i)
   (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
-__builtin_va_end(va); // expected-error{{int}}
+__builtin_va_end(va); // expected-error{{int}} expected-error{{char *}}
   }
 };
 
 template struct VaArg1<__builtin_va_list, int>;
+template struct VaArg1<__builtin_ms_va_list, int>; // expected-note{{instantiation}}
 template struct VaArg1; // expected-note{{instantiation}}
+
+template
+struct VaArg2 {
+  void __attribute__((ms_abi)) f(int n, ...) {
+__builtin_ms_va_list va;
+__builtin_ms_va_start(va, n);
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType);
+__builtin_ms_va_end(va);
+  }
+};
+
+template struct VaArg2;
+
+template
+struct VaArg3 {
+  void __attribute__((ms_abi)) f(int n, ...) {
+VaList va;
+__builtin_ms_va_start(va, n); // expected-error{{int}} expected-error{{__va_list_tag}}
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
+__builtin_ms_va_end(va); // expected-error{{int}} expected-error{{__va_list_tag}}
+  }
+};
+
+template struct VaArg3<__builtin_ms_va_list, int>;
+template struct VaArg3<__builtin_va_list, int>; // expected-note{{instantiation}}
+template struct VaArg3; // expected-note{{instantiation}}
Index: test/Sema/varargs-x86-64.c
===
--- test/Sema/varargs-x86-64.c
+++ test/Sema/varargs-x86-64.c
@@ -6,3 +6,75 @@
   (void)__builtin_va_arg(args2, int); // expected-error {{first argument to 'va_arg' is of type 'const __builtin_va_list' and not 'va_list'}}
 }
 
+void f2(int a, ...) {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap, a); // expected-error {{'__builtin_ms_va_start' used in System V ABI function}}
+}
+
+void __attribute__((ms_abi)) g1(int a) {
+  __builtin_ms_va_list ap;
+
+  __builtin_ms_va_start(ap, a, a); // expected-error {{too many arguments to function}}
+  __builtin_ms_va_start(ap, a); // expected-error {{'va_start' used in function with fixed args}}
+}
+
+void __attribute__((ms_abi)) g2(int a, int b, ...) {
+  __builtin_ms_va_list ap;
+
+  __builtin_ms_va_start(ap, 10); // expected-warning {{second parameter of 'va_start' not last named argument}}
+  __builtin_ms_va_start(ap, a); // expected-warning {{second parameter of 'va_start' not last named argument}}
+  __builtin_ms_va_start(ap, b);
+}
+
+void __attribute__((ms_abi)) g3(float a, ...) {
+  __builtin_ms_va_list ap;
+
+  __builtin_ms_va_start(ap, a);
+  __builti

Re: [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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x added a comment.

Ping...


http://reviews.llvm.org/D1623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Reid Kleckner via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

I think this is ready.


http://reviews.llvm.org/D1623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL247941: Support __builtin_ms_va_list. (authored by cdavis).

Changed prior to commit:
  http://reviews.llvm.org/D1623?vs=34728&id=35036#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D1623

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/Basic/BuiltinsX86.def
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Basic/TargetInfo.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/include/clang/Serialization/ASTBitCodes.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/AST/ASTDiagnostic.cpp
  cfe/trunk/lib/Basic/TargetInfo.cpp
  cfe/trunk/lib/Basic/Targets.cpp
  cfe/trunk/lib/CodeGen/ABIInfo.h
  cfe/trunk/lib/CodeGen/CGBuiltin.cpp
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/CodeGen/CGExprAgg.cpp
  cfe/trunk/lib/CodeGen/CGExprComplex.cpp
  cfe/trunk/lib/CodeGen/CGExprScalar.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.h
  cfe/trunk/lib/CodeGen/TargetInfo.cpp
  cfe/trunk/lib/Sema/Sema.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
  cfe/trunk/test/CodeGen/ms_abi.c
  cfe/trunk/test/PCH/Inputs/va_arg.h
  cfe/trunk/test/PCH/va_arg.c
  cfe/trunk/test/PCH/va_arg.cpp
  cfe/trunk/test/PCH/va_arg.h
  cfe/trunk/test/Sema/varargs-win64.c
  cfe/trunk/test/Sema/varargs-x86-32.c
  cfe/trunk/test/Sema/varargs-x86-64.c
  cfe/trunk/test/SemaTemplate/instantiate-expr-3.cpp

Index: cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
+++ cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
@@ -771,6 +771,7 @@
   Writer.AddTypeSourceInfo(E->getWrittenTypeInfo(), Record);
   Writer.AddSourceLocation(E->getBuiltinLoc(), Record);
   Writer.AddSourceLocation(E->getRParenLoc(), Record);
+  Record.push_back(E->isMicrosoftABI());
   Code = serialization::EXPR_VA_ARG;
 }
 
Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -4084,6 +4084,8 @@
  PREDEF_DECL_OBJC_INSTANCETYPE_ID);
   RegisterPredefDecl(Context.BuiltinVaListDecl, PREDEF_DECL_BUILTIN_VA_LIST_ID);
   RegisterPredefDecl(Context.VaListTagDecl, PREDEF_DECL_VA_LIST_TAG);
+  RegisterPredefDecl(Context.BuiltinMSVaListDecl,
+ PREDEF_DECL_BUILTIN_MS_VA_LIST_ID);
   RegisterPredefDecl(Context.ExternCContext, PREDEF_DECL_EXTERN_C_CONTEXT_ID);
 
   // Build a record containing all of the tentative definitions in this file, in
Index: cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
@@ -830,6 +830,7 @@
   E->setWrittenTypeInfo(GetTypeSourceInfo(Record, Idx));
   E->setBuiltinLoc(ReadSourceLocation(Record, Idx));
   E->setRParenLoc(ReadSourceLocation(Record, Idx));
+  E->setIsMicrosoftABI(Record[Idx++]);
 }
 
 void ASTStmtReader::VisitAddrLabelExpr(AddrLabelExpr *E) {
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -6270,6 +6270,9 @@
   case PREDEF_DECL_VA_LIST_TAG:
 return Context.getVaListTagDecl();
 
+  case PREDEF_DECL_BUILTIN_MS_VA_LIST_ID:
+return Context.getBuiltinMSVaListDecl();
+
   case PREDEF_DECL_EXTERN_C_CONTEXT_ID:
 return Context.getExternCContextDecl();
   }
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -61,6 +61,11 @@
   /*ByRef*/ false, Realign);
 }
 
+Address ABIInfo::EmitMSVAArg(CodeGenFunction &CGF, Address VAListAddr,
+ QualType Ty) const {
+  return Address::invalid();
+}
+
 ABIInfo::~ABIInfo() {}
 
 static CGCXXABI::RecordArgABI getRecordArgABI(const RecordType *RT,
@@ -1734,6 +1739,8 @@
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
 QualType Ty) const override;
+  Address EmitMSVAArg(CodeGenFunction &CGF, Address VAListAddr,
+  QualType Ty) const override;
 
   bool has64BitPointers() const {
 return Has64BitPointers;
@@ -3266,6 +3273,14 @@
   return ResAddr;
 }
 
+Address X86_64ABIInfo::EmitMSVAArg(CodeGenFunction &CGF, Address VAListAddr,
+   QualType Ty) const {
+  return emit

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x updated this revision to Diff 33490.
cdavis5x added a comment.

- Rebase onto http://reviews.llvm.org/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.


http://reviews.llvm.org/D1623

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Expr.h
  include/clang/Basic/BuiltinsX86.def
  include/clang/Basic/TargetInfo.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDiagnostic.cpp
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprComplex.cpp
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/CodeGen/ms_abi.c
  test/PCH/Inputs/va_arg.h
  test/PCH/va_arg.c
  test/PCH/va_arg.cpp
  test/PCH/va_arg.h
  test/Sema/varargs-x86-64.c
  test/SemaTemplate/instantiate-expr-3.cpp

Index: test/SemaTemplate/instantiate-expr-3.cpp
===
--- test/SemaTemplate/instantiate-expr-3.cpp
+++ test/SemaTemplate/instantiate-expr-3.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
 
 // -
 // Imaginary literals
@@ -108,12 +108,41 @@
 struct VaArg1 {
   void f(int n, ...) {
 VaList va;
-__builtin_va_start(va, n); // expected-error{{int}}
+__builtin_va_start(va, n); // expected-error{{int}} expected-error{{char *}}
 for (int i = 0; i != n; ++i)
   (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
-__builtin_va_end(va); // expected-error{{int}}
+__builtin_va_end(va); // expected-error{{int}} expected-error{{char *}}
   }
 };
 
 template struct VaArg1<__builtin_va_list, int>;
+template struct VaArg1<__builtin_ms_va_list, int>; // expected-note{{instantiation}}
 template struct VaArg1; // expected-note{{instantiation}}
+
+template
+struct VaArg2 {
+  void f(int n, ...) {
+__builtin_ms_va_list va;
+__builtin_ms_va_start(va, n);
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType);
+__builtin_ms_va_end(va);
+  }
+};
+
+template struct VaArg2;
+
+template
+struct VaArg3 {
+  void f(int n, ...) {
+VaList va;
+__builtin_ms_va_start(va, n); // expected-error{{int}} expected-error{{__va_list_tag}}
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
+__builtin_ms_va_end(va); // expected-error{{int}} expected-error{{__va_list_tag}}
+  }
+};
+
+template struct VaArg3<__builtin_ms_va_list, int>;
+template struct VaArg3<__builtin_va_list, int>; // expected-note{{instantiation}}
+template struct VaArg3; // expected-note{{instantiation}}
Index: test/Sema/varargs-x86-64.c
===
--- test/Sema/varargs-x86-64.c
+++ test/Sema/varargs-x86-64.c
@@ -6,3 +6,70 @@
   (void)__builtin_va_arg(args2, int); // expected-error {{first argument to 'va_arg' is of type 'const __builtin_va_list' and not 'va_list'}}
 }
 
+void __attribute__((ms_abi)) g1(int a)
+{
+__builtin_ms_va_list ap;
+
+__builtin_ms_va_start(ap, a, a); // expected-error {{too many arguments to function}}
+__builtin_ms_va_start(ap, a); // expected-error {{'va_start' used in function with fixed args}}
+}
+
+void __attribute__((ms_abi)) g2(int a, int b, ...)
+{
+__builtin_ms_va_list ap;
+
+__builtin_ms_va_start(ap, 10); // expected-warning {{second parameter of 'va_start' not last named argument}}
+__builtin_ms_va_start(ap, a); // expected-warning {{second parameter of 'va_start' not last named argument}}
+__builtin_ms_va_start(ap, b);
+}
+
+void __attribute__((ms_abi)) g3(float a, ...)
+{
+__builtin_ms_va_list ap;
+
+__builtin_ms_va_start(ap, a);
+__builtin_ms_va_start(ap, (a));
+}
+
+void __attribute__((ms_abi)) g5() {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap,ap); // expected-error {{'va_start' used in function with fixed args}}
+}
+
+void __attribute__((ms_abi)) g6(int a, ...) {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap); // expected-error {{too few arguments to function}}
+}
+
+void __attribute__((ms_abi))
+bar(__builtin_ms_va_list authors, ...) {
+  __builtin_ms_va_start (authors, authors);
+  (void)__builtin_va_arg(authors, int);
+  __builtin_ms_va_end (authors);
+}
+
+void __attribute__((ms_abi)) g7(int a, ...) {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap, a);
+  // FIXME: This error message is sub-par.
+  __builtin_va_arg(ap, int) = 1; // expected-er

Re: [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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Richard Smith via cfe-commits]

rsmith added inline comments.


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());

rsmith wrote:
> Do you really need this dispatch? `CodeGenFunction` dispatches this to the 
> function's ABI info class, which should do the right thing already.
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.


http://reviews.llvm.org/D1623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x updated this revision to Diff 34012.
cdavis5x added a comment.

Address @rsmith's comments.


http://reviews.llvm.org/D1623

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Expr.h
  include/clang/Basic/BuiltinsX86.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/TargetInfo.h
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDiagnostic.cpp
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprComplex.cpp
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/CodeGen/ms_abi.c
  test/PCH/Inputs/va_arg.h
  test/PCH/va_arg.c
  test/PCH/va_arg.cpp
  test/PCH/va_arg.h
  test/Sema/varargs-win64.c
  test/Sema/varargs-x86-32.c
  test/Sema/varargs-x86-64.c
  test/SemaTemplate/instantiate-expr-3.cpp

Index: test/SemaTemplate/instantiate-expr-3.cpp
===
--- test/SemaTemplate/instantiate-expr-3.cpp
+++ test/SemaTemplate/instantiate-expr-3.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
 
 // -
 // Imaginary literals
@@ -108,12 +108,41 @@
 struct VaArg1 {
   void f(int n, ...) {
 VaList va;
-__builtin_va_start(va, n); // expected-error{{int}}
+__builtin_va_start(va, n); // expected-error{{int}} expected-error{{char *}}
 for (int i = 0; i != n; ++i)
   (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
-__builtin_va_end(va); // expected-error{{int}}
+__builtin_va_end(va); // expected-error{{int}} expected-error{{char *}}
   }
 };
 
 template struct VaArg1<__builtin_va_list, int>;
+template struct VaArg1<__builtin_ms_va_list, int>; // expected-note{{instantiation}}
 template struct VaArg1; // expected-note{{instantiation}}
+
+template
+struct VaArg2 {
+  void __attribute__((ms_abi)) f(int n, ...) {
+__builtin_ms_va_list va;
+__builtin_ms_va_start(va, n);
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType);
+__builtin_ms_va_end(va);
+  }
+};
+
+template struct VaArg2;
+
+template
+struct VaArg3 {
+  void __attribute__((ms_abi)) f(int n, ...) {
+VaList va;
+__builtin_ms_va_start(va, n); // expected-error{{int}} expected-error{{__va_list_tag}}
+for (int i = 0; i != n; ++i)
+  (void)__builtin_va_arg(va, ArgType); // expected-error{{int}}
+__builtin_ms_va_end(va); // expected-error{{int}} expected-error{{__va_list_tag}}
+  }
+};
+
+template struct VaArg3<__builtin_ms_va_list, int>;
+template struct VaArg3<__builtin_va_list, int>; // expected-note{{instantiation}}
+template struct VaArg3; // expected-note{{instantiation}}
Index: test/Sema/varargs-x86-64.c
===
--- test/Sema/varargs-x86-64.c
+++ test/Sema/varargs-x86-64.c
@@ -6,3 +6,80 @@
   (void)__builtin_va_arg(args2, int); // expected-error {{first argument to 'va_arg' is of type 'const __builtin_va_list' and not 'va_list'}}
 }
 
+void f2(int a, ...) {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap, a); // expected-error {{'__builtin_ms_va_start' used in System V ABI function}}
+}
+
+void __attribute__((ms_abi)) g1(int a)
+{
+__builtin_ms_va_list ap;
+
+__builtin_ms_va_start(ap, a, a); // expected-error {{too many arguments to function}}
+__builtin_ms_va_start(ap, a); // expected-error {{'va_start' used in function with fixed args}}
+}
+
+void __attribute__((ms_abi)) g2(int a, int b, ...)
+{
+__builtin_ms_va_list ap;
+
+__builtin_ms_va_start(ap, 10); // expected-warning {{second parameter of 'va_start' not last named argument}}
+__builtin_ms_va_start(ap, a); // expected-warning {{second parameter of 'va_start' not last named argument}}
+__builtin_ms_va_start(ap, b);
+}
+
+void __attribute__((ms_abi)) g3(float a, ...)
+{
+__builtin_ms_va_list ap;
+
+__builtin_ms_va_start(ap, a);
+__builtin_ms_va_start(ap, (a));
+}
+
+void __attribute__((ms_abi)) g5() {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap,ap); // expected-error {{'va_start' used in function with fixed args}}
+}
+
+void __attribute__((ms_abi)) g6(int a, ...) {
+  __builtin_ms_va_list ap;
+  __builtin_ms_va_start(ap); // expected-error {{too few arguments to function}}
+}
+
+void __attribute__((ms_abi))
+bar(__builtin_ms_va_list authors, ...) {
+  __builtin_ms_va_start (authors, authors);
+  (void)__builtin_va_arg(authors, int);
+  __builtin_ms_va_end (authors);
+}
+
+void __attribute__((ms_abi)) g7(i

Re: [PATCH] D1623: Support __builtin_ms_va_list.

[Charles Davis via cfe-commits]

cdavis5x 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,

rsmith wrote:
> Is there a reason to add a default argument for this? It looks like all calls 
> provide an argument.
Nope. The default arg is gone now.


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) { }
 

rsmith wrote:
> The `IsMS` flag you added here is never used.
Removed. (I don't know what I was thinking with that...)


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;

rsmith wrote:
> 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.
Done.


http://reviews.llvm.org/D1623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits