Skip to content

[Driver][PS5] Set visibility option defaults #92091

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2024
Merged

[Driver][PS5] Set visibility option defaults #92091

merged 1 commit into from
May 20, 2024

Conversation

bd1976bris
Copy link
Collaborator

Adjust the PS5 driver defaults for the -fvisibility-from-dllstorageclass
sub-options so that only globals with dllimport/dllexport annotations
are adjusted. This allows globals without dllimport/export to retain
the visibility and pre-emptability assigned during IR-Gen. Set
-fvisibility=hidden on PS5 by default to compensate for no longer
overriding the visibility of definitions without dllexport. Note there
is no behaviour change for PS4 (the behaviour of overriding the
visibility for all globals is retained on PS4).

@bd1976bris
Use symbol visibility for export control in the compiler frontend for…
dcf195d 
… PS5

Adjust the PS5 driver defaults for the `-fvisibility-from-dllstorageclass`
sub-options so that only globals with dllimport/dllexport annotations
are adjusted. This allows globals without dllimport/export to retain
the visibility and pre-emptability assigned during IR-Gen. Set
`-fvisibility=hidden` on PS5 by default to compensate for no longer
overriding the visibility of definitions without dllexport. Note there
is no behaviour change for PS4 (the behaviour of overriding the
visibility for all globals is retained on PS4).
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels May 14, 2024
@bd1976bris bd1976bris requested a review from pogo59 May 14, 2024 09:26
@llvmbot
Copy link
Member

llvmbot commented May 14, 2024

@llvm/pr-subscribers-clang-driver

Author: bd1976bris (bd1976bris)

Changes

Adjust the PS5 driver defaults for the -fvisibility-from-dllstorageclass
sub-options so that only globals with dllimport/dllexport annotations
are adjusted. This allows globals without dllimport/export to retain
the visibility and pre-emptability assigned during IR-Gen. Set
-fvisibility=hidden on PS5 by default to compensate for no longer
overriding the visibility of definitions without dllexport. Note there
is no behaviour change for PS4 (the behaviour of overriding the
visibility for all globals is retained on PS4).

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

4 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/PS4CPU.cpp (+16-2)
  • (modified) clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c (+26-13)
  • (added) clang/test/Driver/ps4-visibility.cl (+32)
  • (added) clang/test/Driver/ps5-visibility.cl (+33)
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 7bf9aa79384c5..3fd62d9793093 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -358,6 +358,12 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
 
   CC1Args.push_back("-fno-use-init-array");
 
+  // Default to `hidden` visibility for PS5.
+  if (getTriple().isPS5() &&
+      !DriverArgs.hasArg(options::OPT_fvisibility_EQ,
+                         options::OPT_fvisibility_ms_compat))
+    CC1Args.push_back("-fvisibility=hidden");
+
   // Default to -fvisibility-global-new-delete=source for PS5.
   if (getTriple().isPS5() &&
       !DriverArgs.hasArg(options::OPT_fvisibility_global_new_delete_EQ,
@@ -376,11 +382,15 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
     else
       CC1Args.push_back("-fvisibility-dllexport=protected");
 
+    // For PS4 we override the visibilty of globals definitions without
+    // dllimport or  dllexport annotations.
     if (DriverArgs.hasArg(options::OPT_fvisibility_nodllstorageclass_EQ))
       DriverArgs.AddLastArg(CC1Args,
                             options::OPT_fvisibility_nodllstorageclass_EQ);
-    else
+    else if (getTriple().isPS4())
       CC1Args.push_back("-fvisibility-nodllstorageclass=hidden");
+    else
+      CC1Args.push_back("-fvisibility-nodllstorageclass=keep");
 
     if (DriverArgs.hasArg(options::OPT_fvisibility_externs_dllimport_EQ))
       DriverArgs.AddLastArg(CC1Args,
@@ -388,12 +398,16 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
     else
       CC1Args.push_back("-fvisibility-externs-dllimport=default");
 
+    // For PS4 we override the visibilty of external globals without
+    // dllimport or  dllexport annotations.
     if (DriverArgs.hasArg(
             options::OPT_fvisibility_externs_nodllstorageclass_EQ))
       DriverArgs.AddLastArg(
           CC1Args, options::OPT_fvisibility_externs_nodllstorageclass_EQ);
-    else
+    else if (getTriple().isPS4())
       CC1Args.push_back("-fvisibility-externs-nodllstorageclass=default");
+    else
+      CC1Args.push_back("-fvisibility-externs-nodllstorageclass=keep");
   }
 }
 
diff --git a/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c b/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c
index 430827805a8fd..71f8661679eb7 100644
--- a/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c
+++ b/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c
@@ -1,16 +1,19 @@
 // Check behaviour of -fvisibility-from-dllstorageclass options for PS4/PS5.
 
 // DEFINE: %{triple} =
+// DEFINE: %{prefix} =
 // DEFINE: %{run} = \
 // DEFINE: %clang -### -target %{triple} %s -Werror -o - 2>&1 | \
-// DEFINE:   FileCheck %s --check-prefix=DEFAULTS \
+// DEFINE:   FileCheck %s --check-prefixes=DEFAULTS,%{prefix} \
 // DEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // DEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // DEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // DEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // DEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = DEFAULTS-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = DEFAULTS-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 //
@@ -20,25 +23,29 @@
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -Werror \
 // REDEFINE:     %s -o - 2>&1 | \
-// REDEFINE:   FileCheck %s --check-prefix=DEFAULTS \
+// REDEFINE:   FileCheck %s --check-prefixes=DEFAULTS,%{prefix} \
 // REDEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = DEFAULTS-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = DEFAULTS-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
 // DEFAULTS:      "-fvisibility-from-dllstorageclass"
 // DEFAULTS-SAME: "-fvisibility-dllexport=protected"
-// DEFAULTS-SAME: "-fvisibility-nodllstorageclass=hidden"
+// DEFAULTS-PS4-SAME: "-fvisibility-nodllstorageclass=hidden"
+// DEFAULTS-PS5-SAME: "-fvisibility-nodllstorageclass=keep"
 // DEFAULTS-SAME: "-fvisibility-externs-dllimport=default"
-// DEFAULTS-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// DEFAULTS-PS4-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// DEFAULTS-PS5-SAME: "-fvisibility-externs-nodllstorageclass=keep"
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -fvisibility-dllexport=hidden \
 // REDEFINE:     -fvisibility-nodllstorageclass=protected \
@@ -64,37 +71,41 @@
 // UNUSED-NEXT: warning: argument unused during compilation: '-fvisibility-externs-nodllstorageclass=protected'
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-nodllstorageclass=protected \
 // REDEFINE:     -fvisibility-externs-dllimport=hidden \
 // REDEFINE:     -Werror \
 // REDEFINE:     %s -o - 2>&1 | \
-// REDEFINE:   FileCheck %s -check-prefix=SOME \
+// REDEFINE:   FileCheck %s -check-prefixes=SOME,%{prefix} \
 // REDEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = SOME-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = SOME-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -fvisibility-nodllstorageclass=protected \
 // REDEFINE:     -fvisibility-externs-dllimport=hidden \
 // REDEFINE:     -Werror \
 // REDEFINE:     %s -o - 2>&1 | \
-// REDEFINE:   FileCheck %s -check-prefix=SOME \
+// REDEFINE:   FileCheck %s -check-prefixes=SOME,%{prefix} \
 // REDEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = SOME-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = SOME-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
@@ -102,10 +113,11 @@
 // SOME-SAME: "-fvisibility-dllexport=protected"
 // SOME-SAME: "-fvisibility-nodllstorageclass=protected"
 // SOME-SAME: "-fvisibility-externs-dllimport=hidden"
-// SOME-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// SOME-PS4-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// SOME-PS5-SAME: "-fvisibility-externs-nodllstorageclass=keep"
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-dllexport=default \
 // REDEFINE:     -fvisibility-dllexport=hidden \
 // REDEFINE:     -fvisibility-nodllstorageclass=default \
@@ -121,14 +133,15 @@
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
-// REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass \
+// REDEFINE:     --implicit-check-not="warning: argument unused"
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -fvisibility-dllexport=default \
 // REDEFINE:     -fvisibility-dllexport=hidden \
diff --git a/clang/test/Driver/ps4-visibility.cl b/clang/test/Driver/ps4-visibility.cl
new file mode 100644
index 0000000000000..a0ed7c71f1f0e
--- /dev/null
+++ b/clang/test/Driver/ps4-visibility.cl
@@ -0,0 +1,32 @@
+/// Check PS4 specific interactions between visibility options.
+/// Detailed testing of -fvisibility-from-dllstorageclass is covered elsewhere.
+
+/// Check defaults.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// DEFAULT-DAG: "-fvisibility-from-dllstorageclass"
+// DEFAULT-DAG: "-fvisibility-dllexport=protected"
+// DEFAULT-DAG: "-fvisibility-nodllstorageclass=hidden"
+// DEFAULT-DAG: "-fvisibility-externs-dllimport=default"
+// DEFAULT-DAG: "-fvisibility-externs-nodllstorageclass=default"
+
+/// Check that -fvisibility-from-dllstorageclass is added in the presence of -fvisibility=.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fvisibility=default  %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=DEFAULT,VISEQUALS %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// VISEQUALS-DAG: "-fvisibility=default"
+
+/// Check that -fvisibility-from-dllstorageclass is added in the presence of -fvisibility-ms-compat.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fvisibility-ms-compat %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=DEFAULT,MSCOMPT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// MSCOMPT-DAG: "-fvisibility=hidden"
+// MSCOMPT-DAG: "-ftype-visibility=default"
+
+/// -fvisibility-from-dllstorageclass added explicitly.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fvisibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+
+/// -fvisibility-from-dllstorageclass disabled explicitly.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fno-visibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=NOVISFROM %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// NOVISFROM-NOT: "-fvisibility-from-dllstorageclass"
+
diff --git a/clang/test/Driver/ps5-visibility.cl b/clang/test/Driver/ps5-visibility.cl
new file mode 100644
index 0000000000000..ad144057be63a
--- /dev/null
+++ b/clang/test/Driver/ps5-visibility.cl
@@ -0,0 +1,33 @@
+/// Check PS5 specific interactions between visibility options.
+/// Detailed testing of -fvisibility-from-dllstorageclass is covered elsewhere.
+
+/// Check defaults.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// VDEFAULT-DAG:     "-fvisibility=hidden"
+// VGND_DEFAULT-DAG: "-fvisibility-global-new-delete=source"
+// DEFAULT-DAG:      "-fvisibility-from-dllstorageclass"
+// DEFAULT-DAG:      "-fvisibility-dllexport=protected"
+// DEFAULT-DAG:      "-fvisibility-nodllstorageclass=keep"
+// DEFAULT-DAG:      "-fvisibility-externs-dllimport=default"
+// DEFAULT-DAG:      "-fvisibility-externs-nodllstorageclass=keep"
+
+/// -fvisibility= specified explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fvisibility=protected %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VPROTECTED,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// VPROTECTED-DAG: "-fvisibility=protected"
+
+/// -fvisibility-ms-compat added explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fvisibility-ms-compat %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=MSCOMPT,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// MSCOMPT-DAG: "-fvisibility=hidden"
+// MSCOMPT-DAG: "-ftype-visibility=default"
+
+/// -fvisibility-from-dllstorageclass added explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fvisibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+
+/// -fvisibility-from-dllstorageclass disabled explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fno-visibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,NOVISFROM %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// NOVISFROM-NOT: "-fvisibility-from-dllstorageclass"

@llvmbot
Copy link
Member

llvmbot commented May 14, 2024

@llvm/pr-subscribers-clang

Author: bd1976bris (bd1976bris)

Changes

Adjust the PS5 driver defaults for the -fvisibility-from-dllstorageclass
sub-options so that only globals with dllimport/dllexport annotations
are adjusted. This allows globals without dllimport/export to retain
the visibility and pre-emptability assigned during IR-Gen. Set
-fvisibility=hidden on PS5 by default to compensate for no longer
overriding the visibility of definitions without dllexport. Note there
is no behaviour change for PS4 (the behaviour of overriding the
visibility for all globals is retained on PS4).

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

4 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/PS4CPU.cpp (+16-2)
  • (modified) clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c (+26-13)
  • (added) clang/test/Driver/ps4-visibility.cl (+32)
  • (added) clang/test/Driver/ps5-visibility.cl (+33)
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 7bf9aa79384c5..3fd62d9793093 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -358,6 +358,12 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
 
   CC1Args.push_back("-fno-use-init-array");
 
+  // Default to `hidden` visibility for PS5.
+  if (getTriple().isPS5() &&
+      !DriverArgs.hasArg(options::OPT_fvisibility_EQ,
+                         options::OPT_fvisibility_ms_compat))
+    CC1Args.push_back("-fvisibility=hidden");
+
   // Default to -fvisibility-global-new-delete=source for PS5.
   if (getTriple().isPS5() &&
       !DriverArgs.hasArg(options::OPT_fvisibility_global_new_delete_EQ,
@@ -376,11 +382,15 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
     else
       CC1Args.push_back("-fvisibility-dllexport=protected");
 
+    // For PS4 we override the visibilty of globals definitions without
+    // dllimport or  dllexport annotations.
     if (DriverArgs.hasArg(options::OPT_fvisibility_nodllstorageclass_EQ))
       DriverArgs.AddLastArg(CC1Args,
                             options::OPT_fvisibility_nodllstorageclass_EQ);
-    else
+    else if (getTriple().isPS4())
       CC1Args.push_back("-fvisibility-nodllstorageclass=hidden");
+    else
+      CC1Args.push_back("-fvisibility-nodllstorageclass=keep");
 
     if (DriverArgs.hasArg(options::OPT_fvisibility_externs_dllimport_EQ))
       DriverArgs.AddLastArg(CC1Args,
@@ -388,12 +398,16 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
     else
       CC1Args.push_back("-fvisibility-externs-dllimport=default");
 
+    // For PS4 we override the visibilty of external globals without
+    // dllimport or  dllexport annotations.
     if (DriverArgs.hasArg(
             options::OPT_fvisibility_externs_nodllstorageclass_EQ))
       DriverArgs.AddLastArg(
           CC1Args, options::OPT_fvisibility_externs_nodllstorageclass_EQ);
-    else
+    else if (getTriple().isPS4())
       CC1Args.push_back("-fvisibility-externs-nodllstorageclass=default");
+    else
+      CC1Args.push_back("-fvisibility-externs-nodllstorageclass=keep");
   }
 }
 
diff --git a/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c b/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c
index 430827805a8fd..71f8661679eb7 100644
--- a/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c
+++ b/clang/test/Driver/ps4-ps5-visibility-dllstorageclass.c
@@ -1,16 +1,19 @@
 // Check behaviour of -fvisibility-from-dllstorageclass options for PS4/PS5.
 
 // DEFINE: %{triple} =
+// DEFINE: %{prefix} =
 // DEFINE: %{run} = \
 // DEFINE: %clang -### -target %{triple} %s -Werror -o - 2>&1 | \
-// DEFINE:   FileCheck %s --check-prefix=DEFAULTS \
+// DEFINE:   FileCheck %s --check-prefixes=DEFAULTS,%{prefix} \
 // DEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // DEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // DEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // DEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // DEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = DEFAULTS-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = DEFAULTS-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 //
@@ -20,25 +23,29 @@
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -Werror \
 // REDEFINE:     %s -o - 2>&1 | \
-// REDEFINE:   FileCheck %s --check-prefix=DEFAULTS \
+// REDEFINE:   FileCheck %s --check-prefixes=DEFAULTS,%{prefix} \
 // REDEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = DEFAULTS-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = DEFAULTS-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
 // DEFAULTS:      "-fvisibility-from-dllstorageclass"
 // DEFAULTS-SAME: "-fvisibility-dllexport=protected"
-// DEFAULTS-SAME: "-fvisibility-nodllstorageclass=hidden"
+// DEFAULTS-PS4-SAME: "-fvisibility-nodllstorageclass=hidden"
+// DEFAULTS-PS5-SAME: "-fvisibility-nodllstorageclass=keep"
 // DEFAULTS-SAME: "-fvisibility-externs-dllimport=default"
-// DEFAULTS-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// DEFAULTS-PS4-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// DEFAULTS-PS5-SAME: "-fvisibility-externs-nodllstorageclass=keep"
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -fvisibility-dllexport=hidden \
 // REDEFINE:     -fvisibility-nodllstorageclass=protected \
@@ -64,37 +71,41 @@
 // UNUSED-NEXT: warning: argument unused during compilation: '-fvisibility-externs-nodllstorageclass=protected'
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-nodllstorageclass=protected \
 // REDEFINE:     -fvisibility-externs-dllimport=hidden \
 // REDEFINE:     -Werror \
 // REDEFINE:     %s -o - 2>&1 | \
-// REDEFINE:   FileCheck %s -check-prefix=SOME \
+// REDEFINE:   FileCheck %s -check-prefixes=SOME,%{prefix} \
 // REDEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = SOME-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = SOME-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -fvisibility-nodllstorageclass=protected \
 // REDEFINE:     -fvisibility-externs-dllimport=hidden \
 // REDEFINE:     -Werror \
 // REDEFINE:     %s -o - 2>&1 | \
-// REDEFINE:   FileCheck %s -check-prefix=SOME \
+// REDEFINE:   FileCheck %s -check-prefixes=SOME,%{prefix} \
 // REDEFINE:     --implicit-check-not=-fvisibility-from-dllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE: %{prefix} = SOME-PS4
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
+// REDEFINE: %{prefix} = SOME-PS5
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
@@ -102,10 +113,11 @@
 // SOME-SAME: "-fvisibility-dllexport=protected"
 // SOME-SAME: "-fvisibility-nodllstorageclass=protected"
 // SOME-SAME: "-fvisibility-externs-dllimport=hidden"
-// SOME-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// SOME-PS4-SAME: "-fvisibility-externs-nodllstorageclass=default"
+// SOME-PS5-SAME: "-fvisibility-externs-nodllstorageclass=keep"
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-dllexport=default \
 // REDEFINE:     -fvisibility-dllexport=hidden \
 // REDEFINE:     -fvisibility-nodllstorageclass=default \
@@ -121,14 +133,15 @@
 // REDEFINE:     --implicit-check-not=-fvisibility-dllexport \
 // REDEFINE:     --implicit-check-not=-fvisibility-nodllstorageclass \
 // REDEFINE:     --implicit-check-not=-fvisibility-externs-dllimport \
-// REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass
+// REDEFINE:     --implicit-check-not=-fvisibility-externs-nodllstorageclass \
+// REDEFINE:     --implicit-check-not="warning: argument unused"
 // REDEFINE: %{triple} = x86_64-scei-ps4
 // RUN: %{run}
 // REDEFINE: %{triple} = x86_64-sie-ps5
 // RUN: %{run}
 
 // REDEFINE: %{run} = \
-// REDEFINE: %clang -### -target x86_64-scei-ps4 \
+// REDEFINE: %clang -### -target %{triple} \
 // REDEFINE:     -fvisibility-from-dllstorageclass \
 // REDEFINE:     -fvisibility-dllexport=default \
 // REDEFINE:     -fvisibility-dllexport=hidden \
diff --git a/clang/test/Driver/ps4-visibility.cl b/clang/test/Driver/ps4-visibility.cl
new file mode 100644
index 0000000000000..a0ed7c71f1f0e
--- /dev/null
+++ b/clang/test/Driver/ps4-visibility.cl
@@ -0,0 +1,32 @@
+/// Check PS4 specific interactions between visibility options.
+/// Detailed testing of -fvisibility-from-dllstorageclass is covered elsewhere.
+
+/// Check defaults.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// DEFAULT-DAG: "-fvisibility-from-dllstorageclass"
+// DEFAULT-DAG: "-fvisibility-dllexport=protected"
+// DEFAULT-DAG: "-fvisibility-nodllstorageclass=hidden"
+// DEFAULT-DAG: "-fvisibility-externs-dllimport=default"
+// DEFAULT-DAG: "-fvisibility-externs-nodllstorageclass=default"
+
+/// Check that -fvisibility-from-dllstorageclass is added in the presence of -fvisibility=.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fvisibility=default  %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=DEFAULT,VISEQUALS %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// VISEQUALS-DAG: "-fvisibility=default"
+
+/// Check that -fvisibility-from-dllstorageclass is added in the presence of -fvisibility-ms-compat.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fvisibility-ms-compat %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=DEFAULT,MSCOMPT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// MSCOMPT-DAG: "-fvisibility=hidden"
+// MSCOMPT-DAG: "-ftype-visibility=default"
+
+/// -fvisibility-from-dllstorageclass added explicitly.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fvisibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+
+/// -fvisibility-from-dllstorageclass disabled explicitly.
+// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fno-visibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=NOVISFROM %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// NOVISFROM-NOT: "-fvisibility-from-dllstorageclass"
+
diff --git a/clang/test/Driver/ps5-visibility.cl b/clang/test/Driver/ps5-visibility.cl
new file mode 100644
index 0000000000000..ad144057be63a
--- /dev/null
+++ b/clang/test/Driver/ps5-visibility.cl
@@ -0,0 +1,33 @@
+/// Check PS5 specific interactions between visibility options.
+/// Detailed testing of -fvisibility-from-dllstorageclass is covered elsewhere.
+
+/// Check defaults.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// VDEFAULT-DAG:     "-fvisibility=hidden"
+// VGND_DEFAULT-DAG: "-fvisibility-global-new-delete=source"
+// DEFAULT-DAG:      "-fvisibility-from-dllstorageclass"
+// DEFAULT-DAG:      "-fvisibility-dllexport=protected"
+// DEFAULT-DAG:      "-fvisibility-nodllstorageclass=keep"
+// DEFAULT-DAG:      "-fvisibility-externs-dllimport=default"
+// DEFAULT-DAG:      "-fvisibility-externs-nodllstorageclass=keep"
+
+/// -fvisibility= specified explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fvisibility=protected %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VPROTECTED,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// VPROTECTED-DAG: "-fvisibility=protected"
+
+/// -fvisibility-ms-compat added explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fvisibility-ms-compat %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=MSCOMPT,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// MSCOMPT-DAG: "-fvisibility=hidden"
+// MSCOMPT-DAG: "-ftype-visibility=default"
+
+/// -fvisibility-from-dllstorageclass added explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fvisibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,DEFAULT %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+
+/// -fvisibility-from-dllstorageclass disabled explicitly.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fno-visibility-from-dllstorageclass %s 2>&1 | \
+// RUN:   FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,NOVISFROM %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
+// NOVISFROM-NOT: "-fvisibility-from-dllstorageclass"

bd1976bris
bd1976bris commented May 14, 2024
View reviewed changes
clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -376,24 +382,32 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
else
CC1Args.push_back("-fvisibility-dllexport=protected");

// For PS4 we override the visibilty of globals definitions without
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note for reviewer: This comment is new and replaces a comment in our downstream code that wasn't suitable for upstream as it referenced internal tracking numbers.

bd1976bris
bd1976bris commented May 14, 2024
View reviewed changes
clang/lib/Driver/ToolChains/PS4CPU.cpp

if (DriverArgs.hasArg(options::OPT_fvisibility_externs_dllimport_EQ))
DriverArgs.AddLastArg(CC1Args,
options::OPT_fvisibility_externs_dllimport_EQ);
else
CC1Args.push_back("-fvisibility-externs-dllimport=default");

// For PS4 we override the visibilty of external globals without
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note for reviewer: This comment is new and replaces a comment in our downstream code that wasn't suitable for upstream as it referenced internal tracking numbers.

pogo59
pogo59 approved these changes May 17, 2024
View reviewed changes
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. I noted a couple of redundant checks, but if you think it's better for them to be explicit, I'm okay with keeping them.

clang/test/Driver/ps4-visibility.cl
/// -fvisibility-from-dllstorageclass disabled explicitly.
// RUN: %clang -### -target x86_64-scei-ps4 -x cl -c -emit-llvm -fno-visibility-from-dllstorageclass %s 2>&1 | \
// RUN: FileCheck -check-prefixes=NOVISFROM %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
// NOVISFROM-NOT: "-fvisibility-from-dllstorageclass"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already implicitly checked by --implicit-check-not=fvisibility

clang/test/Driver/ps5-visibility.cl
/// -fvisibility-from-dllstorageclass disabled explicitly.
// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm -fno-visibility-from-dllstorageclass %s 2>&1 | \
// RUN: FileCheck -check-prefixes=VDEFAULT,VGND_DEFAULT,NOVISFROM %s --implicit-check-not=fvisibility --implicit-check-not=ftype-visibility --implicit-check-not=dllstorageclass
// NOVISFROM-NOT: "-fvisibility-from-dllstorageclass"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already excluded by --implicit-check-not=fvsibility

@bd1976bris
Copy link
Collaborator Author

LGTM. I noted a couple of redundant checks, but if you think it's better for them to be explicit, I'm okay with keeping them.

Thanks. I think in this case it makes it more obvious what is primarily been checked in these cases to have the check explicit.

@bd1976bris bd1976bris merged commit 384bf54 into llvm:main May 20, 2024
6 of 7 checks passed
@PeddleSpam PeddleSpam mentioned this pull request May 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pogo59 pogo59 pogo59 approved these changes

Assignees
No one assigned
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bd1976bris @llvmbot @pogo59