Sunil_Srivastava created this revision.
Sunil_Srivastava added a reviewer: rsmith.
Sunil_Srivastava added a subscriber: cfe-commits.
This review is for a fix for PR 27895, but it requires some discussion. The
bugzilla and the email exchange with Richard Smith in
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160530/161107.html
contain more details.
The basic problem: When the front end sees a call to a virtual function of a
template class, it treats that as an odr use, therefore requiring the
instantiation; except, for some reason, when it is an operator and used with an
operator syntax. In those cases, if the call gets devirtualized later, it will
generate a direct call to a function that has not been instantiated. Most of
the time it will still work because someone else will instantiate it for some
other reason, but if not, then we have converted a potentially dead code to a
link time error of a missing definition.
As discussed in the email exchange, there are two alternative ways to fix the
problem. Either will be sufficient, but we want to do both.
The first fix will be in the front end to force the instantiation on virtual
calls that are potentially devirtualizable. For reasons described below I plan
on submitting this in a later checkin.
The second fix, the matter of this review, is to avoid devirtualization under
certain conditions. The conditions that I am choosing is to prevent
devirtualization if the function could be implicitly instantiated but front end
chose to not mark it for instantiation. I welcome any suggestions about
improvement to this condition.
The reason I am planning on submitting them in this order, is that, once the
first fix is checked in, there will be no way, known to me, to write a test for
the second, devirtualization fix.
Now some discussion about the way I am testing the avoid-devirtualization
condition: I have added a bit to FunctionDecl that I am setting whenever a
function is put on the PendingInstantiations list. Then, at the time of
devirtualization, if the target function isImplictlyInstantiable and does not
have that bit set, I avoid the devirtualization. This change adds one bit to
FunctionDecl, but the test is quite fast. However, it duplicates the
information between this bit and the PendingInstantiationsList.
We have considered an alternate implementation which replaces the call (in
CGClass.cpp) to
isMarkedForPendingInstantiation with
- explcitly-test-whether-it-is-on-the-PendingInstations-list ||
- has-already-been-instantiated.
This alternate implementation does not add anything to FunctionDecl, avoids
information duplication, but the test for its presence in the list will be
expensive; it will require a search in the list. It will be done only for
devirtualizable calls though, which is not that common.
A related point that I am not sure about is: Are there circumstances where a
function on the PendingInstantiations list can fails to be instantiated for
reasons other than errors? If so then this checkin is not perfect, though it is
still better than nothing.
I am open to suggestion as to which of these implementations makes more sense.
Once this change has been approved and submitted, I will open another review
for the first fix, which will force the instantiation of functions that are
target of potentially devirtualizable calls.
http://reviews.llvm.org/D22057
Files:
include/clang/AST/Decl.h
lib/CodeGen/CGClass.cpp
lib/Sema/Sema.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/no-devirt.cpp
Index: test/CodeGen/no-devirt.cpp
===================================================================
--- test/CodeGen/no-devirt.cpp
+++ test/CodeGen/no-devirt.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %s
+template < typename T, int N = 0 > class TmplWithArray {
+public:
+ virtual T& operator [] (int idx);
+ virtual T& func1 (int idx);
+ virtual T& func2 (int idx);
+ T ar[N+1];
+};
+class Wrapper {
+ TmplWithArray<bool, 10> data;
+ bool indexIt(int a);
+};
+bool Wrapper::indexIt(int a)
+{
+ if (a > 6) return data[a] ; // Should not devirtualize
+ if (a > 4) return data.func1(a); // Should devirtualize
+ return data.func2(a); // Should devirtualize
+}
+template <typename T, int N> T& TmplWithArray<T, N >::operator[](int idx) {
+ return ar[idx];
+}
+template <typename T, int N> T& TmplWithArray<T, N >::func1(int idx) {
+ return ar[idx];
+}
+// CHECK-NOT: call {{.*}} @_ZN13TmplWithArrayIbLi10EEixEi
+// CHECK-DAG: call {{.*}} @_ZN13TmplWithArrayIbLi10EE5func1Ei
+// CHECK-DAG: call {{.*}} @_ZN13TmplWithArrayIbLi10EE5func2Ei
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3531,6 +3531,7 @@
// Postpone late parsed template instantiations.
if (PatternDecl->isLateTemplateParsed() &&
!LateTemplateParser) {
+ Function->setMarkedForPendingInstantiation();
PendingInstantiations.push_back(
std::make_pair(Function, PointOfInstantiation));
return;
@@ -3582,6 +3583,7 @@
} else if (Function->getTemplateSpecializationKind()
== TSK_ExplicitInstantiationDefinition) {
assert(!Recursive);
+ Function->setMarkedForPendingInstantiation();
PendingInstantiations.push_back(
std::make_pair(Function, PointOfInstantiation));
} else if (Function->getTemplateSpecializationKind()
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -13082,6 +13082,7 @@
// call to such a function.
InstantiateFunctionDefinition(PointOfInstantiation, Func);
else {
+ Func->setMarkedForPendingInstantiation();
PendingInstantiations.push_back(std::make_pair(Func,
PointOfInstantiation));
// Notify the consumer that a function was implicitly instantiated.
Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -680,6 +680,9 @@
// Load pending instantiations from the external source.
SmallVector<PendingImplicitInstantiation, 4> Pending;
ExternalSource->ReadPendingInstantiations(Pending);
+ for (auto PII : Pending)
+ if (FunctionDecl *Func = dyn_cast<FunctionDecl>(PII.first))
+ Func->setMarkedForPendingInstantiation();
PendingInstantiations.insert(PendingInstantiations.begin(),
Pending.begin(), Pending.end());
}
Index: lib/CodeGen/CGClass.cpp
===================================================================
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -2874,10 +2874,16 @@
// We can devirtualize calls on an object accessed by a class member access
// expression, since by C++11 [basic.life]p6 we know that it can't refer to
- // a derived class object constructed in the same location.
+ // a derived class object constructed in the same location. However, we avoid
+ // devirtualizing a call to a function that can be instantiated implicitly,
+ // unless we know that we will be instantiating it in this TU. If this
+ // function does not get instantiated, the devirtualization will create a
+ // direct call to a function whose body may not exist.
if (const MemberExpr *ME = dyn_cast<MemberExpr>(Base))
if (const ValueDecl *VD = dyn_cast<ValueDecl>(ME->getMemberDecl()))
- return VD->getType()->isRecordType();
+ return VD->getType()->isRecordType() &&
+ (MD->isMarkedForPendingInstantiation() ||
+ !MD->isImplicitlyInstantiable());
// We can always devirtualize calls on temporary object expressions.
if (isa<CXXConstructExpr>(Base))
Index: include/clang/AST/Decl.h
===================================================================
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -1599,6 +1599,7 @@
unsigned HasImplicitReturnZero : 1;
unsigned IsLateTemplateParsed : 1;
unsigned IsConstexpr : 1;
+ unsigned IsMarkedForPendingInstantiation : 1;
/// \brief Indicates if the function uses __try.
unsigned UsesSEHTry : 1;
@@ -1691,7 +1692,8 @@
HasWrittenPrototype(true), IsDeleted(false), IsTrivial(false),
IsDefaulted(false), IsExplicitlyDefaulted(false),
HasImplicitReturnZero(false), IsLateTemplateParsed(false),
- IsConstexpr(isConstexprSpecified), UsesSEHTry(false),
+ IsConstexpr(isConstexprSpecified),
+ IsMarkedForPendingInstantiation(false), UsesSEHTry(false),
HasSkippedBody(false), EndRangeLoc(NameInfo.getEndLoc()),
TemplateOrSpecialization(),
DNLoc(NameInfo.getInfo()) {}
@@ -1872,6 +1874,15 @@
bool isConstexpr() const { return IsConstexpr; }
void setConstexpr(bool IC) { IsConstexpr = IC; }
+ /// \brief Whether this function has been put on the pending instatiations
+ /// list.
+ bool isMarkedForPendingInstantiation() const {
+ return IsMarkedForPendingInstantiation;
+ }
+ void setMarkedForPendingInstantiation(bool IM = true) {
+ IsMarkedForPendingInstantiation = IM;
+ }
+
/// \brief Indicates the function uses __try.
bool usesSEHTry() const { return UsesSEHTry; }
void setUsesSEHTry(bool UST) { UsesSEHTry = UST; }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
