[PATCH] D22057: Prevent devirtualization of calls to un-instantiated functions.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/Decl.h:1602
@@ -1601,2 +1601,3 @@
   unsigned IsConstexpr : 1;
+  unsigned IsMarkedForPendingInstantiation : 1;
 
----------------
Drop the `MarkedFor` here. This flag *is* the mark from the point of view of a user of the AST.

This flag also seems very similar to the `Used` flag on the `Decl` base class, so a documentation comment might help. Something like "Indicates that an instantiation of this function has been required but may not yet have been performed. If the function has already been instantiated, the value of this flag is unspecified."

... though it would be nicer if the value of the flag were always correct, even in the case where the function has already been instantiated.

================
Comment at: include/clang/AST/Decl.h:1877-1878
@@ -1874,1 +1876,4 @@
 
+  /// \brief Whether this function has been put on the pending instatiations
+  /// list.
+  bool isMarkedForPendingInstantiation() const {
----------------
The fact that we have a separate list is an implementation detail of `Sema`; drop that from this comment.

================
Comment at: lib/CodeGen/CGClass.cpp:2884-2886
@@ -2879,3 +2883,5 @@
     if (const ValueDecl *VD = dyn_cast<ValueDecl>(ME->getMemberDecl()))
-      return VD->getType()->isRecordType();
+      return VD->getType()->isRecordType() &&
+                  (MD->isMarkedForPendingInstantiation() ||
+                   !MD->isImplicitlyInstantiable());
 
----------------
If the function is already defined (if we instantiated it already for some reason), we should be able to devirtualize calls to it.

================
Comment at: lib/Sema/Sema.cpp:683-685
@@ -682,2 +682,5 @@
       ExternalSource->ReadPendingInstantiations(Pending);
+      for (auto PII : Pending) 
+        if (FunctionDecl *Func = dyn_cast<FunctionDecl>(PII.first))
+          Func->setMarkedForPendingInstantiation();
       PendingInstantiations.insert(PendingInstantiations.begin(),
----------------
Please add a test that we handle this properly if instantiation is triggered in a PCH.


https://reviews.llvm.org/D22057