[Sema] Provide -fvisibility-global-new-delete-hidden option

[phosek]

Summary

When the global new and delete operators aren't declared, Clang
provides and implicit declaration, but this declaration currently
always uses the default visibility. This is a problem when the
C++ library itself is being built with non-default visibility because
the implicit declaration will force the new and delete operators to
have the default visibility unlike the rest of the library.

The existing workaround is to use assembly to enforce the visiblity:
https://fuchsia.googlesource.com/zircon/+/master/system/ulib/zxcpp/new.cpp#108
but that solution is not always available, e.g. in the case of of
libFuzzer which is using an internal version of libc++ that's also built
with -fvisibility=hidden where the existing behavior is causing issues.

This change introduces a new option -fvisibility-global-new-delete-hidden
which makes the implicit declaration of the global new and delete
operators hidden.

[rnk]

This seems pretty inconsistent with how -fvisibility=hidden behaves with normal, user written declarations. Consider this code:

void foo();
void bar() { foo(); }

We get this IR:

$ clang -S -emit-llvm --target=x86_64-linux t.c -o - -fvisibility=hidden
...
define hidden void @bar() #0 {
entry:
  call void @foo()
  ret void
}
declare dso_local void @foo() #1

Basically, declarations are never marked hidden, only definitions are. This is because system headers are not explicitly marked with default visibility annotations, and you still want to be able to call libc from code compiled with -fvisibility=hidden.

I'm... kind of surprised we have code to explicitly mark these declarations with default visibility attributes.

[rsmith]

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

[phosek]

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

That sounds reasonable, do you have any suggestion for the name of such flag?

[rsmith]

In D53787#1279913, @phosek wrote:

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

That sounds reasonable, do you have any suggestion for the name of such flag?

-fglobal-new-delete-visibility= is the first thing that springs to mind, but maybe there's something better?

[mcgrathr]

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

I don't really understand how these functions are different from other functions. The language standards don't have anything to say about ELF visibility. What you say about "whole-program operation" is true of any global symbol. When we use visibility switches or annotations it's because we want to change how global symbols behave. I don't understand the rationale for treating these particular functions differently from all other functions. It is especially bizarre to me that explicit attributes on the definition sites are silently ignored for these functions and no others.

Few if any existing users of -fvisibility or visibility attributes use them on definitions of operator new and operator delete. The notion that existing users are expecting this bizarrely inconsistent behavior seems pretty questionable to me.

But indeed I do know what I'm doing and I am willing to tell the compiler even more explicitly if you insist that I should have to do that for some reason.
I don't care what the switch is called. This is only ever going to be used in basically one place in the world (the libc++ definitions when building it for hermetic static linking).

[rsmith]

In D53787#1282930, @mcgrathr wrote:

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

I don't really understand how these functions are different from other functions. The language standards don't have anything to say about ELF visibility. What you say about "whole-program operation" is true of any global symbol.

These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler. Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program. Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.

It is especially bizarre to me that explicit attributes on the definition sites are silently ignored for these functions and no others.

Do you have a testcase? That's not the behavior I'm seeing. What I see is that we get a hard error for an explicit attribute on the definition site, because the prior compiler-generated declaration has default visibility. Eg:

$ echo '__attribute__((visibility("hidden"))) void *operator new(__SIZE_TYPE__) { return (void*)42; }' | clang -x c++ -
<stdin>:1:16: error: visibility does not match previous declaration
__attribute__((visibility("hidden"))) void *operator new(__SIZE_TYPE__) { return (void*)42; }
               ^
note: previous attribute is here

(That terrible note at the end is trying to point at the implicit compiler-generated declaration.)

[mcgrathr]

In D53787#1282975, @rsmith wrote:

These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler.

The implicit declaration is the only difference that actually makes sense to me.

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.

I don't understand this claim. These symbols are normally defined in libc++.so and nowhere else.

It is especially bizarre to me that explicit attributes on the definition sites are silently ignored for these functions and no others.

Do you have a testcase? That's not the behavior I'm seeing. What I see is that we get a hard error for an explicit attribute on the definition site, because the prior compiler-generated declaration has default visibility. Eg:

You're right. It's been quite a while since I was fighting with this originally. It might have been GCC that ignored explicit attributes.

[rsmith]

In D53787#1282995, @mcgrathr wrote:

In D53787#1282975, @rsmith wrote:

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

The program has two choices: either it provides a definition, and that gets used everywhere, or it does not, and the default version (provided by the toolchain) gets used everywhere. This is what the language model requires, and it's so important that the entire program agrees on what the default heap is that we intentionally make this work even when using -fvisibility=hidden.

Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.

I don't understand this claim. These symbols are normally defined in libc++.so and nowhere else.

That's not accurate. As noted above, programs can provide their own definitions, which are required to replace the version that would otherwise be implicitly provided.

[phosek]

Following the earlier suggestion, I've modified to change to introduce a new -fvisibility-global-new-delete-hidden option that controls this behavior.

[mcgrathr]

In D53787#1283051, @rsmith wrote:

In D53787#1282995, @mcgrathr wrote:

In D53787#1282975, @rsmith wrote:

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

The program has two choices: either it provides a definition, and that gets used everywhere, or it does not, and the default version (provided by the toolchain) gets used everywhere.

That's true of all global symbols normally provided by libraries.

This is what the language model requires, and it's so important that the entire program agrees on what the default heap is that we intentionally make this work even when using -fvisibility=hidden.

So you're concerned with people who use -fvisibility=hidden on the TUs that define some functions, and then use dynamic linking with the expectation that those definitions will be seen across shared library boundaries.
I don't see how people specifying visibility different than what they actually want is different for these particular functions than for any others. But I'm biased towards users who actually understand what they asked the compiler to do.

Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.

I don't understand this claim. These symbols are normally defined in libc++.so and nowhere else.

That's not accurate. As noted above, programs can provide their own definitions, which are required to replace the version that would otherwise be implicitly provided.

So they are global symbols defined in libraries. That's all what you've said actually means to me.

I think it's clear that there is nothing different about these symbols than any others from the perspective of how linking works and how that relates to the language specification. The *only* actual difference is that the language specification says that you can override these standard library symbols and that you may not override other standard library symbols--a distinction that does not actually exist materially in linking, nor matter to a programmer following the language specification and not using the compiler in ways outside the language specification (such as controlling symbol visibility).

All of your explanations for how they are different have to do with saving people from themselves when they asked the compiler to do something non-default with all symbols but actually meant to treat these particular symbols differently.
AFAICT the rationale goes something like: linking is subtle and hard to understand; many people use -fvisibility=hidden and do not fully understand what it means; a likely error is failing to understand the need to define a function with explicit-default visibility when it should be visible across shared library boundaries; conforming C++ code may define its own operator new/operator delete functions to have those used by the standard C++ library code (which might be in a shared library) in preference to its own definitions, but may not define other standard functions and have the same expectation; ergo, people with a poor understanding of linking who have followed some advice they didn't fully understand to use -fvisibility=hidden and also might define their own operator new/operator delete functions are best served by the compiler doing what they meant rather than what they said.

I'm a person who understands linking thoroughly, so I'm not the target audience for the concerns you raise. That's fine. Since I do understand the issues, I have no trouble understanding when I need a special compiler option to get the link-time behavior that's correct for my needs.

[phosek]

@rsmith does this look reasonable to you?

[phosek]

Ping?

[phosek]

@rsmith friendly ping?

[rsmith]

Looks good with minor changes, thanks.

clang/include/clang/Basic/LangOptions.def

231

This should just be LANGOPT, not BENIGN_LANGOPT, because it changes how we construct the AST (in an incompatible way).

Also, could you please remove the "default" from the description of this LANGOPT (and the previous one)? "default visibility" means something else, so this is confusing as written.

clang/include/clang/Driver/Options.td

1738

Remove the "by default" here: the program can't change the visibility to anything else via an attribute.

clang/include/clang/Basic/LangOptions.def

 LANGOPT(ConceptsTS , 1, 0, "enable C++ Extensions for Concepts")
 BENIGN_LANGOPT(ModulesCodegen , 1, 0, "Modules code generation")
 BENIGN_LANGOPT(ModulesDebugInfo , 1, 0, "Modules debug info")
 BENIGN_LANGOPT(ElideConstructors , 1, 1, "C++ copy constructor elision")
 BENIGN_LANGOPT(DumpRecordLayouts , 1, 0, "dumping the layout of IRgen'd records")
 BENIGN_LANGOPT(DumpRecordLayoutsSimple , 1, 0, "dumping the layout of IRgen'd records in a simple form")
 BENIGN_LANGOPT(DumpVTableLayouts , 1, 0, "dumping the layouts of emitted vtables")
 LANGOPT(NoConstantCFStrings , 1, 0, "no constant CoreFoundation strings")
-BENIGN_LANGOPT(InlineVisibilityHidden , 1, 0, "hidden default visibility for inline C++ methods")
+BENIGN_LANGOPT(InlineVisibilityHidden , 1, 0, "hidden visibility for inline C++ methods")
+LANGOPT(GlobalAllocationFunctionVisibilityHidden , 1, 0, "hidden visibility for global operator new and delete declaration")
 BENIGN_LANGOPT(ParseUnknownAnytype, 1, 0, "__unknown_anytype")
 BENIGN_LANGOPT(DebuggerSupport , 1, 0, "debugger support")
 BENIGN_LANGOPT(DebuggerCastResultToId, 1, 0, "for 'po' in the debugger, cast the result to id if it is of unknown type")
 BENIGN_LANGOPT(DebuggerObjCLiteral , 1, 0, "debugger Objective-C literals and subscripting support")
 
 BENIGN_LANGOPT(SpellChecking , 1, 1, "spell-checking")
 LANGOPT(SinglePrecisionConstants , 1, 0, "treating double-precision floating point constants as single precision constants")
 LANGOPT(FastRelaxedMath , 1, 0, "OpenCL fast relaxed math")

clang/include/clang/Driver/Options.td

 def fvisibility_EQ : Joined<["-"], "fvisibility=">, Group<f_Group>,
   HelpText<"Set the default symbol visibility for all global declarations">, Values<"hidden,default">;
 def fvisibility_inlines_hidden : Flag<["-"], "fvisibility-inlines-hidden">, Group<f_Group>,
   HelpText<"Give inline C++ member functions hidden visibility by default">,
   Flags<[CC1Option]>;
 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">;
+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">, Flags<[CC1Option]>;
 def fwhole_program_vtables : Flag<["-"], "fwhole-program-vtables">, Group<f_Group>,
   Flags<[CoreOption, CC1Option]>,
   HelpText<"Enables whole-program vtable optimization. Requires -flto">;
 def fno_whole_program_vtables : Flag<["-"], "fno-whole-program-vtables">, Group<f_Group>,
   Flags<[CoreOption]>;
 def fforce_emit_vtables : Flag<["-"], "fforce-emit-vtables">, Group<f_Group>,
     Flags<[CC1Option]>,
     HelpText<"Emits more virtual tables to improve devirtualization">;

clang/lib/Driver/ToolChains/Clang.cpp

if (A->getOption().matches(options::OPT_fvisibility_EQ)) {
       CmdArgs.push_back("-fvisibility");
       CmdArgs.push_back("hidden");
       CmdArgs.push_back("-ftype-visibility");
       CmdArgs.push_back("default");
     }
   }
 
   Args.AddLastArg(CmdArgs, options::OPT_fvisibility_inlines_hidden);
+  Args.AddLastArg(CmdArgs, options::OPT_fvisibility_global_new_delete_hidden);
 
   Args.AddLastArg(CmdArgs, options::OPT_ftlsmodel_EQ);
 
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
   Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);

clang/lib/Frontend/CompilerInvocation.cpp

if (Arg *typeVisOpt = Args.getLastArg(OPT_ftype_visibility)) {
     Opts.setTypeVisibilityMode(parseVisibility(typeVisOpt, Args, Diags));
   } else {
     Opts.setTypeVisibilityMode(Opts.getValueVisibilityMode());
   }
 
   if (Args.hasArg(OPT_fvisibility_inlines_hidden))
     Opts.InlineVisibilityHidden = 1;
 
+  if (Args.hasArg(OPT_fvisibility_global_new_delete_hidden))
+    Opts.GlobalAllocationFunctionVisibilityHidden = 1;
+
   if (Args.hasArg(OPT_ftrapv)) {
     Opts.setSignedOverflowBehavior(LangOptions::SOB_Trapping);
     // Set the handler, if one is specified.
     Opts.OverflowHandler =
         Args.getLastArgValue(OPT_ftrapv_handler);
   }
   else if (Args.hasArg(OPT_fwrapv))
     Opts.setSignedOverflowBehavior(LangOptions::SOB_Defined);

clang/lib/Sema/SemaExprCXX.cpp

auto CreateAllocationFunctionDecl = [&](Attr *ExtraAttr) {
     QualType FnType = Context.getFunctionType(Return, Params, EPI);
     FunctionDecl *Alloc = FunctionDecl::Create(
         Context, GlobalCtx, SourceLocation(), SourceLocation(), Name,
         FnType, /*TInfo=*/nullptr, SC_None, false, true);
     Alloc->setImplicit();
     // Global allocation functions should always be visible.
     Alloc->setVisibleDespiteOwningModule();
 
-    // Implicit sized deallocation functions always have default visibility.
-    Alloc->addAttr(
-        VisibilityAttr::CreateImplicit(Context, VisibilityAttr::Default));
+    Alloc->addAttr(VisibilityAttr::CreateImplicit(
+        Context, LangOpts.GlobalAllocationFunctionVisibilityHidden
+                     ? VisibilityAttr::Hidden
+                     : VisibilityAttr::Default));
 
     llvm::SmallVector<ParmVarDecl *, 3> ParamDecls;
     for (QualType T : Params) {
       ParamDecls.push_back(ParmVarDecl::Create(
           Context, Alloc, SourceLocation(), SourceLocation(), nullptr, T,
           /*TInfo=*/nullptr, SC_None, nullptr));
       ParamDecls.back()->setImplicit();
     }