[clang] Don't emit type test/assume for virtual classes that should never participate in WPD

[aeubanks]

update some comments

[aeubanks]

I realize that this is not a full fix for the general problem involving -lto-whole-program-visibility, but I believe if you ignore -lto-whole-program-visibility (which Chrome doesn't use) then this fix makes sense.

[pcc]

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

[aeubanks]

In D127876#3586134, @pcc wrote:

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

perhaps I'm misunderstanding, but even with -lto-whole-program-visibility classes explicitly marked with public LTO visibility still shouldn't participate in WPD?

[pcc]

In D127876#3586154, @aeubanks wrote:

In D127876#3586134, @pcc wrote:

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

perhaps I'm misunderstanding, but even with -lto-whole-program-visibility classes explicitly marked with public LTO visibility still shouldn't participate in WPD?

We documented the feature in D75655 and there it says "all classes" (and still does).

[aeubanks]

update docs

[aeubanks]

We documented the feature in D75655 and there it says "all classes" (and still does).

I've updated the documentation. It was already slightly inaccurate in that we weren't emitting type test/assumes for std/stdext namespace classes when -flto-visibility-public-std, this now corrects that part and generalizes that part of the documentation.

[tejohnson]

I prefer the way proposed here, which was what I initially intended to do in fact. @pcc, I recall from our early internal conversations about my proposal that you felt the new mechanism should apply to all classes, so that's why the final design did that. I do tend to think that if the source code has specifically annotated the classes with public LTO visibility it is better to enforce that always. I think revisiting this decision is worthwhile.

[pcc]

Okay, it seems reasonable enough to have the [[clang::lto_visibility_public]] attribute override the --lto-whole-program-visibility flag.

What I'm not sure about though is whether __declspec(uuid) and std/stdext in /MT//MTd should also override it.

Fortunately we don't need to solve that now because those are Windows-specific attributes/features and we don't support passing --lto-whole-program-visibility to Windows linkers.

So LGTM if you only mention [[clang::lto_visibility_public]] in your change to the documentation.

[aeubanks]

update docs to only mention [[clang::lto_visibility_public]]

clang/docs/LTOVisibility.rst

 flag) or, on the Windows platform, the dllimport and dllexport attributes. When
 targeting non-Windows platforms, classes with a visibility other than hidden
 visibility receive public LTO visibility. When targeting Windows, classes
 with dllimport or dllexport attributes receive public LTO visibility. All
 other classes receive hidden LTO visibility. Classes with internal linkage
 (e.g. classes declared in unnamed namespaces) also receive hidden LTO
 visibility.
 
-During the LTO link, all classes with public LTO visibility will be refined
-to hidden LTO visibility when the ``--lto-whole-program-visibility`` lld linker
-option is applied (``-plugin-opt=whole-program-visibility`` for gold). This flag
-can be used to defer specifying whether classes have hidden LTO visibility until
-link time, to allow bitcode objects to be shared by different LTO links.
-Due to an implementation limitation, symbols associated with classes with hidden
-LTO visibility may still be exported from the binary when using this flag. It is
+During the LTO link, all classes with public LTO visibility but not marked with
+``[[clang::lto_visibility_public]]`` (see below) will be refined to hidden LTO
+visibility when the ``--lto-whole-program-visibility`` lld linker option is
+applied (``-plugin-opt=whole-program-visibility`` for gold).  This flag can be
+used to defer specifying whether classes have hidden LTO visibility until link
+time, to allow bitcode objects to be shared by different LTO links.  Due to an
+implementation limitation, symbols associated with classes with hidden LTO
+visibility may still be exported from the binary when using this flag. It is
 unsafe to refer to these symbols, and their visibility may be relaxed to hidden
 in a future compiler release.
 
 A class defined in a translation unit built without LTO receives public
 LTO visibility regardless of its object file visibility, linkage or other
 attributes.
 
 This mechanism will produce the correct result in most cases, but there are

clang/lib/CodeGen/CGClass.cpp

 }
 
 void CodeGenFunction::EmitTypeMetadataCodeForVCall(const CXXRecordDecl *RD,
                                                    llvm::Value *VTable,
                                                    SourceLocation Loc) {
   if (SanOpts.has(SanitizerKind::CFIVCall))
     EmitVTablePtrCheckForCall(RD, VTable, CodeGenFunction::CFITCK_VCall, Loc);
   else if (CGM.getCodeGenOpts().WholeProgramVTables &&
-           // Don't insert type test assumes if we are forcing public std
+           // Don't insert type test assumes if we are forcing public
            // visibility.
-           !CGM.HasLTOVisibilityPublicStd(RD)) {
+           !CGM.AlwaysHasLTOVisibilityPublic(RD)) {
     llvm::Metadata *MD =
         CGM.CreateMetadataIdentifierForType(QualType(RD->getTypeForDecl(), 0));
     llvm::Value *TypeId =
         llvm::MetadataAsValue::get(CGM.getLLVMContext(), MD);
 
     llvm::Value *CastedVTable = Builder.CreateBitCast(VTable, Int8PtrTy);
     llvm::Value *TypeTest =
         Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::type_test),

clang/lib/CodeGen/CGVTables.cpp

for (const CXXRecordDecl *RD : DeferredVTables)
     else if (shouldOpportunisticallyEmitVTables())
       OpportunisticVTables.push_back(RD);
 
   assert(savedSize == DeferredVTables.size() &&
          "deferred extra vtables during vtable emission?");
   DeferredVTables.clear();
 }
 
-bool CodeGenModule::HasLTOVisibilityPublicStd(const CXXRecordDecl *RD) {
+bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
+  if (RD->hasAttr<LTOVisibilityPublicAttr>() || RD->hasAttr<UuidAttr>())
+    return true;
+
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
     return false;
 
   const DeclContext *DC = RD;
   while (true) {
     auto *D = cast<Decl>(DC);
     DC = DC->getParent();
     if (isa<TranslationUnitDecl>(DC->getRedeclContext())) {
       if (auto *ND = dyn_cast<NamespaceDecl>(D))
         if (const IdentifierInfo *II = ND->getIdentifier())
           if (II->isStr("std") || II->isStr("stdext"))
             return true;
       break;
     }
   }
 
   return false;
 }
 
 bool CodeGenModule::HasHiddenLTOVisibility(const CXXRecordDecl *RD) {
   LinkageInfo LV = RD->getLinkageAndVisibility();
   if (!isExternallyVisible(LV.getLinkage()))
     return true;
 
-  if (RD->hasAttr<LTOVisibilityPublicAttr>() || RD->hasAttr<UuidAttr>())
-    return false;
-
   if (getTriple().isOSBinFormatCOFF()) {
     if (RD->hasAttr<DLLExportAttr>() || RD->hasAttr<DLLImportAttr>())
       return false;
   } else {
     if (LV.getVisibility() != HiddenVisibility)
       return false;
   }
 
-  return !HasLTOVisibilityPublicStd(RD);
+  return !AlwaysHasLTOVisibilityPublic(RD);
 }
 
 llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
     const CXXRecordDecl *RD, llvm::DenseSet<const CXXRecordDecl *> &Visited) {
   // If we have already visited this RD (which means this is a recursive call
   // since the initial call should have an empty Visited set), return the max
   // visibility. The recursive calls below compute the min between the result
   // of the recursive call and the current TypeVis, so returning the max here

clang/lib/CodeGen/CodeGenModule.h

public:
   /// Return the alignment specified in an allocate directive, if present.
   llvm::Optional<CharUnits> getOMPAllocateAlignment(const VarDecl *VD);
 
   /// Returns whether the given record has hidden LTO visibility and therefore
   /// may participate in (single-module) CFI and whole-program vtable
   /// optimization.
   bool HasHiddenLTOVisibility(const CXXRecordDecl *RD);
 
-  /// Returns whether the given record has public std LTO visibility
-  /// and therefore may not participate in (single-module) CFI and whole-program
-  /// vtable optimization.
-  bool HasLTOVisibilityPublicStd(const CXXRecordDecl *RD);
+  /// Returns whether the given record has public LTO visibility (regardless of
+  /// -lto-whole-program-visibility) and therefore may not participate in
+  /// (single-module) CFI and whole-program vtable optimization.
+  bool AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD);
 
   /// Returns the vcall visibility of the given type. This is the scope in which
   /// a virtual function call could be made which ends up being dispatched to a
   /// member function of this class. This scope can be wider than the visibility
   /// of the class itself when the class has a more-visible dynamic base class.
   /// The client should pass in an empty Visited set, which is used to prevent
   /// redundant recursive processing.
   llvm::GlobalObject::VCallVisibility

clang/lib/CodeGen/ItaniumCXXABI.cpp

CGCallee ItaniumCXXABI::EmitLoadOfMemberFunctionPointer(
   llvm::Constant *CheckSourceLocation;
   llvm::Constant *CheckTypeDesc;
   bool ShouldEmitCFICheck = CGF.SanOpts.has(SanitizerKind::CFIMFCall) &&
                             CGM.HasHiddenLTOVisibility(RD);
   bool ShouldEmitVFEInfo = CGM.getCodeGenOpts().VirtualFunctionElimination &&
                            CGM.HasHiddenLTOVisibility(RD);
   bool ShouldEmitWPDInfo =
       CGM.getCodeGenOpts().WholeProgramVTables &&
-      // Don't insert type tests if we are forcing public std visibility.
-      !CGM.HasLTOVisibilityPublicStd(RD);
+      // Don't insert type tests if we are forcing public visibility.
+      !CGM.AlwaysHasLTOVisibilityPublic(RD);
   llvm::Value *VirtualFn = nullptr;
 
   {
     CodeGenFunction::SanitizerScope SanScope(&CGF);
     llvm::Value *TypeId = nullptr;
     llvm::Value *CheckResult = nullptr;
 
     if (ShouldEmitCFICheck || ShouldEmitVFEInfo || ShouldEmitWPDInfo) {

clang/test/CodeGenCXX/lto-visibility-inference.cpp

void f(C1 *c1, C2 *c2, C3 *c3, C4 *c4, C5 *c5, C6 *c6, std::C7 *c7,
   // MS: type.test{{.*}}!"?AUC2@@"
   c2->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C3"
   // MS: type.test{{.*}}!"?AUC3@@"
   c3->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C4"
   // MS: type.test{{.*}}!"?AUC4@@"
   c4->f();
-  // ITANIUM: type.test{{.*}}!"_ZTS2C5"
-  // MS: type.test{{.*}}!"?AUC5@@"
+  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C5"
+  // MS-NOT: type.test{{.*}}!"?AUC5@@"
   c5->f();
-  // ITANIUM: type.test{{.*}}!"_ZTS2C6"
-  // MS: type.test{{.*}}!"?AUC6@@"
+  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C6"
+  // MS-NOT: type.test{{.*}}!"?AUC6@@"
   c6->f();
   // ITANIUM: type.test{{.*}}!"_ZTSSt2C7"
   // MS-STD: type.test{{.*}}!"?AUC7@std@@"
   // MS-NOSTD-NOT: type.test{{.*}}!"?AUC7@std@@"
   c7->f();
   // ITANIUM: type.test{{.*}}!"_ZTSNSt2C72C8E"
   // MS-STD: type.test{{.*}}!"?AUC8@C7@std@@"
   // MS-NOSTD-NOT: type.test{{.*}}!"?AUC8@C7@std@@"