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

[Sunil Srivastava via cfe-commits]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D22057.62907.patch
Type: text/x-patch
Size: 5952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160706/b65ce35e/attachment-0001.bin>