[clang codegen][regression] Add dso_local/hidden/etc. markings to VTT definitions and declarations

[bd1976bris]

https://reviews.llvm.org/D128482 regressed certain cases of VTT emission which are no longer hidden with -fvisibility=hidden.

Fix this regression by marking both declarations and definitions.

Fixes [clang codegen][regression] VTT definitions missing dso_local/hidden/etc markings #72451

[llvmbot]

@llvm/pr-subscribers-clang

Author: bd1976bris (bd1976bris)

Changes

https://reviews.llvm.org/D128482 regressed certain cases of VTT emission which are no longer hidden with -fvisibility=hidden.

Fix this regression by marking both declarations and definitions.

Fixes [clang codegen][regression] VTT definitions missing dso_local/hidden/etc markings #72451

---

Full diff: https://github.com/llvm/llvm-project/pull/72452.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGVTT.cpp (+3)
• (modified) clang/test/CodeGenCXX/visibility.cpp (+3)

diff --git a/clang/lib/CodeGen/CGVTT.cpp b/clang/lib/CodeGen/CGVTT.cpp
index 22790147c6f5a9f..0eab677ca68a85a 100644
--- a/clang/lib/CodeGen/CGVTT.cpp
+++ b/clang/lib/CodeGen/CGVTT.cpp
@@ -93,6 +93,9 @@ CodeGenVTables::EmitVTTDefinition(llvm::GlobalVariable *VTT,
 
   if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
     VTT->setComdat(CGM.getModule().getOrInsertComdat(VTT->getName()));
+
+  // Set the right visibility.
+  CGM.setGVProperties(VTT, RD);
 }
 
 llvm::GlobalVariable *CodeGenVTables::GetAddrOfVTT(const CXXRecordDecl *RD) {
diff --git a/clang/test/CodeGenCXX/visibility.cpp b/clang/test/CodeGenCXX/visibility.cpp
index b017bc8b52d5a5d..d94303c90f300b4 100644
--- a/clang/test/CodeGenCXX/visibility.cpp
+++ b/clang/test/CodeGenCXX/visibility.cpp
@@ -206,6 +206,9 @@ namespace test27 {
 // CHECK: @_ZGVZN6test681fC1EvE4test = linkonce_odr global
 // CHECK-HIDDEN: @_ZGVZN6test681fC1EvE4test = linkonce_odr hidden global
 
+// CHECK-HIDDEN: @_ZTVN6test701DE = linkonce_odr hidden unnamed_addr constant { [3 x ptr] } { [3 x ptr] [ptr null, ptr null, ptr @_ZTIN6test701DE] }, align 8
+// CHECK-HIDDEN: @_ZTTN6test701DE = linkonce_odr hidden unnamed_addr constant [1 x ptr] [ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6test701DE, i32 0, inrange i32 0, i32 3)], align 8
+
 // CHECK: @_ZZN6Test193fooIiEEvvE1a = linkonce_odr global
 // CHECK-HIDDEN: @_ZZN6Test193fooIiEEvvE1a = linkonce_odr hidden global
 

[llvmbot]

@llvm/pr-subscribers-clang-codegen

Author: bd1976bris (bd1976bris)

Changes

https://reviews.llvm.org/D128482 regressed certain cases of VTT emission which are no longer hidden with -fvisibility=hidden.

Fix this regression by marking both declarations and definitions.

Fixes [clang codegen][regression] VTT definitions missing dso_local/hidden/etc markings #72451

---

Full diff: https://github.com/llvm/llvm-project/pull/72452.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGVTT.cpp (+3)
• (modified) clang/test/CodeGenCXX/visibility.cpp (+3)

diff --git a/clang/lib/CodeGen/CGVTT.cpp b/clang/lib/CodeGen/CGVTT.cpp
index 22790147c6f5a9f..0eab677ca68a85a 100644
--- a/clang/lib/CodeGen/CGVTT.cpp
+++ b/clang/lib/CodeGen/CGVTT.cpp
@@ -93,6 +93,9 @@ CodeGenVTables::EmitVTTDefinition(llvm::GlobalVariable *VTT,
 
   if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
     VTT->setComdat(CGM.getModule().getOrInsertComdat(VTT->getName()));
+
+  // Set the right visibility.
+  CGM.setGVProperties(VTT, RD);
 }
 
 llvm::GlobalVariable *CodeGenVTables::GetAddrOfVTT(const CXXRecordDecl *RD) {
diff --git a/clang/test/CodeGenCXX/visibility.cpp b/clang/test/CodeGenCXX/visibility.cpp
index b017bc8b52d5a5d..d94303c90f300b4 100644
--- a/clang/test/CodeGenCXX/visibility.cpp
+++ b/clang/test/CodeGenCXX/visibility.cpp
@@ -206,6 +206,9 @@ namespace test27 {
 // CHECK: @_ZGVZN6test681fC1EvE4test = linkonce_odr global
 // CHECK-HIDDEN: @_ZGVZN6test681fC1EvE4test = linkonce_odr hidden global
 
+// CHECK-HIDDEN: @_ZTVN6test701DE = linkonce_odr hidden unnamed_addr constant { [3 x ptr] } { [3 x ptr] [ptr null, ptr null, ptr @_ZTIN6test701DE] }, align 8
+// CHECK-HIDDEN: @_ZTTN6test701DE = linkonce_odr hidden unnamed_addr constant [1 x ptr] [ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6test701DE, i32 0, inrange i32 0, i32 3)], align 8
+
 // CHECK: @_ZZN6Test193fooIiEEvvE1a = linkonce_odr global
 // CHECK-HIDDEN: @_ZZN6Test193fooIiEEvvE1a = linkonce_odr hidden global
 

[efriedma-quic]

I'd prefer not to call setGVProperties() twice on the same variable; under what conditions is the first call not sufficient?

[bd1976bris]

I'd prefer not to call setGVProperties() twice on the same variable; under what conditions is the first call not sufficient?

The test-case I posted is one such scenario: https://godbolt.org/z/s4rPffsrK. I suspect it may be related to whether there is a key function present. I'm not an expert in this area of the code so I'll have to investigate further.

[efriedma-quic]

I mean, looking at the code, as far as I can tell, we always call GetAddrOfVTT immediately before EmitVTTDefinition, so setGVProperties() has already run, so running it again shouldn't do anything... unless somehow EmitVTTDefinition is overwriting the visibility, or setGVProperties() behaves differently for declarations vs. definitions. If something like that is happening, I'd like to explicitly document it.

[bd1976bris]

I mean, looking at the code, as far as I can tell, we always call GetAddrOfVTT immediately before EmitVTTDefinition, so setGVProperties() has already run, so running it again shouldn't do anything... unless somehow EmitVTTDefinition is overwriting the visibility, or setGVProperties() behaves differently for declarations vs. definitions. If something like that is happening, I'd like to explicitly document it.

Thanks for clarifying. Luckily a full understanding of the VTT emission isn't isn't needed to answer your question :)

When the visibility is applied via visibility attributes there is no difference for a definition or a declaration. However, for the visibility from the cli option -fvisibility=<visibility>, the visibility is applied only to definitions and not to declarations. My understanding is that this is by design - so that system headers do not have to be marked with explicit visibility annotations.

You can see this in the code in setGlobalVisibility():

  if (LV.isVisibilityExplicit() || getLangOpts().SetVisibilityForExternDecls ||
      !GV->isDeclarationForLinker())
    GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));

[efriedma-quic]

Okay. Please add a comment to the code briefly explaining that.

Otherwise looks fine.

[bd1976bris]

Okay. Please add a comment to the code briefly explaining that.

Done.

[efriedma-quic]

LGTM

[justincady]

LGTM as well, as the original fix remains in place.

[bd1976bris]

Thanks for the reviews 👍