[PATCH] D12865: Generating available_externally vtables and assume loads bugfix

[Richard Smith via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM


================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:399-400
@@ +398,4 @@
+          return true;
+      }
+      else if (VtableComponent.isUsedFunctionPointerKind()) {
+        const CXXMethodDecl *Method = VtableComponent.getFunctionDecl();
----------------
No newline before `else`.

================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:1632
@@ -1611,3 +1631,3 @@
 
 bool ItaniumCXXABI::canSpeculativelyEmitVTable(const CXXRecordDecl *RD) const {
   // We don't emit available_externally vtables if we are in -fapple-kext mode
----------------
This function is now serving two purposes:

1) Can we generate a new reference to the vtable that was not implied by the source code and checked by `Sema`, and
2) Can we generate a speculative definition of the vtable, despite it (perhaps) not being used.

I'm OK with the current approach as a conservative fix for the issue we're seeing with visibility, but we should aim to separate out these two questions eventually. For the former, we don't need to check the vtable contents, just the visibility of the vtable symbol itself.

================
Comment at: test/CodeGenCXX/vtable-assume-load.cpp:186
@@ -185,2 +185,3 @@
 };
-// Because A's vtable is external, it's safe to generate assumption loads.
+// FIXME: Because A's vtable is external, and all functions aint hidden,
+// it's safe to generate assumption loads.
----------------
"and all functions aint hidden," -> "and no virtual functions are hidden,"


http://reviews.llvm.org/D12865