Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "don't override" mapping for -fvisibility-from-dllstorageclass #74629

Merged
merged 5 commits into from
Jan 19, 2024

Conversation

bd1976bris
Copy link
Collaborator

-fvisibility-from-dllstorageclass allows for overriding the visibility of globals from their DLL storage class. The visibility to apply can be customised for the different classes of globals via a set of dependent options that specify the mapping values:

  • -fvisibility-dllexport=<value>
  • -fvisibility-nodllstorageclass=<value>
  • -fvisibility-externs-dllimport=<value>
  • -fvisibility-externs-nodllstorageclass=<value>

Currently, one of the existing LLVM visibilities, hidden, protected, default, can be used as a mapping value. This change adds a new mapping value: keep, which specifies that the visibility should not be overridden for that class of globals. The behaviour of -fvisibility-from-dllstorageclass is otherwise unchanged and existing uses of this set of options will be unaffected.

The background to this change is that currently the PS4 and PS5 compilers effectively ignore visibility - dllimport/export is the supported method for export control in C/C++ source code. Now, we would like to support visibility attributes and options in our frontend, in addition to dllimport/export. To support this, we will override the visibility of globals with explicit dllimport/export annotations but use the keep setting for globals which do not have an explicit dllimport/export.

There are also some minor improvements to the existing options:

  • Make the LANGOPS BENIGN as they don't involve the AST.
  • Correct/clarify the help text for the options.

`-fvisibility-from-dllstorageclass` allows for overriding the visibility
of globals from their DLL storage class. The visibility to apply can be
customised for the different classes of globals via a set of dependent
options that specify the mapping values:
- `-fvisibility-dllexport=<value>`
- `-fvisibility-nodllstorageclass=<value>`
- `-fvisibility-externs-dllimport=<value>`
- `-fvisibility-externs-nodllstorageclass=<value>`
Currently, one of the existing LLVM visibilities, `hidden`, `protected`,
`default`, can be used as a mapping value. This change adds a new
mapping value: `keep`, which specifies that the visibility should not
be overridden for that class of globals. The behaviour of
`-fvisibility-from-dllstorageclass` is otherwise unchanged and existing
uses of this set of options will be unaffected.

The background to this change is that currently the PS4 and PS5
compilers effectively ignore visibility - dllimport/export is the
supported method for export control in C/C++ source code. Now, we would
like to support visibility attributes and options in our frontend, in
addition to dllimport/export. To support this, we will override the
visibility of globals with explicit dllimport/export attributes but use
the `keep` setting for globals which do not have an explicit
dllimport/export.

There are also some minor improvements to the existing options:
- Make the LANGOPS `BENIGN` as they don't involve the AST.
- Correct/clarify the help text for the options.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen labels Dec 6, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 6, 2023

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang-codegen

Author: bd1976bris (bd1976bris)

Changes

-fvisibility-from-dllstorageclass allows for overriding the visibility of globals from their DLL storage class. The visibility to apply can be customised for the different classes of globals via a set of dependent options that specify the mapping values:

  • -fvisibility-dllexport=&lt;value&gt;
  • -fvisibility-nodllstorageclass=&lt;value&gt;
  • -fvisibility-externs-dllimport=&lt;value&gt;
  • -fvisibility-externs-nodllstorageclass=&lt;value&gt;

Currently, one of the existing LLVM visibilities, hidden, protected, default, can be used as a mapping value. This change adds a new mapping value: keep, which specifies that the visibility should not be overridden for that class of globals. The behaviour of -fvisibility-from-dllstorageclass is otherwise unchanged and existing uses of this set of options will be unaffected.

The background to this change is that currently the PS4 and PS5 compilers effectively ignore visibility - dllimport/export is the supported method for export control in C/C++ source code. Now, we would like to support visibility attributes and options in our frontend, in addition to dllimport/export. To support this, we will override the visibility of globals with explicit dllimport/export annotations but use the keep setting for globals which do not have an explicit dllimport/export.

There are also some minor improvements to the existing options:

  • Make the LANGOPS BENIGN as they don't involve the AST.
  • Correct/clarify the help text for the options.

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

6 Files Affected:

  • (modified) clang/include/clang/Basic/LangOptions.def (+10-10)
  • (modified) clang/include/clang/Basic/LangOptions.h (+11)
  • (modified) clang/include/clang/Driver/Options.td (+14-9)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+54-27)
  • (modified) clang/test/CodeGenCXX/visibility-dllstorageclass.cpp (+26-2)
  • (modified) clang/test/Driver/visibility-dllstorageclass.c (+33-1)
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index c3d5399905a3f..1ebe33f7151e8 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -357,16 +357,16 @@ BENIGN_ENUM_LANGOPT(TypeVisibilityMode, Visibility, 3, DefaultVisibility,
              "default visibility for types [-ftype-visibility]")
 LANGOPT(SetVisibilityForExternDecls, 1, 0,
         "apply global symbol visibility to external declarations without an explicit visibility")
-LANGOPT(VisibilityFromDLLStorageClass, 1, 0,
-        "set the visiblity of globals from their DLL storage class [-fvisibility-from-dllstorageclass]")
-ENUM_LANGOPT(DLLExportVisibility, Visibility, 3, DefaultVisibility,
-             "visibility for functions and variables with dllexport annotations [-fvisibility-from-dllstorageclass]")
-ENUM_LANGOPT(NoDLLStorageClassVisibility, Visibility, 3, HiddenVisibility,
-             "visibility for functions and variables without an explicit DLL storage class [-fvisibility-from-dllstorageclass]")
-ENUM_LANGOPT(ExternDeclDLLImportVisibility, Visibility, 3, DefaultVisibility,
-             "visibility for external declarations with dllimport annotations [-fvisibility-from-dllstorageclass]")
-ENUM_LANGOPT(ExternDeclNoDLLStorageClassVisibility, Visibility, 3, HiddenVisibility,
-             "visibility for external declarations without an explicit DLL storage class [-fvisibility-from-dllstorageclass]")
+BENIGN_LANGOPT(VisibilityFromDLLStorageClass, 1, 0,
+               "override the visibility of globals based on their final DLL storage class [-fvisibility-from-dllstorageclass]")
+BENIGN_ENUM_LANGOPT(DLLExportVisibility, VisibilityFromDLLStorageClassKinds, 3, VisibilityFromDLLStorageClassKinds::Default,
+             "how to adjust the visibility for functions and variables with dllexport annotations [-fvisibility-dllexport]")
+BENIGN_ENUM_LANGOPT(NoDLLStorageClassVisibility, VisibilityFromDLLStorageClassKinds, 3, VisibilityFromDLLStorageClassKinds::Hidden,
+             "how to adjust the visibility for functions and variables without an explicit DLL storage class [-fvisibility-nodllstorageclass]")
+BENIGN_ENUM_LANGOPT(ExternDeclDLLImportVisibility, VisibilityFromDLLStorageClassKinds, 3, VisibilityFromDLLStorageClassKinds::Default,
+             "how to adjust the visibility for external declarations with dllimport annotations [-fvisibility-externs-dllimport]")
+BENIGN_ENUM_LANGOPT(ExternDeclNoDLLStorageClassVisibility, VisibilityFromDLLStorageClassKinds, 3,  VisibilityFromDLLStorageClassKinds::Hidden,
+             "how to adjust the visibility for external declarations without an explicit DLL storage class [-fvisibility-externs-nodllstorageclass]")
 BENIGN_LANGOPT(SemanticInterposition        , 1, 0, "semantic interposition")
 BENIGN_LANGOPT(HalfNoSemanticInterposition, 1, 0,
                "Like -fno-semantic-interposition but don't use local aliases")
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index 2d167dd2bdf12..5688aae0fd23c 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -380,6 +380,17 @@ class LangOptions : public LangOptionsBase {
     All,
   };
 
+  enum class VisibilityFromDLLStorageClassKinds {
+    /// Keep the IR-gen assigned visibility.
+    Keep,
+    /// Override the IR-gen assigned visibility with default visibility.
+    Default,
+    /// Override the IR-gen assigned visibility with hidden visibility.
+    Hidden,
+    /// Override the IR-gen assigned visibility with protected visibility.
+    Protected,
+  };
+
   enum class StrictFlexArraysLevelKind {
     /// Any trailing array member is a FAM.
     Default = 0,
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index db2190318c931..0400e93528564 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3821,27 +3821,32 @@ def dA : Flag<["-"], "dA">, Alias<fverbose_asm>;
 defm visibility_from_dllstorageclass : BoolFOption<"visibility-from-dllstorageclass",
   LangOpts<"VisibilityFromDLLStorageClass">, DefaultFalse,
   PosFlag<SetTrue, [], [ClangOption, CC1Option],
-          "Set the visibility of symbols in the generated code from their DLL storage class">,
+          "Override the visibility of globals based on their final DLL storage class.s">,
   NegFlag<SetFalse>>;
+class MarshallingInfoVisibilityFromDLLStorage<KeyPathAndMacro kpm, code default>
+  : MarshallingInfoEnum<kpm, default>,
+    Values<"keep,hidden,protected,default">,
+    NormalizedValuesScope<"LangOptions::VisibilityFromDLLStorageClassKinds">,
+    NormalizedValues<["Keep", "Hidden", "Protected", "Default"]> {}
 def fvisibility_dllexport_EQ : Joined<["-"], "fvisibility-dllexport=">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"The visibility for dllexport definitions [-fvisibility-from-dllstorageclass]">,
-  MarshallingInfoVisibility<LangOpts<"DLLExportVisibility">, "DefaultVisibility">,
+  HelpText<"The visibility for dllexport definitions. If Keep is specified the visibility is not adjusted [-fvisibility-from-dllstorageclass]">,
+  MarshallingInfoVisibilityFromDLLStorage<LangOpts<"DLLExportVisibility">, "Default">,
   ShouldParseIf<fvisibility_from_dllstorageclass.KeyPath>;
 def fvisibility_nodllstorageclass_EQ : Joined<["-"], "fvisibility-nodllstorageclass=">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"The visibility for definitions without an explicit DLL export class [-fvisibility-from-dllstorageclass]">,
-  MarshallingInfoVisibility<LangOpts<"NoDLLStorageClassVisibility">, "HiddenVisibility">,
+  HelpText<"The visibility for definitions without an explicit DLL storage class. If Keep is specified the visibility is not adjusted [-fvisibility-from-dllstorageclass]">,
+  MarshallingInfoVisibilityFromDLLStorage<LangOpts<"NoDLLStorageClassVisibility">, "Hidden">,
   ShouldParseIf<fvisibility_from_dllstorageclass.KeyPath>;
 def fvisibility_externs_dllimport_EQ : Joined<["-"], "fvisibility-externs-dllimport=">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"The visibility for dllimport external declarations [-fvisibility-from-dllstorageclass]">,
-  MarshallingInfoVisibility<LangOpts<"ExternDeclDLLImportVisibility">, "DefaultVisibility">,
+  HelpText<"The visibility for dllimport external declarations. If Keep is specified the visibility is not adjusted [-fvisibility-from-dllstorageclass]">,
+  MarshallingInfoVisibilityFromDLLStorage<LangOpts<"ExternDeclDLLImportVisibility">, "Default">,
   ShouldParseIf<fvisibility_from_dllstorageclass.KeyPath>;
 def fvisibility_externs_nodllstorageclass_EQ : Joined<["-"], "fvisibility-externs-nodllstorageclass=">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"The visibility for external declarations without an explicit DLL dllstorageclass [-fvisibility-from-dllstorageclass]">,
-  MarshallingInfoVisibility<LangOpts<"ExternDeclNoDLLStorageClassVisibility">, "HiddenVisibility">,
+  HelpText<"The visibility for external declarations without an explicit DLL storage class. If Keep is specified the visibility is not adjusted [-fvisibility-from-dllstorageclass]">,
+  MarshallingInfoVisibilityFromDLLStorage<LangOpts<"ExternDeclNoDLLStorageClassVisibility">, "Hidden">,
   ShouldParseIf<fvisibility_from_dllstorageclass.KeyPath>;
 def fvisibility_EQ : Joined<["-"], "fvisibility=">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option]>,
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index dea58a7ff4146..6c573853bc9a3 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -721,43 +721,70 @@ void InstrProfStats::reportDiagnostics(DiagnosticsEngine &Diags,
   }
 }
 
+static std::optional<llvm::GlobalValue::VisibilityTypes>
+getLLVMVisibility(clang::LangOptions::VisibilityFromDLLStorageClassKinds K) {
+  // Map to LLVM visibility.
+  switch (K) {
+  case clang::LangOptions::VisibilityFromDLLStorageClassKinds::Keep:
+    return std::nullopt;
+  case clang::LangOptions::VisibilityFromDLLStorageClassKinds::Default:
+    return llvm::GlobalValue::DefaultVisibility;
+  case clang::LangOptions::VisibilityFromDLLStorageClassKinds::Hidden:
+    return llvm::GlobalValue::HiddenVisibility;
+  case clang::LangOptions::VisibilityFromDLLStorageClassKinds::Protected:
+    return llvm::GlobalValue::ProtectedVisibility;
+  }
+  llvm_unreachable("unknown option value!");
+}
+
+void setLLVMVisibility(llvm::GlobalValue &GV,
+                       std::optional<llvm::GlobalValue::VisibilityTypes> V) {
+  if (!V)
+    return;
+
+  // Reset DSO locality before setting the visibility. This removes
+  // any effects that visibility options and annotations may have
+  // had 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);
+  GV.setVisibility(*V);
+}
+
 static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
                                              llvm::Module &M) {
   if (!LO.VisibilityFromDLLStorageClass)
     return;
 
-  llvm::GlobalValue::VisibilityTypes DLLExportVisibility =
-      CodeGenModule::GetLLVMVisibility(LO.getDLLExportVisibility());
-  llvm::GlobalValue::VisibilityTypes NoDLLStorageClassVisibility =
-      CodeGenModule::GetLLVMVisibility(LO.getNoDLLStorageClassVisibility());
-  llvm::GlobalValue::VisibilityTypes ExternDeclDLLImportVisibility =
-      CodeGenModule::GetLLVMVisibility(LO.getExternDeclDLLImportVisibility());
-  llvm::GlobalValue::VisibilityTypes ExternDeclNoDLLStorageClassVisibility =
-      CodeGenModule::GetLLVMVisibility(
-          LO.getExternDeclNoDLLStorageClassVisibility());
+  std::optional<llvm::GlobalValue::VisibilityTypes> DLLExportVisibility =
+      getLLVMVisibility(LO.getDLLExportVisibility());
+
+  std::optional<llvm::GlobalValue::VisibilityTypes>
+      NoDLLStorageClassVisibility =
+          getLLVMVisibility(LO.getNoDLLStorageClassVisibility());
+
+  std::optional<llvm::GlobalValue::VisibilityTypes>
+      ExternDeclDLLImportVisibility =
+          getLLVMVisibility(LO.getExternDeclDLLImportVisibility());
+
+  std::optional<llvm::GlobalValue::VisibilityTypes>
+      ExternDeclNoDLLStorageClassVisibility =
+          getLLVMVisibility(LO.getExternDeclNoDLLStorageClassVisibility());
 
   for (llvm::GlobalValue &GV : M.global_values()) {
     if (GV.hasAppendingLinkage() || GV.hasLocalLinkage())
       continue;
 
-    // Reset DSO locality before setting the visibility. This removes
-    // any effects that visibility options and annotations may have
-    // had 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
-                           ? DLLExportVisibility
-                           : NoDLLStorageClassVisibility);
-    }
+    if (GV.isDeclarationForLinker())
+      setLLVMVisibility(GV, GV.getDLLStorageClass() ==
+                                    llvm::GlobalValue::DLLImportStorageClass
+                                ? ExternDeclDLLImportVisibility
+                                : ExternDeclNoDLLStorageClassVisibility);
+    else
+      setLLVMVisibility(GV, GV.getDLLStorageClass() ==
+                                    llvm::GlobalValue::DLLExportStorageClass
+                                ? DLLExportVisibility
+                                : NoDLLStorageClassVisibility);
 
     GV.setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
   }
diff --git a/clang/test/CodeGenCXX/visibility-dllstorageclass.cpp b/clang/test/CodeGenCXX/visibility-dllstorageclass.cpp
index 6520e7039728e..9f10a016ed90d 100644
--- a/clang/test/CodeGenCXX/visibility-dllstorageclass.cpp
+++ b/clang/test/CodeGenCXX/visibility-dllstorageclass.cpp
@@ -1,7 +1,7 @@
 // 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.
+//// 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 \
@@ -32,12 +32,23 @@
 // RUN:     -x c++  %s -S -emit-llvm -o - | \
 // RUN:   FileCheck %s --check-prefixes=ALL_DEFAULT
 
+// 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=protected \
+// RUN:     -fvisibility-nodllstorageclass=keep \
+// RUN:     -fvisibility-externs-dllimport=default \
+// RUN:     -fvisibility-externs-nodllstorageclass=keep \
+// RUN:     -x c++  %s -S -emit-llvm -o - | tee %t.log | \
+// RUN:   FileCheck %s --check-prefixes=ALL_KEEP
 // Local
 static void l() {}
 void use_locals(){l();}
 // DEFAULTS-DAG: define internal void @_ZL1lv()
 // EXPLICIT-DAG: define internal void @_ZL1lv()
 // ALL_DEFAULT-DAG: define internal void @_ZL1lv()
+// ALL_KEEP-DAG: define internal void @_ZL1lv()
 
 // Function
 void f() {}
@@ -48,6 +59,8 @@ void __declspec(dllexport) exported_f() {}
 // EXPLICIT-DAG: define hidden void @_Z10exported_fv()
 // ALL_DEFAULT-DAG: define void @_Z1fv()
 // ALL_DEFAULT-DAG: define void @_Z10exported_fv()
+// ALL_KEEP-DAG: define hidden void @_Z1fv()
+// ALL_KEEP-DAG: define protected void @_Z10exported_fv()
 
 // Variable
 int d = 123;
@@ -58,6 +71,8 @@ __declspec(dllexport) int exported_d = 123;
 // EXPLICIT-DAG: @exported_d = hidden global
 // ALL_DEFAULT-DAG: @d = global
 // ALL_DEFAULT-DAG: @exported_d = global
+// ALL_KEEP-DAG: @d = hidden global
+// ALL_KEEP-DAG: @exported_d = protected global
 
 // Alias
 extern "C" void aliased() {}
@@ -69,6 +84,8 @@ void __declspec(dllexport) a_exported() __attribute__((alias("aliased")));
 // EXPLICIT-DAG: @_Z10a_exportedv = hidden alias
 // ALL_DEFAULT-DAG: @_Z1av = alias
 // ALL_DEFAULT-DAG: @_Z10a_exportedv = alias
+// ALL_KEEP-DAG: @_Z1av = hidden alias
+// ALL_KEEP-DAG: @_Z10a_exportedv = protected alias
 
 // Declaration
 extern void e();
@@ -79,6 +96,8 @@ extern void __declspec(dllimport) imported_e();
 // EXPLICIT-DAG: declare hidden void @_Z10imported_ev()
 // ALL_DEFAULT-DAG: declare void @_Z1ev()
 // ALL_DEFAULT-DAG: declare void @_Z10imported_ev()
+// ALL_KEEP-DAG: declare hidden void @_Z1ev()
+// ALL_KEEP-DAG: void @_Z10imported_ev()
 
 // Weak Declaration
 __attribute__((weak))
@@ -91,6 +110,8 @@ extern void __declspec(dllimport) imported_w();
 // 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()
+// ALL_KEEP-DAG: declare extern_weak hidden void @_Z1wv()
+// ALL_KEEP-DAG: declare extern_weak void @_Z10imported_wv()
 
 void use_declarations(){e(); imported_e(); w(); imported_w();}
 
@@ -101,11 +122,14 @@ struct __attribute__((type_visibility("protected"))) t {
 };
 void t::foo() {}
 // DEFAULTS-DAG: @_ZTV1t = hidden unnamed_addr constant
+// ALL_KEEP-DAG: @_ZTV1t = protected unnamed_addr constant
 
 int v __attribute__ ((__visibility__ ("protected"))) = 123;
 // DEFAULTS-DAG: @v = hidden global
+// ALL_KEEP-DAG: @v = protected global
 
 #pragma GCC visibility push(protected)
 int p = 345;
 #pragma GCC visibility pop
 // DEFAULTS-DAG: @p = hidden global
+// ALL_KEEP-DAG: @p = protected global
diff --git a/clang/test/Driver/visibility-dllstorageclass.c b/clang/test/Driver/visibility-dllstorageclass.c
index 1e495f694a5a5..65d13d348cc97 100644
--- a/clang/test/Driver/visibility-dllstorageclass.c
+++ b/clang/test/Driver/visibility-dllstorageclass.c
@@ -1,4 +1,4 @@
-// Check behaviour of -fvisibility-from-dllstorageclass options
+//// Check behaviour of -fvisibility-from-dllstorageclass options
 
 // RUN: %clang -target x86_64-unknown-windows-itanium -fdeclspec \
 // RUN:     -Werror -S -### %s 2>&1 | \
@@ -85,3 +85,35 @@
 // ALL-SAME: "-fvisibility-nodllstorageclass=protected"
 // ALL-SAME: "-fvisibility-externs-dllimport=hidden"
 // ALL-SAME: "-fvisibility-externs-nodllstorageclass=protected"
+
+//// Test that "keep" can be specified, which means that the visibility of
+//// the matching globals will not be adjusted.
+
+// RUN: %clang -target x86_64-unknown-windows-itanium -fdeclspec \
+// RUN:     -fvisibility-from-dllstorageclass \
+// RUN:     -fvisibility-dllexport=keep \
+// RUN:     -fvisibility-nodllstorageclass=keep \
+// RUN:     -fvisibility-externs-dllimport=keep \
+// RUN:     -fvisibility-externs-nodllstorageclass=keep \
+// RUN:     -Werror -S -### %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=KEEP
+
+// KEEP:        "-fvisibility-from-dllstorageclass"
+// KEEP-SAME:   "-fvisibility-dllexport=keep"
+// KEEP-SAME:   "-fvisibility-nodllstorageclass=keep"
+// KEEP-SAME:   "-fvisibility-externs-dllimport=keep"
+// KEEP-SAME:   "-fvisibility-externs-nodllstorageclass=keep"
+
+// RUN: %clang -target x86_64-unknown-windows-itanium -fdeclspec \
+// RUN:     -fvisibility-from-dllstorageclass \
+// RUN:     -fvisibility-dllexport=default \
+// RUN:     -fvisibility-dllexport=keep \
+// RUN:     -fvisibility-nodllstorageclass=default \
+// RUN:     -fvisibility-nodllstorageclass=keep \
+// RUN:     -fvisibility-externs-dllimport=default \
+// RUN:     -fvisibility-externs-dllimport=keep \
+// RUN:     -fvisibility-externs-nodllstorageclass=default \
+// RUN:     -fvisibility-externs-nodllstorageclass=keep \
+// RUN:     -Werror -S -### %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=KEEP
+

@pogo59
Copy link
Collaborator

pogo59 commented Dec 18, 2023

Ben and I talked about this offline. I was a little skittish about these power-user options letting you do whatever you wanted, and maybe a small number (2? 3?) of predefined combinations would be more straightforward. He convinced me that (a) these wouldn't be needed often, and (b) people who did want to use them would already be thinking in terms of visibility so adding predefined combinations would actually add more complexity.

@bd1976bris bd1976bris requested a review from smeenai December 18, 2023 20:31
Copy link
Collaborator

@pogo59 pogo59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -79,6 +107,8 @@ extern void __declspec(dllimport) imported_e();
// EXPLICIT-DAG: declare hidden void @_Z10imported_ev()
// ALL_DEFAULT-DAG: declare void @_Z1ev()
// ALL_DEFAULT-DAG: declare void @_Z10imported_ev()
// ALL_KEEP-DAG: declare hidden void @_Z1ev()
// ALL_KEEP-DAG: void @_Z10imported_ev()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this line looks like it would allow any visibility (missing declare and whatever might be between that and the void)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thanks the declare is missing here. I have added it back.

Copy link
Collaborator

@pogo59 pogo59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving.

@bd1976bris bd1976bris merged commit cd05ade into llvm:main Jan 19, 2024
3 of 4 checks passed
@tstellar
Copy link
Collaborator

This commit broke the llvm-x86_64-debian-dylib bot

bd1976bris added a commit that referenced this pull request Jan 20, 2024
This test-case was generating invalid IR and causing the test to fail.

Given that the reviewer initially accepted the change without this
test case I feel that it is appropriate to remove this test case,
to get the build to pass, and find a way to reimplement this test-case
in a later commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants