[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
rsmith added a comment. In https://reviews.llvm.org/D53787#1282995, @mcgrathr wrote: > In https://reviews.llvm.org/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. Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
mcgrathr added a comment. In https://reviews.llvm.org/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. Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
rsmith added a comment.
In https://reviews.llvm.org/D53787#1282930, @mcgrathr wrote:
> In https://reviews.llvm.org/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++ -
: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.)
Repository:
rC Clang
https://reviews.llvm.org/D53787
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
mcgrathr added a comment. In https://reviews.llvm.org/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). Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
rsmith added a comment. In https://reviews.llvm.org/D53787#1279913, @phosek wrote: > In https://reviews.llvm.org/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? Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
phosek added a comment. In https://reviews.llvm.org/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? Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. 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=. Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
rnk added a comment.
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.
Repository:
rC Clang
https://reviews.llvm.org/D53787
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
phosek created this revision.
phosek added reviewers: rsmith, rnk.
Herald added a subscriber: cfe-commits.
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 modifies the implicit declaration of the global new and
delete operators to respect the Clang visibility setting i.e. the
-fvisibility= flag.
Repository:
rC Clang
https://reviews.llvm.org/D53787
Files:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaExprCXX.cpp
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -2816,9 +2816,9 @@
// 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,
+getVisibilityAttr(Context.getLangOpts().getValueVisibilityMode(;
llvm::SmallVector ParamDecls;
for (QualType T : Params) {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -5195,6 +5195,18 @@
AFS_Both
};
+ static VisibilityAttr::VisibilityType getVisibilityAttr(clang::Visibility V)
{
+switch (V) {
+case DefaultVisibility:
+ return VisibilityAttr::Default;
+case HiddenVisibility:
+ return VisibilityAttr::Hidden;
+case ProtectedVisibility:
+ return VisibilityAttr::Protected;
+}
+llvm_unreachable("unknown visibility!");
+ };
+
/// Finds the overloads of operator new and delete that are appropriate
/// for the allocation.
bool FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -2816,9 +2816,9 @@
// 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,
+getVisibilityAttr(Context.getLangOpts().getValueVisibilityMode(;
llvm::SmallVector ParamDecls;
for (QualType T : Params) {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -5195,6 +5195,18 @@
AFS_Both
};
+ static VisibilityAttr::VisibilityType getVisibilityAttr(clang::Visibility V) {
+switch (V) {
+case DefaultVisibility:
+ return VisibilityAttr::Default;
+case HiddenVisibility:
+ return VisibilityAttr::Hidden;
+case ProtectedVisibility:
+ return VisibilityAttr::Protected;
+}
+llvm_unreachable("unknown visibility!");
+ };
+
/// Finds the overloads of operator new and delete that are appropriate
/// for the allocation.
bool FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
