[Sema] Make sure the definition of a referenced virtual function is emitted when it is final

[ahatanak]

Summary

The test case I added used to fail because of a linker error.

Linkage failed because Sema::MarkDeclRefReferenced would prevent the virtual method definition from being emitted by setting OdrUse=false and then code-gen would devirtualized the virtual call because its class is marked final.

This patch fixes the bug.

rdar://problem/27455779

[vsk]

This comment has been deleted.

lib/Sema/SemaExpr.cpp

14715

Do you think it makes sense to eliminate all candidate virtual methods which can be devirtualized? If so, you could make "CanDevirtualizeMemberFunctionCall" a shared utility between Sema and CodeGen, and use it here. That function should give "the truth" about whether or not a call can be devirtualized.

14717

"MarkExprReferenced" has what looks like an incomplete version of "CanDevirtualizeMemberFunctionCall". Do you think there is an opportunity to share logic there as well?

test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp

271

Have you looked into why "ptr->operator()();" compiles? We are either missing a devirtualization opportunity, or we have inconsistent logic for setting MightBeOdrUse for member calls. Either way, I think this patch is the right vehicle to address the issue.

[ahatanak]

lib/Sema/SemaExpr.cpp

14715

I moved CanDevirtualizeMemberFunctionCall to DeclCXX.cpp and added overloaded functions of canDevirtualizeCall and used one of them here. This caused changes in the generated IR in two test cases (no-devirt.cpp and vtable-available-externally.cpp). Both changes look correct to me.

test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp

271

"ptr->operator()();" creates a MemberExpr, which MarkMemberReferenced handles. The difference between MarkMemberReferenced and MarkDeclRefReferenced is that the former sets OdrUse to false when the method is pure while the latter does so when the method is virtual. Prior to r174341, MarkDeclRefReferenced was setting OdrUse to false for pure methods too (see r174242).

[ahatanak]

Address review comments.

[vsk]

I suggest changing the API for 'canDevirtualizeCall' a bit but this is looking great overall. The code movement and test case changes look good.

include/clang/AST/DeclCXX.h

1902 ↗

(On Diff #106149)

Something like CXXMethodDecl *getDevirtualizedMethod(...) would fit in better with the current API. E.g, getCorrespondingMethodInClass is done this way (and returns nullptr when a method is not found).

lib/AST/DeclCXX.cpp

1693 ↗

(On Diff #106149)

Instead of templating canDevirtualizeCall, you could do something like const_cast<CXXMethodDecl *>(this)->getDevirtualizedMethod(...).

[ahatanak]

Define and use CXXMethodDecl::getDevirtualizedMethod, which returns the function that is called when a call is devirtualized.

[vsk]

Thanks, this looks great.

lib/Sema/SemaExpr.cpp

 
 /// \brief Perform reference-marking and odr-use handling for a DeclRefExpr.
 void Sema::MarkDeclRefReferenced(DeclRefExpr *E) {
   // TODO: update this with DR# once a defect report is filed.
   // C++11 defect. The address of a pure member should not be an ODR use, even
   // if it's a qualified reference.
   bool OdrUse = true;
   if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getDecl()))
-    if (Method->isVirtual())
+    if (Method->isVirtual() && !(Method->hasAttr<FinalAttr>() ||
+                                 Method->getParent()->hasAttr<FinalAttr>()))
       OdrUse = false;
   MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
 }
 
 /// \brief Perform reference-marking and odr-use handling for a MemberExpr.
 void Sema::MarkMemberReferenced(MemberExpr *E) {
   // C++11 [basic.def.odr]p2:
   //   A non-overloaded function whose name appears as a potentially-evaluated
   //   expression or a member of a set of candidate functions, if selected by
   //   overload resolution when referred to from a potentially-evaluated

test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp

namespace Test10 {
   };
 
   // CHECK-LABEL: define i32 @_ZN6Test101fEPNS_1BE
   int f(B *b) {
     // CHECK: call i32 @_ZN6Test101B1fEv
     return static_cast<A *>(b)->f();
   }
 }
+
+namespace Test11 {
+  // Check that the definition of Derived::operator() is emitted.
+
+  // CHECK-LABEL: define linkonce_odr void @_ZN6Test111SIiE4foo1Ev(
+  // CHECK: call void @_ZN6Test111SIiE7DerivedclEv(
+  // CHECK: define linkonce_odr void @_ZN6Test111SIiE7DerivedclEv(
+  class Base {
+  public:
+    virtual void operator()() {
+    }
+  };
+
+  template<class T>
+  struct S {
+    class Derived final : public Base {
+    public:
+      void operator()() override {
+      }
+    };
+
+    Derived *ptr = nullptr;
+
+    void foo1() {
+      if (ptr) {
+        // This call gets devirtualized. If the definition of
+        // Derived::operator() is not emitted, there will be a linker error.
+        (*ptr)();
+      }
+    }
+  };
+
+  void foo2() {
+    S<int> *s = new S<int>;
+    s->foo1();
+  }
+}