Re: [PATCH] D1623: Support __builtin_ms_va_list.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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