Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
This revision was automatically updated to reflect the committed changes.
Closed by commit rL272971: [CodeGen] Fix a segfault caused by pass_object_size.
(authored by gbiv).
Changed prior to commit:
http://reviews.llvm.org/D17462?vs=61041=61048#toc
Repository:
rL LLVM
http://reviews.llvm.org/D17462
Files:
cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CGExprCXX.cpp
cfe/trunk/lib/CodeGen/CGVTables.cpp
cfe/trunk/test/CodeGen/pass-object-size.c
cfe/trunk/test/CodeGenCXX/pass-object-size.cpp
Index: cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h
===
--- cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h
+++ cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h
@@ -16,17 +16,17 @@
#ifndef LLVM_CLANG_CODEGEN_CGFUNCTIONINFO_H
#define LLVM_CLANG_CODEGEN_CGFUNCTIONINFO_H
+#include "clang/AST/Attr.h"
#include "clang/AST/CanonicalType.h"
#include "clang/AST/CharUnits.h"
+#include "clang/AST/Decl.h"
#include "clang/AST/Type.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/ADT/FoldingSet.h"
#include "llvm/Support/TrailingObjects.h"
#include
namespace clang {
-class Decl;
-
namespace CodeGen {
/// ABIArgInfo - Helper class to encapsulate information about how a
@@ -393,23 +393,34 @@
/// Compute the arguments required by the given formal prototype,
/// given that there may be some additional, non-formal arguments
/// in play.
+ ///
+ /// If FD is not null, this will consider pass_object_size params in FD.
static RequiredArgs forPrototypePlus(const FunctionProtoType *prototype,
- unsigned additional) {
+ unsigned additional,
+ const FunctionDecl *FD) {
if (!prototype->isVariadic()) return All;
+if (FD)
+ additional += std::count_if(FD->param_begin(), FD->param_end(),
+ [](const ParmVarDecl *PVD) {
+return PVD->hasAttr();
+ });
return RequiredArgs(prototype->getNumParams() + additional);
}
- static RequiredArgs forPrototype(const FunctionProtoType *prototype) {
-return forPrototypePlus(prototype, 0);
+ static RequiredArgs forPrototype(const FunctionProtoType *prototype,
+ const FunctionDecl *FD) {
+return forPrototypePlus(prototype, 0, FD);
}
- static RequiredArgs forPrototype(CanQual prototype) {
-return forPrototype(prototype.getTypePtr());
+ static RequiredArgs forPrototype(CanQual prototype,
+ const FunctionDecl *FD) {
+return forPrototype(prototype.getTypePtr(), FD);
}
static RequiredArgs forPrototypePlus(CanQual prototype,
- unsigned additional) {
-return forPrototypePlus(prototype.getTypePtr(), additional);
+ unsigned additional,
+ const FunctionDecl *FD) {
+return forPrototypePlus(prototype.getTypePtr(), additional, FD);
}
bool allowsOptionalArgs() const { return NumRequired != ~0U; }
Index: cfe/trunk/test/CodeGenCXX/pass-object-size.cpp
===
--- cfe/trunk/test/CodeGenCXX/pass-object-size.cpp
+++ cfe/trunk/test/CodeGenCXX/pass-object-size.cpp
@@ -53,3 +53,30 @@
// CHECK: define void @_ZN8delegate1AC1EPvU17pass_object_size0({{[^,]*}}, i8*{{[^,]*}}, i64{{[^,]*}})
// CHECK: call void @_ZN8delegate1AC2EPvU17pass_object_size0({{[^,]*}}, i8*{{[^,]*}}, i64{{[^,]*}})
}
+
+namespace variadic {
+// We had an issue where variadic member/operator calls with pass_object_size
+// would cause crashes.
+
+struct AsCtor {
+ AsCtor(const char *const c __attribute__((pass_object_size(0))), double a,
+ ...) {}
+};
+
+struct AsMember {
+ void bar(const char *const c __attribute__((pass_object_size(0))), double a,
+ ...) {}
+ void operator()(const char *const c __attribute__((pass_object_size(0))),
+ double a, ...) {}
+};
+
+// CHECK-LABEL: define void @_ZN8variadic4testEv()
+void test() {
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic6AsCtorC1EPKcU17pass_object_size0dz
+ AsCtor("a", 1.0);
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMember3barEPKcU17pass_object_size0dz
+ AsMember{}.bar("a", 1.0);
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMemberclEPKcU17pass_object_size0dz
+ AsMember{}("a", 1.0);
+}
+}
Index: cfe/trunk/test/CodeGen/pass-object-size.c
===
--- cfe/trunk/test/CodeGen/pass-object-size.c
+++ cfe/trunk/test/CodeGen/pass-object-size.c
@@ -351,3 +351,18 @@
ObjectSize0(++p);
ObjectSize0(p++);
}
+
+// There was a bug where variadic functions with pass_object_size would cause
+// problems in the form of failed
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added inline comments. Comment at: lib/CodeGen/CGExprCXX.cpp:331 @@ -329,3 +330,3 @@ // And the rest of the call args EmitCallArgs(Args, FPT, E->arguments(), E->getDirectCallee()); return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required), rsmith wrote: > Seems inconsistent to pass `E->getDirectCallee()` here but not above. Since > you can't take the address of a pass_object_size function, do we need to pass > the callee in either place? Good point. :) http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv updated this revision to Diff 61041.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback.
http://reviews.llvm.org/D17462
Files:
include/clang/CodeGen/CGFunctionInfo.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CGVTables.cpp
test/CodeGen/pass-object-size.c
test/CodeGenCXX/pass-object-size.cpp
Index: test/CodeGenCXX/pass-object-size.cpp
===
--- test/CodeGenCXX/pass-object-size.cpp
+++ test/CodeGenCXX/pass-object-size.cpp
@@ -53,3 +53,30 @@
// CHECK: define void @_ZN8delegate1AC1EPvU17pass_object_size0({{[^,]*}}, i8*{{[^,]*}}, i64{{[^,]*}})
// CHECK: call void @_ZN8delegate1AC2EPvU17pass_object_size0({{[^,]*}}, i8*{{[^,]*}}, i64{{[^,]*}})
}
+
+namespace variadic {
+// We had an issue where variadic member/operator calls with pass_object_size
+// would cause crashes.
+
+struct AsCtor {
+ AsCtor(const char *const c __attribute__((pass_object_size(0))), double a,
+ ...) {}
+};
+
+struct AsMember {
+ void bar(const char *const c __attribute__((pass_object_size(0))), double a,
+ ...) {}
+ void operator()(const char *const c __attribute__((pass_object_size(0))),
+ double a, ...) {}
+};
+
+// CHECK-LABEL: define void @_ZN8variadic4testEv()
+void test() {
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic6AsCtorC1EPKcU17pass_object_size0dz
+ AsCtor("a", 1.0);
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMember3barEPKcU17pass_object_size0dz
+ AsMember{}.bar("a", 1.0);
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMemberclEPKcU17pass_object_size0dz
+ AsMember{}("a", 1.0);
+}
+}
Index: test/CodeGen/pass-object-size.c
===
--- test/CodeGen/pass-object-size.c
+++ test/CodeGen/pass-object-size.c
@@ -351,3 +351,18 @@
ObjectSize0(++p);
ObjectSize0(p++);
}
+
+// There was a bug where variadic functions with pass_object_size would cause
+// problems in the form of failed assertions.
+void my_sprintf(char *const c __attribute__((pass_object_size(0))), ...) {}
+
+// CHECK-LABEL: define void @test14
+void test14(char *c) {
+ // CHECK: @llvm.objectsize
+ // CHECK: call void (i8*, i64, ...) @my_sprintf
+ my_sprintf(c);
+
+ // CHECK: @llvm.objectsize
+ // CHECK: call void (i8*, i64, ...) @my_sprintf
+ my_sprintf(c, 1, 2, 3);
+}
Index: lib/CodeGen/CGVTables.cpp
===
--- lib/CodeGen/CGVTables.cpp
+++ lib/CodeGen/CGVTables.cpp
@@ -290,9 +290,8 @@
const FunctionProtoType *FPT = MD->getType()->getAs();
#ifndef NDEBUG
- const CGFunctionInfo =
-CGM.getTypes().arrangeCXXMethodCall(CallArgs, FPT,
- RequiredArgs::forPrototypePlus(FPT, 1));
+ const CGFunctionInfo = CGM.getTypes().arrangeCXXMethodCall(
+ CallArgs, FPT, RequiredArgs::forPrototypePlus(FPT, 1, MD));
assert(CallFnInfo.getRegParm() == CurFnInfo->getRegParm() &&
CallFnInfo.isNoReturn() == CurFnInfo->isNoReturn() &&
CallFnInfo.getCallingConvention() == CurFnInfo->getCallingConvention());
Index: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -54,7 +54,7 @@
}
const FunctionProtoType *FPT = MD->getType()->castAs();
- RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size());
+ RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size(), MD);
// And the rest of the call args.
if (CE) {
@@ -324,10 +324,11 @@
// Push the this ptr.
Args.add(RValue::get(ThisPtrForCall), ThisType);
- RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, 1);
-
+ RequiredArgs required =
+ RequiredArgs::forPrototypePlus(FPT, 1, /*FD=*/nullptr);
+
// And the rest of the call args
- EmitCallArgs(Args, FPT, E->arguments(), E->getDirectCallee());
+ EmitCallArgs(Args, FPT, E->arguments());
return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required),
Callee, ReturnValue, Args);
}
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -141,15 +141,16 @@
CanQual FTP,
const FunctionDecl *FD) {
SmallVector paramInfos;
- RequiredArgs required = RequiredArgs::forPrototypePlus(FTP, prefix.size());
+ RequiredArgs Required =
+ RequiredArgs::forPrototypePlus(FTP, prefix.size(), FD);
// FIXME: Kill copy.
appendParameterTypes(CGT, prefix, paramInfos, FTP, FD);
CanQualType resultType = FTP->getReturnType().getUnqualifiedType();
return CGT.arrangeLLVMFunctionInfo(resultType, instanceMethod,
/*chainCall=*/false, prefix,
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
rsmith added inline comments. Comment at: lib/CodeGen/CGExprCXX.cpp:57-58 @@ -56,2 +56,4 @@ const FunctionProtoType *FPT = MD->getType()->castAs(); - RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size()); + // Don't pass in the MethodDecl, because we should already have the + // pass_object_size values in the arglist. + RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size(), MD); Comment doesn't seem to match the code; `Args.size()` here includes only the 'this' pointer and the possible destructor VTT parameter. Delete comment? Comment at: lib/CodeGen/CGExprCXX.cpp:331 @@ -329,3 +330,3 @@ // And the rest of the call args EmitCallArgs(Args, FPT, E->arguments(), E->getDirectCallee()); return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required), Seems inconsistent to pass `E->getDirectCallee()` here but not above. Since you can't take the address of a pass_object_size function, do we need to pass the callee in either place? http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added a comment. (: ƃuıd http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added a comment. Ping :) http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added a comment. Ping :) http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added a comment. Ping :) http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added a comment. Ping :) http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv added inline comments. Comment at: lib/CodeGen/CGCall.cpp:142-143 @@ -132,1 +141,4 @@ + appendParameterTypes(CGT, prefix, FTP, FD, ); + RequiredArgs Required = + RequiredArgs::forPrototypePlus(FTP, StartParams + SynthesizedParams); CanQualType resultType = FTP->getReturnType().getUnqualifiedType(); rsmith wrote: > Would it make sense for `RequiredArgs::forPrototypePlus` to do this > computation? Are the other callers of that function also getting the wrong > number of required arguments in this case? ...There was a reason I didn't do that. I don't remember what it was, so either it disappeared, or I was just being dumb, but there was a reason. Either way, I moved this logic into `RequiredArgs`, and ended up fixing a few more bugs in the process. Yay for (fixing) bugs! http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
george.burgess.iv updated this revision to Diff 51374.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed all feedback
http://reviews.llvm.org/D17462
Files:
include/clang/CodeGen/CGFunctionInfo.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CGVTables.cpp
test/CodeGen/pass-object-size.c
test/CodeGenCXX/pass-object-size.cpp
Index: test/CodeGenCXX/pass-object-size.cpp
===
--- test/CodeGenCXX/pass-object-size.cpp
+++ test/CodeGenCXX/pass-object-size.cpp
@@ -43,3 +43,30 @@
()(nullptr);
}
}
+
+namespace variadic {
+// We had an issue where variadic member/operator calls with pass_object_size
+// would cause crashes.
+
+struct AsCtor {
+ AsCtor(const char *const c __attribute__((pass_object_size(0))), double a,
+ ...) {}
+};
+
+struct AsMember {
+ void bar(const char *const c __attribute__((pass_object_size(0))), double a,
+ ...) {}
+ void operator()(const char *const c __attribute__((pass_object_size(0))),
+ double a, ...) {}
+};
+
+// CHECK-LABEL: define void @_ZN8variadic4testEv()
+void test() {
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic6AsCtorC1EPKcU17pass_object_size0dz
+ AsCtor("a", 1.0);
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMember3barEPKcU17pass_object_size0dz
+ AsMember{}.bar("a", 1.0);
+ // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMemberclEPKcU17pass_object_size0dz
+ AsMember{}("a", 1.0);
+}
+}
Index: test/CodeGen/pass-object-size.c
===
--- test/CodeGen/pass-object-size.c
+++ test/CodeGen/pass-object-size.c
@@ -351,3 +351,18 @@
ObjectSize0(++p);
ObjectSize0(p++);
}
+
+// There was a bug where variadic functions with pass_object_size would cause
+// problems in the form of failed assertions.
+void my_sprintf(char *const c __attribute__((pass_object_size(0))), ...) {}
+
+// CHECK-LABEL: define void @test14
+void test14(char *c) {
+ // CHECK: @llvm.objectsize
+ // CHECK: call void (i8*, i64, ...) @my_sprintf
+ my_sprintf(c);
+
+ // CHECK: @llvm.objectsize
+ // CHECK: call void (i8*, i64, ...) @my_sprintf
+ my_sprintf(c, 1, 2, 3);
+}
Index: lib/CodeGen/CGVTables.cpp
===
--- lib/CodeGen/CGVTables.cpp
+++ lib/CodeGen/CGVTables.cpp
@@ -292,9 +292,8 @@
const FunctionProtoType *FPT = MD->getType()->getAs();
#ifndef NDEBUG
- const CGFunctionInfo =
-CGM.getTypes().arrangeCXXMethodCall(CallArgs, FPT,
- RequiredArgs::forPrototypePlus(FPT, 1));
+ const CGFunctionInfo = CGM.getTypes().arrangeCXXMethodCall(
+ CallArgs, FPT, RequiredArgs::forPrototypePlus(FPT, 1, MD));
assert(CallFnInfo.getRegParm() == CurFnInfo->getRegParm() &&
CallFnInfo.isNoReturn() == CurFnInfo->isNoReturn() &&
CallFnInfo.getCallingConvention() == CurFnInfo->getCallingConvention());
Index: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -54,7 +54,9 @@
}
const FunctionProtoType *FPT = MD->getType()->castAs();
- RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size());
+ // Don't pass in the MethodDecl, because we should already have the
+ // pass_object_size values in the arglist.
+ RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size(), MD);
// And the rest of the call args.
if (CE) {
@@ -323,8 +325,7 @@
// Push the this ptr.
Args.add(RValue::get(ThisPtrForCall), ThisType);
-
- RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, 1);
+ RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, 1, nullptr);
// And the rest of the call args
EmitCallArgs(Args, FPT, E->arguments(), E->getDirectCallee());
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -139,15 +139,16 @@
CanQual FTP,
const FunctionDecl *FD) {
SmallVector paramInfos;
- RequiredArgs required = RequiredArgs::forPrototypePlus(FTP, prefix.size());
+ RequiredArgs Required =
+ RequiredArgs::forPrototypePlus(FTP, prefix.size(), FD);
// FIXME: Kill copy.
appendParameterTypes(CGT, prefix, paramInfos, FTP, FD);
CanQualType resultType = FTP->getReturnType().getUnqualifiedType();
return CGT.arrangeLLVMFunctionInfo(resultType, instanceMethod,
/*chainCall=*/false, prefix,
FTP->getExtInfo(), paramInfos,
- required);
+ Required);
}
/// Arrange the argument and result information for a value of the
@@ -336,7 +337,7 @@
Re: [PATCH] D17462: Fix a codegen bug for variadic functions with pass_object_size params
rsmith added inline comments. Comment at: lib/CodeGen/CGCall.cpp:142-143 @@ -132,1 +141,4 @@ + appendParameterTypes(CGT, prefix, FTP, FD, ); + RequiredArgs Required = + RequiredArgs::forPrototypePlus(FTP, StartParams + SynthesizedParams); CanQualType resultType = FTP->getReturnType().getUnqualifiedType(); Would it make sense for `RequiredArgs::forPrototypePlus` to do this computation? Are the other callers of that function also getting the wrong number of required arguments in this case? http://reviews.llvm.org/D17462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
