[Sema] Add `-fvisibility-global-new-delete=` option #75364

bd1976bris · 2023-12-13T17:46:55Z

By default the implicitly declared replaceable global new and delete
operators are given a default visibility attribute. Previous work, see:
https://reviews.llvm.org/D53787, added
-fvisibility-global-new-delete-hidden to change this to a hidden
visibility attribute.

This change adds -fvisibility-global-new-delete= which controls
whether (or not) to add an implicit visibility attribute to the implicit
declarations for these functions, and what visibility that attribute
will specify. The option takes 4 values: force-hidden,
force-protected, force-default and source. Option values
force-hidden, force-protected and force-default assign hidden,
protected, and default visibilities respectively; the use of the term
force in the value names is designed to imply to a user that the semantics
of this option differ significantly from -fvisibility=. An option
value of source implies that no implicit attribute is added; without
the attribute the replaceable global new and delete operators behave
normally (like other functions) with respect to visibility attributes,
pragmas and options.

The motivation for the source value is to facilitate users who intend
to replace these functions either for a single linkage unit or a limited
set of linkage units. -fvisibility-global-new-delete=source can be
applied globally to the compilations in a build where the existing
-fvisibility-global-new-delete-hidden cannot, as it conflicts with a
common pattern where these functions are dynamically imported.

The existing -fvisibility-global-new-delete-hidden is now a deprecated
spelling of -fvisibility-global-new-delete=force-hidden

The command line help for these options is rendered as:

  -fvisibility-global-new-delete-hidden
                          Give global C++ operator new and delete declarations hidden visibility

  -fvisibility-global-new-delete=<value>
                          The visibility for global C++ operator new and delete declarations. If
                          'source' is specified the visibility is not adjusted

A release note has been added for these changes.

-fvisibility-global-new-delete=source will be set by default for PS5.
PS5 users that want the normal toolchain behaviour will be able to
supply -fvisibility-global-new-delete=force-default.

llvmbot · 2023-12-13T17:47:22Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: bd1976bris (bd1976bris)

Changes

By default the implicitly declared replaceable global new and delete operators are given a default visibility attribute. Previous work, see: https://reviews.llvm.org/D53787, added -fvisibility-global-new-delete-hidden to change this to a hidden visibility attribute.

This change adds: -fno/-fvisibility-global-new-delete which controls whether or not to add a visibility attribute to the implicit declarations for these functions. Without the attribute the replaceable global new and delete operators behave normally (like other functions) with respect to visibility attributes, pragmas and options.

The command line help for these options is rendered as:

  -fvisibility-global-new-delete
                          Add a visibility attribute to the implicit
                          global C++ operator new and delete declarations

  -fno-visibility-global-new-delete
                          Do not add a visibility attribute to the implicit
                          global C++ operator new and delete declarations

The motivation is to allow users to specify -fno-visibility-global-new-delete when they intend to replace these functions either for a single linkage unit or set of linkage units.

-fno-visibility-global-new-delete can be applied globally to the compilations in a build where -fvisibility-global-new-delete-hidden cannot; as it conflicts with a common pattern where these functions are dynamically imported.

-fno-visibility-global-new-delete makes sense as the default for PS5. Users that want the normal toolchain behaviour will be able to supply -fvisibility-global-new-delete.

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

7 Files Affected:

(modified) clang/include/clang/Basic/LangOptions.def (+2-1)
(modified) clang/include/clang/Driver/Options.td (+6)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+12)
(modified) clang/lib/Driver/ToolChains/PS4CPU.cpp (+6)
(modified) clang/lib/Sema/SemaExprCXX.cpp (+5-4)
(added) clang/test/CodeGenCXX/visibility-global-new-delete.cpp (+13)
(added) clang/test/Driver/visibility-global-new-delete.cl (+47)

diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index c3d5399905a3fd..1471fc11e11663 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -306,7 +306,8 @@ BENIGN_LANGOPT(IgnoreXCOFFVisibility, 1, 0, "All the visibility attributes that
 BENIGN_LANGOPT(VisibilityInlinesHiddenStaticLocalVar, 1, 0,
                "hidden visibility for static local variables in inline C++ "
                "methods when -fvisibility-inlines hidden is enabled")
-LANGOPT(GlobalAllocationFunctionVisibilityHidden , 1, 0, "hidden visibility for global operator new and delete declaration")
+LANGOPT(GlobalAllocationFunctionVisibility, 1, 1, "add a visibility attribute to the implicit global operator new and delete declarations")
+LANGOPT(GlobalAllocationFunctionVisibilityHidden, 1, 0, "hidden visibility for global operator new and delete declarations")
 LANGOPT(NewInfallible , 1, 0, "Treats throwing global C++ operator new as always returning valid memory (annotates with __attribute__((returns_nonnull)) and throw()). This is detectable in source.")
 BENIGN_LANGOPT(ParseUnknownAnytype, 1, 0, "__unknown_anytype")
 BENIGN_LANGOPT(DebuggerSupport , 1, 0, "debugger support")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index db2190318c931a..a9f43b18df6fbf 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3863,6 +3863,12 @@ defm visibility_inlines_hidden_static_local_var : BoolFOption<"visibility-inline
 def fvisibility_ms_compat : Flag<["-"], "fvisibility-ms-compat">, Group<f_Group>,
   HelpText<"Give global types 'default' visibility and global functions and "
            "variables 'hidden' visibility by default">;
+defm visibility_global_new_delete : BoolFOption<"visibility-global-new-delete",
+  LangOpts<"GlobalAllocationFunctionVisibility">, DefaultTrue,
+  PosFlag<SetTrue, [], [ClangOption], "Add">,
+  NegFlag<SetFalse, [], [ClangOption], "Do not add">,
+  BothFlags<[], [ClangOption, CC1Option],
+          " a visibility attribute to the implicit global C++ operator new and delete declarations">>;
 def fvisibility_global_new_delete_hidden : Flag<["-"], "fvisibility-global-new-delete-hidden">, Group<f_Group>,
   HelpText<"Give global C++ operator new and delete declarations hidden visibility">,
   Visibility<[ClangOption, CC1Option]>,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index f02f7c841b91f0..979e2a0e83be37 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6204,7 +6204,19 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
 
   Args.AddLastArg(CmdArgs, options::OPT_fvisibility_inlines_hidden_static_local_var,
                            options::OPT_fno_visibility_inlines_hidden_static_local_var);
+  Args.AddLastArg(CmdArgs, options::OPT_fvisibility_global_new_delete,
+                           options::OPT_fno_visibility_global_new_delete);
   Args.AddLastArg(CmdArgs, options::OPT_fvisibility_global_new_delete_hidden);
+  // Error for incompatible global new and delete directives.
+  const Arg *N = Args.getLastArg(options::OPT_fvisibility_global_new_delete,
+                                 options::OPT_fno_visibility_global_new_delete);
+  if (N &&
+      N->getOption().matches(options::OPT_fno_visibility_global_new_delete)) {
+    if (Arg *H =
+            Args.getLastArg(options::OPT_fvisibility_global_new_delete_hidden))
+      D.Diag(diag::err_drv_incompatible_options)
+          << N->getSpelling() << H->getSpelling();
+  }
   Args.AddLastArg(CmdArgs, options::OPT_ftlsmodel_EQ);
 
   if (Args.hasFlag(options::OPT_fnew_infallible,
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 5fd82d1da19926..a535d5fb2a7200 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -359,6 +359,12 @@ void toolchains::PS4PS5Base::addClangTargetOptions(
 
   CC1Args.push_back("-fno-use-init-array");
 
+  // Default to -fno-visibility-global-new-delete for PS5.
+  if (getTriple().isPS5() &&
+      !DriverArgs.hasArg(options::OPT_fvisibility_global_new_delete,
+                         options::OPT_fno_visibility_global_new_delete,
+                         options::OPT_fvisibility_global_new_delete_hidden))
+    CC1Args.push_back("-fno-visibility-global-new-delete");
   const Arg *A =
       DriverArgs.getLastArg(options::OPT_fvisibility_from_dllstorageclass,
                             options::OPT_fno_visibility_from_dllstorageclass);
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 081b568762ae22..29480a1e0c0346 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3222,10 +3222,11 @@ void Sema::DeclareGlobalAllocationFunction(DeclarationName Name,
       Alloc->setLocalOwningModule(TheGlobalModuleFragment);
     }
 
-    Alloc->addAttr(VisibilityAttr::CreateImplicit(
-        Context, LangOpts.GlobalAllocationFunctionVisibilityHidden
-                     ? VisibilityAttr::Hidden
-                     : VisibilityAttr::Default));
+    if (LangOpts.GlobalAllocationFunctionVisibility)
+      Alloc->addAttr(VisibilityAttr::CreateImplicit(
+          Context, LangOpts.GlobalAllocationFunctionVisibilityHidden
+                       ? VisibilityAttr::Hidden
+                       : VisibilityAttr::Default));
 
     llvm::SmallVector<ParmVarDecl *, 3> ParamDecls;
     for (QualType T : Params) {
diff --git a/clang/test/CodeGenCXX/visibility-global-new-delete.cpp b/clang/test/CodeGenCXX/visibility-global-new-delete.cpp
new file mode 100644
index 00000000000000..b29a5f01e9a647
--- /dev/null
+++ b/clang/test/CodeGenCXX/visibility-global-new-delete.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -std=c++11 -triple x86_64-unknown-unknown -fvisibility=hidden -emit-llvm -o - | FileCheck %s -DLINKAGE=dso_local
+// RUN: %clang_cc1 %s -std=c++11 -triple x86_64-unknown-unknown -fvisibility=hidden -fvisibility-global-new-delete -emit-llvm -o - | FileCheck %s -DLINKAGE=dso_local
+// RUN: %clang_cc1 %s -std=c++11 -triple x86_64-unknown-unknown -fvisibility=hidden -fno-visibility-global-new-delete -emit-llvm -o - | FileCheck %s -DLINKAGE=hidden
+// RUN: %clang_cc1 %s -std=c++11 -triple x86_64-unknown-unknown -fvisibility=hidden -fvisibility-global-new-delete-hidden -emit-llvm -o - | FileCheck %s -DLINKAGE=hidden
+
+namespace std {
+  typedef __typeof__(sizeof(0)) size_t;
+  struct nothrow_t {};
+}
+
+// Definition which inherits visibility from the implicit compiler generated declaration.
+void operator delete(void*) throw() {}
+// CHECK: define [[LINKAGE]]  void @_ZdlPv
diff --git a/clang/test/Driver/visibility-global-new-delete.cl b/clang/test/Driver/visibility-global-new-delete.cl
new file mode 100644
index 00000000000000..08e3a812887452
--- /dev/null
+++ b/clang/test/Driver/visibility-global-new-delete.cl
@@ -0,0 +1,47 @@
+/// Check driver handling for "-fvisibility-global-new-delete-hidden" and "-fvisibility-global-new-delete".
+
+/// These options are not added by default.
+// RUN: %clang -### -target x86_64-unknown-unknown -x cl -c -emit-llvm %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=DEFAULTS %s
+// DEFAULTS-NOT: "-fvisibility-global-new-delete"
+// DEFAULTS-NOT: "-fno-visibility-global-new-delete"
+// DEFAULTS-NOT: "-fvisibility-global-new-delete-hidden"
+
+// DEFINE: %{implicit-check-nots} = --implicit-check-not=-fvisibility-global-new-delete --implicit-check-not=-fno-visibility-global-new-delete
+
+/// "-fno-visibility-global-new-delete" added by default for PS5.
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=PS5 %s 
+// PS5: "-fno-visibility-global-new-delete"
+
+/// -fvisibility-global-new-delete-hidden added explicitly.
+// RUN: %clang -### -target x86_64-unknown-unknown -x cl -c -emit-llvm \
+// RUN:   -fvisibility-global-new-delete-hidden %s 2>&1 | FileCheck -check-prefixes=HIDDEN %s %{implicit-check-nots}
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm \
+// RUN:   -fvisibility-global-new-delete-hidden %s 2>&1 | FileCheck -check-prefixes=HIDDEN %s %{implicit-check-nots}
+// HIDDEN-DAG: "-fvisibility-global-new-delete-hidden"
+
+/// -fvisibility-global-new-delete added explicitly.
+// RUN: %clang -### -target x86_64-unknown-unknown -x cl -c -emit-llvm \
+// RUN:   -fvisibility-global-new-delete %s 2>&1 | FileCheck -check-prefixes=VGND %s %{implicit-check-nots}
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm \
+// RUN:   -fvisibility-global-new-delete %s 2>&1 | FileCheck -check-prefixes=VGND %s %{implicit-check-nots}
+// VGND-DAG: "-fvisibility-global-new-delete"
+
+/// -fno-visibility-global-new-delete added explicitly.
+// RUN: %clang -### -target x86_64-unknown-unknown -x cl -c -emit-llvm \
+// RUN:   -fno-visibility-global-new-delete %s 2>&1 | FileCheck -check-prefixes=NO_VGND %s %{implicit-check-nots}
+// RUN: %clang -### -target x86_64-sie-ps5 -x cl -c -emit-llvm \
+// RUN:   -fno-visibility-global-new-delete %s 2>&1 | FileCheck -check-prefixes=NO_VGND %s %{implicit-check-nots}
+// NO_VGND-DAG: "-fno-visibility-global-new-delete"
+
+/// No error if both -fvisibility-global-new-delete and -fvisibility-global-new-delete-hidden specified.
+// RUN: %clang -### -target x86_64-unknown-unknown -x cl -c -emit-llvm \ 
+// RUN:   -fvisibility-global-new-delete-hidden -fvisibility-global-new-delete %s 2>&1 | \
+// RUN:     FileCheck -check-prefixes=VGND,HIDDEN %s %{implicit-check-nots}
+
+/// Error if both -fno-visibility-global-new-delete and -fvisibility-global-new-delete-hidden specified.
+// RUN: not %clang -### -target x86_64-unknown-unknown -x cl -c -emit-llvm \ 
+// RUN:   -fvisibility-global-new-delete-hidden -fvisibility-global-new-delete -fno-visibility-global-new-delete %s 2>&1 | \
+// RUN:     FileCheck -check-prefixes=INCOMPAT %s
+// INCOMPAT: clang: error: the combination of '-fno-visibility-global-new-delete' and '-fvisibility-global-new-delete-hidden' is incompatible

bd1976bris · 2023-12-13T17:48:45Z

See #74629 for more on visibility support in PlayStation.

github-actions · 2023-12-13T17:49:26Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bd1976bris · 2023-12-18T16:35:00Z

Noting previous discussions w.r.t. -fvisibility-global-new-delete-hidden on https://reviews.llvm.org/D133266. I have added reviewers from that phab review in case they are interested in this.

pogo59 · 2023-12-18T18:52:05Z

From a UI perspective, "add a visibility attribute" doesn't seem all that descriptive. What visibility does it add? Therefore, I suggest -fvisibility-global-new-delete[=<visibility>] so the user can ask for what they want, and -fvisibility-global-new-delete-hidden equals -fvisibility-global-new-delete=hidden. The help text can indicate that the default is default.

I'm unclear what the -fno variant actually turns into. There's always a visibility, ultimately, right? So wouldn't that be equivalent to the keep option in #74629 ? Which suggests not having the -fno variant here, and allowing =keep instead.

(If the universe would implode given a visibility other than default or hidden for global new/delete, then constrain the argument to the option to be only default/hidden/keep.)

I hope this makes sense...

bd1976bris · 2023-12-18T20:30:24Z

From a UI perspective, "add a visibility attribute" doesn't seem all that descriptive. What visibility does it add? Therefore, I suggest -fvisibility-global-new-delete[=<visibility>] so the user can ask for what they want, and -fvisibility-global-new-delete-hidden equals -fvisibility-global-new-delete=hidden. The help text can indicate that the default is default.

Thanks Paul. It's a nice suggestion for the UI. I didn't go down this route because the name -fvisibility-global-new-delete[=<visibility>] seems to imply (at least to me) that the option should behave like -fvisibility[=<visibility>] . However, the latter only applies to definitions and not to declarations whereas the former would apply to both.

I'm unclear what the -fno variant actually turns into. There's always a visibility, ultimately, right? So wouldn't that be equivalent to the keep option in #74629 ? Which suggests not having the -fno variant here, and allowing =keep instead.

There's a difference! The keep option in #74629 is applying to the visibility of the IR globals. However, the replaceable new and delete declarations are given a visibility during SEMA (a VisibilityAttr) - as if those declarations were parsed with an explicit visibility attribute. By default that VisibilityAttr has default visibility but -fvisibility-global-new-delete-hidden changes it to hidden visibility. The proposed -fno-visibility-global-new-delete instructs the compiler not to add a VisibilityAttr to these special globals. Therefore, these declarations behave like any other declarations w.r.t. visibility options/attributes and pragmas.

petrhosek · 2023-12-19T09:22:48Z

I don't think it's very clear from -f[no-]visibility-global-new-delete what the option does. How about -f[no-]visibility-attribute-global-new-delete?

frobtech · 2023-12-20T03:23:11Z

The meaning that we want to make clear is that this toggles the special behavior of forcing the visibility of these symbols despite all the other mechanisms that usually control visibility for all other symbols. So perhaps -fforced-global-new-delete-visibility (or even -fforced-global-new-delete-visibility=default) describes the status quo ante, and -fno-forced-global-new-delete-visibility describes the new opt-in wherein these symbols are not treated differently than others. (The =... variant of the positive form would permit forcing to a different visibility than default, though I'm not sure there's a real need for that capability since in -fno-forced-... state the usual pragmas et al can arrange for that.)

bd1976bris · 2023-12-20T12:42:17Z

Thanks for the responses.

The meaning that we want to make clear is that this toggles the special behavior of forcing the visibility of these symbols despite all the other mechanisms that usually control visibility for all other symbols.

Agreed. I'm in favour of your suggestion of -f[no-]forced-global-new-delete-visibility. Including "forced" in the option name makes it clear that something unusual is going on here and hints that this is going to take precedence over other sources of visibility. I did consider that it might be better to rearrange your suggested option name a bit to -f[no-]visibility-global-new-delete-forced to group with other visibility options that start with -fvisibility, but -f[no-]forced-global-new-delete-visibility is much clearer. @petrhosek I did consider -f[no-]visibility-attribute-global-new-delete but that naming is possibly tied a bit too strongly to the current implementation in the complier - so I think I like the forced suggestion better?

(The =... variant of the positive form would permit forcing to a different visibility than default, though I'm not sure there's a real need for that capability since in -fno-forced-... state the usual pragmas et al can arrange for that.)

Indeed and the use of the usual pragmas etc is preferable, IMO, as users are already familiar with those - as opposed to the effects of an unusual/niche option.

bd1976bris · 2023-12-21T22:38:13Z

I have updated the patch to use -f[no-]forced-global-new-delete-visibility as suggested.

pogo59 · 2024-01-03T17:43:58Z

Actually I kind of prefer all these options to have -fvisibility- as a prefix. Even if it doesn't read quite naturally, it strongly implies that the options are related (which is true) and any lexically sorted list of options will naturally group them together. WDYT @frobtech ?

frobtech · 2024-01-03T18:19:05Z

Actually I kind of prefer all these options to have -fvisibility- as a prefix. Even if it doesn't read quite naturally, it strongly implies that the options are related (which is true) and any lexically sorted list of options will naturally group them together. WDYT @frobtech ?

I don't have a strong opinion about the exact spelling. I just recommend that it be as unambiguous as possible, and the use of "forced" in the name seems like a good way to help with that.

zygoloid · 2024-01-03T19:35:25Z

How about instead using -fvisibility-global-new-delete=<value>? We want a mutually-exclusive set of options here, and we know we want more than two of them, and a flag with an argument value is the way we usually express that.

Perhaps we can use values of force-hidden, force-default to force the visibility to specific things, and something like unconstrained or source to say "use the normal visibility calculation"? (It's a bit unfortunate that we can't use default for that, given that it means something else...) -fvisibility-global-new-delete-hidden could then be converted into a deprecated synonym for -fvisibility-global-new-delete=force-hidden.

bd1976bris · 2024-01-03T20:31:55Z

@zygoloid - yes, the choice of "default" for the name of one of the visibility values in ELF is annoying :) Thanks for the comments all. I feel that what we might need is a bit of a benevolent dictator here. Does anyone object to Richard's -fvisibility-global-new-delete=<value> proposal - I know that -fvisibility-global-new-delete=force-hidden is in use so deprecating might be painful for someone. Thinking about it more critically, I think I was a bit put off by what might be involved in deprecating -fvisibility-global-new-delete=force-hidden and that might have coloured my view as to what to implement for this.

zygoloid · 2024-01-03T21:29:38Z

To be clear: I didn't mean to suggest that -fvisibility-global-new-delete=force-hidden itself would be deprecated. I think supporting it would have low cost and it may be exactly what is desired in some cases, and the previous spelling has quite a few uses. Instead, what I'd suggest is that we add the more-general mechanism of -fvisibility-global-new-delete=<value> and make the existing -fvisibility-global-new-delete-hidden equivalent to -fvisibility-global-new-delete=force-hidden, except that the former spelling is deprecated and the latter spelling is not.

bd1976bris · 2024-01-19T09:31:41Z

Given that there have been no objections I have modified this to match @zygoloid's suggestion of -fvisibility-global-new-delete=<value>. Please take another look.

pogo59

I'm okay with the PS5-specific bits. Other people should have a chance to chime in so I won't mark it approved.

clang/lib/Driver/ToolChains/PS4CPU.cpp

clang/test/Driver/visibility-global-new-delete.cl

MaskRay · 2024-01-19T22:21:52Z

clang/test/CodeGenCXX/visibility-global-new-delete.cpp

+
+// Definition which inherits visibility from the implicit compiler generated declaration.
+void operator delete(void*) throw() {}
+// CHECK: define [[LINKAGE]] void @_ZdlPv


Add a test for operator new?

-fvisibility-global-new-delete-hidden, from https://reviews.llvm.org/D53787, does not have a test. Thanks for adding the test

I don't think there is much value in adding operator new as these declarations are all handled in the same way. I think minimal testing here is better.

clang/include/clang/Driver/Options.td

MaskRay

Consider adding a note to clang/docs/ReleaseNotes.rst. With a few nits this LGTM.

By default the implicitly declared replaceable global new and delete operators are given a default visibility attribute. Previous work, see: https://reviews.llvm.org/D53787, added `-fvisibility-global-new-delete-hidden` to change this to a hidden visibility attribute. This change adds `-fvisibility-global-new-delete=` which controls whether (or not) to add an implicit visibility attribute to the implicit declarations for these functions, and what visibility that attribute will specify. The option takes 4 values: `force-hidden`, `force-protected`, `force-default` and `source`. Option values `force-hidden`, `force-protected` and `force-default` assign hidden, protected, and default visibilities respectively; the use of the term force in the value names is designed to imply to a user that the semantics of this option differ significantly from `-fvisibility=`. An option value of `source` implies that no implicit attribute is added; without the attribute the replaceable global new and delete operators behave normally (like other functions) with respect to visibility attributes, pragmas and options. The motivation for the `source` value is to facilitate users who intend to replace these functions either for a single linkage unit or a limited set of linkage units. `-fvisibility-global-new-delete=source` can be applied globally to the compilations in a build where the existing `-fvisibility-global-new-delete-hidden` cannot, as it conflicts with a common pattern where these functions are dynamically imported. The existing `-fvisibility-global-new-delete-hidden` is now a deprecated spelling of `-fvisibility-global-new-delete=force-hidden` A release note has been added for these changes. `-fvisibility-global-new-delete=source` will be set by default for PS5. PS5 users that want the normal toolchain behaviour will be able to supply `-fvisibility-global-new-delete=force-default`.

bd1976bris · 2024-01-21T17:21:17Z

I rebased the changes onto the latest main to resolve the merge conflicts - these were just textual not semantic.

MaskRay · 2024-01-21T22:47:19Z

I rebased the changes onto the latest main to resolve the merge conflicts - these were just textual not semantic.

You may like getcord/spr :)
https://maskray.me/blog/2023-09-09-reflections-on-llvm-switch-to-github-pull-requests#my-workflow

bd1976bris · 2024-01-22T12:43:54Z

I rebased the changes onto the latest main to resolve the merge conflicts - these were just textual not semantic.

You may like getcord/spr :) https://maskray.me/blog/2023-09-09-reflections-on-llvm-switch-to-github-pull-requests#my-workflow

Thanks for the link to the write up. Very much appreciated. I need to improve my workflow now that everything is via github. I don't particularly like GUI interfaces... you'll notice that I managed to repeat the header line in the commit comment when committing through the GUI: 27ce26b 🥇

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" labels Dec 13, 2023

bd1976bris requested review from uweigand, MaskRay and rnk December 13, 2023 17:48

bd1976bris requested review from frobtech, zygoloid, petrhosek, smeenai, compnerd, justincady, efriedma-quic, rjmccall, aaronsm, sstefan1 and mstorsjo December 13, 2023 17:50

bd1976bris changed the title ~~[Sema] Provide -fno-/-fvisibility-global-new-delete option~~ [Sema] Provide -fvisibility-global-new-delete= option Jan 19, 2024

pogo59 reviewed Jan 19, 2024

View reviewed changes

clang/lib/Driver/ToolChains/PS4CPU.cpp Outdated Show resolved Hide resolved

MaskRay reviewed Jan 19, 2024

View reviewed changes

clang/test/Driver/visibility-global-new-delete.cl Outdated Show resolved Hide resolved

MaskRay reviewed Jan 19, 2024

View reviewed changes

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

MaskRay approved these changes Jan 19, 2024

View reviewed changes

bd1976bris force-pushed the fvisibility-global-new-delete branch from 708140b to dc60cb2 Compare January 21, 2024 17:18

bd1976bris changed the title ~~[Sema] Provide -fvisibility-global-new-delete= option~~ [Sema] Add -fvisibility-global-new-delete= option Jan 21, 2024

MaskRay approved these changes Jan 21, 2024

View reviewed changes

bd1976bris closed this Jan 22, 2024

bd1976bris reopened this Jan 22, 2024

bd1976bris merged commit 27ce26b into llvm:main Jan 22, 2024
9 checks passed

bd1976bris deleted the fvisibility-global-new-delete branch January 22, 2024 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sema] Add `-fvisibility-global-new-delete=` option #75364

[Sema] Add `-fvisibility-global-new-delete=` option #75364

bd1976bris commented Dec 13, 2023 •

edited

llvmbot commented Dec 13, 2023 •

edited

bd1976bris commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 •

edited

bd1976bris commented Dec 18, 2023

pogo59 commented Dec 18, 2023

bd1976bris commented Dec 18, 2023 •

edited

petrhosek commented Dec 19, 2023

frobtech commented Dec 20, 2023

bd1976bris commented Dec 20, 2023

bd1976bris commented Dec 21, 2023

pogo59 commented Jan 3, 2024

frobtech commented Jan 3, 2024

zygoloid commented Jan 3, 2024

bd1976bris commented Jan 3, 2024

zygoloid commented Jan 3, 2024

bd1976bris commented Jan 19, 2024

pogo59 left a comment

MaskRay Jan 19, 2024

bd1976bris Jan 19, 2024

MaskRay left a comment

bd1976bris commented Jan 21, 2024

MaskRay commented Jan 21, 2024

bd1976bris commented Jan 22, 2024 •

edited

[Sema] Add -fvisibility-global-new-delete= option #75364

[Sema] Add -fvisibility-global-new-delete= option #75364

Conversation

bd1976bris commented Dec 13, 2023 • edited

llvmbot commented Dec 13, 2023 • edited

bd1976bris commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 • edited

bd1976bris commented Dec 18, 2023

pogo59 commented Dec 18, 2023

bd1976bris commented Dec 18, 2023 • edited

petrhosek commented Dec 19, 2023

frobtech commented Dec 20, 2023

bd1976bris commented Dec 20, 2023

bd1976bris commented Dec 21, 2023

pogo59 commented Jan 3, 2024

frobtech commented Jan 3, 2024

zygoloid commented Jan 3, 2024

bd1976bris commented Jan 3, 2024

zygoloid commented Jan 3, 2024

bd1976bris commented Jan 19, 2024

pogo59 left a comment

Choose a reason for hiding this comment

MaskRay Jan 19, 2024

Choose a reason for hiding this comment

bd1976bris Jan 19, 2024

Choose a reason for hiding this comment

MaskRay left a comment

Choose a reason for hiding this comment

bd1976bris commented Jan 21, 2024

MaskRay commented Jan 21, 2024

bd1976bris commented Jan 22, 2024 • edited

[Sema] Add `-fvisibility-global-new-delete=` option #75364

[Sema] Add `-fvisibility-global-new-delete=` option #75364

bd1976bris commented Dec 13, 2023 •

edited

llvmbot commented Dec 13, 2023 •

edited

github-actions bot commented Dec 13, 2023 •

edited

bd1976bris commented Dec 18, 2023 •

edited

bd1976bris commented Jan 22, 2024 •

edited