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

2022-06-16 Thread Arthur Eubanks via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa70b39abffb4: [clang] Dont emit type test/assume for 
virtual classes that should never… (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

Files:
  clang/docs/LTOVisibility.rst
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp

Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -79,11 +79,11 @@
   // 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@@"
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -668,8 +668,8 @@
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;
 
   {
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1382,10 +1382,10 @@
   /// 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
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1175,7 +1175,10 @@
   DeferredVTables.clear();
 }
 
-bool CodeGenModule::HasLTOVisibilityPublicStd(const CXXRecordDecl *RD) {
+bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
+  if (RD->hasAttr() || RD->hasAttr())
+return true;
+
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
 return false;
 
@@ -1200,9 +1203,6 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (RD->hasAttr() || RD->hasAttr())
-return false;
-
   if (getTriple().isOSBinFormatCOFF()) {
 if (RD->hasAttr() || RD->hasAttr())
   return false;
@@ -1211,7 +1211,7 @@
   return false;
   }
 
-  return !HasLTOVisibilityPublicStd(RD);
+  return !AlwaysHasLTOVisibilityPublic(RD);
 }
 
 llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2695,9 +2695,9 @@
   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 =
Index: clang/docs/LTOVisibility.rst
===
--- clang/docs/LTOVisibility.rst
+++ clang/docs/LTOVisibility.rst
@@ -35,13 +35,14 @@
 (e.g.

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks updated this revision to Diff 437420.
aeubanks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

Files:
  clang/docs/LTOVisibility.rst
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp

Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -79,11 +79,11 @@
   // 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@@"
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -668,8 +668,8 @@
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;
 
   {
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1382,10 +1382,10 @@
   /// 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
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1175,7 +1175,10 @@
   DeferredVTables.clear();
 }
 
-bool CodeGenModule::HasLTOVisibilityPublicStd(const CXXRecordDecl *RD) {
+bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
+  if (RD->hasAttr() || RD->hasAttr())
+return true;
+
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
 return false;
 
@@ -1200,9 +1203,6 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (RD->hasAttr() || RD->hasAttr())
-return false;
-
   if (getTriple().isOSBinFormatCOFF()) {
 if (RD->hasAttr() || RD->hasAttr())
   return false;
@@ -1211,7 +1211,7 @@
   return false;
   }
 
-  return !HasLTOVisibilityPublicStd(RD);
+  return !AlwaysHasLTOVisibilityPublic(RD);
 }
 
 llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2695,9 +2695,9 @@
   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 =
Index: clang/docs/LTOVisibility.rst
===
--- clang/docs/LTOVisibility.rst
+++ clang/docs/LTOVisibility.rst
@@ -35,13 +35,14 @@
 (e.g. classes declared in unnamed namespaces) also receive hidden LTO
 visibility.
 
-During the LTO link, all classes with public LTO

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

2022-06-15 Thread Peter Collingbourne via Phabricator via cfe-commits 
pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Teresa Johnson via Phabricator via cfe-commits 
tejohnson added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks updated this revision to Diff 437258.
aeubanks added a comment.

update docs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

Files:
  clang/docs/LTOVisibility.rst
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp

Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -79,11 +79,11 @@
   // 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@@"
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -668,8 +668,8 @@
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;
 
   {
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1382,10 +1382,10 @@
   /// 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
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1175,7 +1175,10 @@
   DeferredVTables.clear();
 }
 
-bool CodeGenModule::HasLTOVisibilityPublicStd(const CXXRecordDecl *RD) {
+bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
+  if (RD->hasAttr() || RD->hasAttr())
+return true;
+
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
 return false;
 
@@ -1200,9 +1203,6 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (RD->hasAttr() || RD->hasAttr())
-return false;
-
   if (getTriple().isOSBinFormatCOFF()) {
 if (RD->hasAttr() || RD->hasAttr())
   return false;
@@ -1211,7 +1211,7 @@
   return false;
   }
 
-  return !HasLTOVisibilityPublicStd(RD);
+  return !AlwaysHasLTOVisibilityPublic(RD);
 }
 
 llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2695,9 +2695,9 @@
   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 =
Index: clang/docs/LTOVisibility.rst
===
--- clang/docs/LTOVisibility.rst
+++ clang/docs/LTOVisibility.rst
@@ -35,15 +35,16 @@
 (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

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

2022-06-15 Thread Peter Collingbourne via Phabricator via cfe-commits 
pcc added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Peter Collingbourne via Phabricator via cfe-commits 
pcc requested changes to this revision.
pcc added a comment.
This revision now requires changes to proceed.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks updated this revision to Diff 437224.
aeubanks added a comment.

update some comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127876/new/

https://reviews.llvm.org/D127876

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp

Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -74,16 +74,16 @@
   // MS: type.test{{.*}}!"?AUC2@@"
   c2->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C3"
-  // MS: type.test{{.*}}!"?AUC3@@"
+  // MS-NOT: type.test{{.*}}!"?AUC3@@"
   c3->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C4"
-  // MS: type.test{{.*}}!"?AUC4@@"
+  // MS-NOT: 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@@"
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -668,8 +668,8 @@
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;
 
   {
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1382,10 +1382,10 @@
   /// 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
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1175,7 +1175,15 @@
   DeferredVTables.clear();
 }
 
-bool CodeGenModule::HasLTOVisibilityPublicStd(const CXXRecordDecl *RD) {
+bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
+  if (RD->hasAttr() || RD->hasAttr())
+return true;
+
+  if (getTriple().isOSBinFormatCOFF()) {
+if (RD->hasAttr() || RD->hasAttr())
+  return true;
+  }
+
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
 return false;
 
@@ -1200,18 +1208,12 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (RD->hasAttr() || RD->hasAttr())
-return false;
-
-  if (getTriple().isOSBinFormatCOFF()) {
-if (RD->hasAttr() || RD->hasAttr())
-  return false;
-  } else {
+  if (!getTriple().isOSBinFormatCOFF()) {
 if (LV.getVisibility() != HiddenVisibility)
   return false;
   }
 
-  return !HasLTOVisibilityPublicStd(RD);
+  return !AlwaysHasLTOVisibilityPublic(RD);
 }
 
 llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2695,9 +2695,9 @@
   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

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

2022-06-15 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks created this revision.
aeubanks added reviewers: pcc, tejohnson.
Herald added subscribers: ormris, steven_wu, hiraditya.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127876

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp

Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -74,16 +74,16 @@
   // MS: type.test{{.*}}!"?AUC2@@"
   c2->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C3"
-  // MS: type.test{{.*}}!"?AUC3@@"
+  // MS-NOT: type.test{{.*}}!"?AUC3@@"
   c3->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C4"
-  // MS: type.test{{.*}}!"?AUC4@@"
+  // MS-NOT: 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@@"
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -669,7 +669,7 @@
   bool ShouldEmitWPDInfo =
   CGM.getCodeGenOpts().WholeProgramVTables &&
   // Don't insert type tests if we are forcing public std visibility.
-  !CGM.HasLTOVisibilityPublicStd(RD);
+  !CGM.AlwaysHasLTOVisibilityPublic(RD);
   llvm::Value *VirtualFn = nullptr;
 
   {
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1385,7 +1385,7 @@
   /// 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);
+  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
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1175,7 +1175,15 @@
   DeferredVTables.clear();
 }
 
-bool CodeGenModule::HasLTOVisibilityPublicStd(const CXXRecordDecl *RD) {
+bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
+  if (RD->hasAttr() || RD->hasAttr())
+return true;
+
+  if (getTriple().isOSBinFormatCOFF()) {
+if (RD->hasAttr() || RD->hasAttr())
+  return true;
+  }
+
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
 return false;
 
@@ -1200,18 +1208,12 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (RD->hasAttr() || RD->hasAttr())
-return false;
-
-  if (getTriple().isOSBinFormatCOFF()) {
-if (RD->hasAttr() || RD->hasAttr())
-  return false;
-  } else {
+  if (!getTriple().isOSBinFormatCOFF()) {
 if (LV.getVisibility() != HiddenVisibility)
   return false;
   }
 
-  return !HasLTOVisibilityPublicStd(RD);
+  return !AlwaysHasLTOVisibilityPublic(RD);
 }
 
 llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2697,7 +2697,7 @@
   else if (CGM.getCodeGenOpts().WholeProgramVTables &&
// Don't insert type test assumes if we are forcing public std
// visibility.
-   !CGM.HasLTOVisibilityPublicStd(RD)) {
+   !CGM.AlwaysHasLTOVisibilityPublic(RD)) {
 llvm::Metadata *MD =
 CGM.CreateMetadataIdentifierForType(QualType(RD->getTypeForDecl(), 0));
 llvm::Value *TypeId =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits