[Clang][-fvisibility-from-dllstorageclass] Set DSO Locality from final visibility

[bd1976llvm]

Summary

Ensure that the DSO Locality of the globals in the IR is derived from their final visibility when using -fvisibility-from-dllstorageclass.

[compnerd]

Im not sure if renaming the check prefix is actually helpful, but sure. This does seem like somewhat unfortunate. For the dllexport case, you really want dso_local. define dso_local global and declare dso_local global are preferable in this case as that guarantees a local definition, which is always the case for an exported symbol. You definitely do not want declare extern dso_local global. A few of the changes here are actually changing the IRGen from the desired state, is that intentional?

[bd1976llvm]

In D91779#2405841, @compnerd wrote:

Im not sure if renaming the check prefix is actually helpful, but sure. This does seem like somewhat unfortunate. For the dllexport case, you really want dso_local. define dso_local global and declare dso_local global are preferable in this case as that guarantees a local definition, which is always the case for an exported symbol. You definitely do not want declare extern dso_local global. A few of the changes here are actually changing the IRGen from the desired state, is that intentional?

Thanks for looking at this. Yes, overriding the IRGen pessimistically was my intention. -fvisibility-from-dllstorageclass is supposed to override the effects of visibility options and annotations - and those can have an effect on the DSO locality. I had also thought that anyone deploying this option is probably in a similar circumstance to us where they have a file format that deviates somewhat from the expected rules. Therefore, setting DSO locality pessimistically and fixing things up in the linker makes sense. Having said that I can also see an argument for leaving the DSO locality as it is. Do you think it would be better to put this behaviour on a switch? It can be off by default but the PS4 driver can set it on?

[compnerd]

I'm not convinced that making this an option is valuable at this time. Right now, I don't think that this functionality is broadly adopted, and, I suppose that if your linker is clever enough to perform optimizations thoroughly enough, it wouldn't matter too much (modulo link time explosion). I would like to see this more accurately explained in the documentation though - CodeGenModule.cpp L423-L424 would be a good site for that. Beyond that, LGTM.

[bd1976llvm]

Improved comment.

[bd1976llvm]

Fixed spelling errors.

[rnk]

Looks good to me

clang/lib/CodeGen/CodeGenModule.cpp

static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
   llvm::GlobalValue::VisibilityTypes ExternDeclNoDLLStorageClassVisibility =
       CodeGenModule::GetLLVMVisibility(
           LO.getExternDeclNoDLLStorageClassVisibility());
 
   for (llvm::GlobalValue &GV : M.global_values()) {
     if (GV.hasAppendingLinkage() || GV.hasLocalLinkage())
       continue;
 
+    // Reset DSO locality before setting the visibilty. This removes
+    // any effects that visibility options and annotations may have
+    // hasd on the DSO locality. Setting the visibility will implicitly set
+    // appropriate globals to DSO Local; however, this will be pessimistic
+    // w.r.t. to the normal compiler IRGen.
+    GV.setDSOLocal(false);
+
     if (GV.isDeclarationForLinker()) {
       GV.setVisibility(GV.getDLLStorageClass() ==
                                llvm::GlobalValue::DLLImportStorageClass
                            ? ExternDeclDLLImportVisibility
                            : ExternDeclNoDLLStorageClassVisibility);
     } else {
       GV.setVisibility(GV.getDLLStorageClass() ==
                                llvm::GlobalValue::DLLExportStorageClass

clang/test/CodeGenCXX/visibility-dllstorageclass.cpp

 // REQUIRES: x86-registered-target
 
 // Test that -fvisibility-from-dllstorageclass maps DLL storage class to visibility
 // and that it overrides the effect of visibility options and annotations.
 
 // RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -fdeclspec \
 // RUN:     -fvisibility hidden \
+// RUN:     -fapply-global-visibility-to-externs \
 // RUN:     -fvisibility-from-dllstorageclass \
 // RUN:     -x c++ %s -S -emit-llvm -o - | \
-// RUN:   FileCheck %s --check-prefixes=DEFAULT
+// RUN:   FileCheck %s --check-prefixes=DEFAULTS
 
 // RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -fdeclspec \
 // RUN:     -fvisibility hidden \
+// RUN:     -fapply-global-visibility-to-externs \
 // RUN:     -fvisibility-from-dllstorageclass \
 // RUN:     -fvisibility-dllexport=hidden \
 // RUN:     -fvisibility-nodllstorageclass=protected \
 // RUN:     -fvisibility-externs-dllimport=hidden \
 // RUN:     -fvisibility-externs-nodllstorageclass=protected \
 // RUN:     -x c++  %s -S -emit-llvm -o - | \
 // RUN:   FileCheck %s --check-prefixes=EXPLICIT
 
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -fdeclspec \
+// RUN:     -fvisibility hidden \
+// RUN:     -fapply-global-visibility-to-externs \
+// RUN:     -fvisibility-from-dllstorageclass \
+// RUN:     -fvisibility-dllexport=default \
+// RUN:     -fvisibility-nodllstorageclass=default \
+// RUN:     -fvisibility-externs-dllimport=default \
+// RUN:     -fvisibility-externs-nodllstorageclass=default \
+// RUN:     -x c++  %s -S -emit-llvm -o - | \
+// RUN:   FileCheck %s --check-prefixes=ALL_DEFAULT
+
 // Local
 static void l() {}
 void use_locals(){l();}
-// DEFAULT-DAG: define internal void @_ZL1lv()
+// DEFAULTS-DAG: define internal void @_ZL1lv()
 // EXPLICIT-DAG: define internal void @_ZL1lv()
+// ALL_DEFAULT-DAG: define internal void @_ZL1lv()
 
 // Function
 void f() {}
 void __declspec(dllexport) exported_f() {}
-// DEFAULT-DAG: define hidden void @_Z1fv()
-// DEFAULT-DAG: define dso_local void @_Z10exported_fv()
+// DEFAULTS-DAG: define hidden void @_Z1fv()
+// DEFAULTS-DAG: define void @_Z10exported_fv()
 // EXPLICIT-DAG: define protected void @_Z1fv()
 // EXPLICIT-DAG: define hidden void @_Z10exported_fv()
+// ALL_DEFAULT-DAG: define void @_Z1fv()
+// ALL_DEFAULT-DAG: define void @_Z10exported_fv()
 
 // Variable
 int d = 123;
 __declspec(dllexport) int exported_d = 123;
-// DEFAULT-DAG: @d = hidden global
-// DEFAULT-DAG: @exported_d = dso_local global
+// DEFAULTS-DAG: @d = hidden global
+// DEFAULTS-DAG: @exported_d = global
 // EXPLICIT-DAG: @d = protected global
 // EXPLICIT-DAG: @exported_d = hidden global
+// ALL_DEFAULT-DAG: @d = global
+// ALL_DEFAULT-DAG: @exported_d = global
 
 // Alias
 extern "C" void aliased() {}
 void a() __attribute__((alias("aliased")));
 void __declspec(dllexport) a_exported() __attribute__((alias("aliased")));
-// DEFAULT-DAG: @_Z1av = hidden alias
-// DEFAULT-DAG: @_Z10a_exportedv = dso_local alias
+// DEFAULTS-DAG: @_Z1av = hidden alias
+// DEFAULTS-DAG: @_Z10a_exportedv = alias
 // EXPLICIT-DAG: @_Z1av = protected alias
 // EXPLICIT-DAG: @_Z10a_exportedv = hidden alias
+// ALL_DEFAULT-DAG: @_Z1av = alias
+// ALL_DEFAULT-DAG: @_Z10a_exportedv = alias
 
 // Declaration
 extern void e();
 extern void __declspec(dllimport) imported_e();
-void use_declarations(){e(); imported_e();}
-// DEFAULT-DAG: declare hidden void @_Z1ev()
-// DEFAULT-DAG: declare void @_Z10imported_ev()
+// DEFAULTS-DAG: declare hidden void @_Z1ev()
+// DEFAULTS-DAG: declare void @_Z10imported_ev()
 // EXPLICIT-DAG: declare protected void @_Z1ev()
 // EXPLICIT-DAG: declare hidden void @_Z10imported_ev()
+// ALL_DEFAULT-DAG: declare void @_Z1ev()
+// ALL_DEFAULT-DAG: declare void @_Z10imported_ev()
+
+// Weak Declaration
+__attribute__((weak))
+extern void w();
+__attribute__((weak))
+extern void __declspec(dllimport) imported_w();
+// DEFAULTS-DAG: declare extern_weak hidden void @_Z1wv()
+// DEFAULTS-DAG: declare extern_weak void @_Z10imported_wv()
+// EXPLICIT-DAG: declare extern_weak protected void @_Z1wv()
+// EXPLICIT-DAG: declare extern_weak hidden void @_Z10imported_wv()
+// ALL_DEFAULT-DAG: declare extern_weak void @_Z1wv()
+// ALL_DEFAULT-DAG: declare extern_weak void @_Z10imported_wv()
+
+void use_declarations(){e(); imported_e(); w(); imported_w();}
 
 // Show that -fvisibility-from-dllstorageclass overrides the effect of visibility annotations.
 
 struct __attribute__((type_visibility("protected"))) t {
   virtual void foo();
 };
 void t::foo() {}
-// DEFAULT-DAG: @_ZTV1t = hidden unnamed_addr constant
+// DEFAULTS-DAG: @_ZTV1t = hidden unnamed_addr constant
 
 int v __attribute__ ((__visibility__ ("protected"))) = 123;
-// DEFAULT-DAG: @v = hidden global
+// DEFAULTS-DAG: @v = hidden global
 
 #pragma GCC visibility push(protected)
 int p = 345;
 #pragma GCC visibility pop
-// DEFAULT-DAG: @p = hidden global
+// DEFAULTS-DAG: @p = hidden global