Make operator new/delete symbols have default visibility

Make operator new/delete symbols have default visibility

This CL changes the visibility of operator new and operator delete to
default so that the weak symbols exported by libc++ don't interfere
with the strong symbols exported by Chromium by making them hidden
too.

BUG=732548
R=thakis@chromium.org

Committed: 9a65473a7e8f39f04fd134e69470ab1b48514b0e

[Tom Anderson, 2017-06-13 02:56:25]

thakis ptal

[(unused - use chromium), 2017-06-13 20:49:37]

thakis@google.com changed reviewers:
+ thakis@google.com

[(unused - use chromium), 2017-06-13 20:49:37]

This scares me a bit (for now both libclearkey.so or some other .so and chrome
could export this symbol, and conflicts here are abstractly scary).

Is the problem that the compiler sees both the libc++ operator new decl and the
one in tcmalloc, and then when merging assumes hidden visibility for the one in
tcmalloc? Would putting explicit visibility("default") on the symbols in
tcmalloc help?

[Tom Anderson, 2017-06-13 22:30:17]

On 2017/06/13 20:49:37, (unused - use chromium) wrote:
> This scares me a bit (for now both libclearkey.so or some other .so and chrome
> could export this symbol, and conflicts here are abstractly scary).

Right now, in a normal release build, both chrome and libclearkeycdm.so export
new and delete.

> Is the problem that the compiler sees both the libc++ operator new decl and
the
> one in tcmalloc, and then when merging assumes hidden visibility for the one
in
> tcmalloc? 

Yes

> Would putting explicit visibility("default") on the symbols in
> tcmalloc help?

They already have that

I also tried removing the definitions of new/delete from libc++ and libc++abi
(there are macros you can set for this).  But there are executables like re2c
that don't depend on base but do depend on exe_and_shlib_deps
https://cs.chromium.org/chromium/src/third_party/yasm/BUILD.gn?rcl=5001f884a3...
These will fail to link due to the missing new/delete.

Maybe one thing we can try doing in the long run is disabling the libc++
definitions as per the above, but also move the allocator shim code away from
base into a new target and add it to exe_and_shlib_deps

[Tom Anderson, 2017-06-14 20:00:22]

On 2017/06/13 22:30:17, Tom Anderson wrote:
> Maybe one thing we can try doing in the long run is disabling the libc++
> definitions as per the above, but also move the allocator shim code away from
> base into a new target and add it to exe_and_shlib_deps

Actually this doesn't actually fix the problem of chrome and libclearkeycdm.so
both exporting new/delete symbols.

All executables and shared libraries need the symbols available, and so the only
way to do this without duplicate definitions is to

1. Move the new/delete code into a shared library that all executables and
shared libraries depend on
or
2. Make the new/delete symbols have hidden visibility and have multiple private
allocators

I don't like either one tbh.  I think this CL is the "right" solution

[Tom Anderson, 2017-06-14 20:01:43]

Oh 1 more thing: C++17 has additional new/delete symbols that have alignment
constraints.  The allocator code doesn't override these.  Not sure if this can
cause further issues..

[Nico, 2017-06-14 20:27:56]

On 2017/06/13 22:30:17, Tom Anderson wrote:
> On 2017/06/13 20:49:37, (unused - use chromium) wrote:
> > This scares me a bit (for now both libclearkey.so or some other .so and
chrome
> > could export this symbol, and conflicts here are abstractly scary).
> 
> Right now, in a normal release build, both chrome and libclearkeycdm.so export
> new and delete.
> 
> > Is the problem that the compiler sees both the libc++ operator new decl and
> the
> > one in tcmalloc, and then when merging assumes hidden visibility for the one
> in
> > tcmalloc? 
> 
> Yes
> 
> > Would putting explicit visibility("default") on the symbols in
> > tcmalloc help?
> 
> They already have that
> 
> I also tried removing the definitions of new/delete from libc++ and libc++abi
> (there are macros you can set for this).  But there are executables like re2c
> that don't depend on base but do depend on exe_and_shlib_deps
>
https://cs.chromium.org/chromium/src/third_party/yasm/BUILD.gn?rcl=5001f884a3...
> These will fail to link due to the missing new/delete.

This is surprising to me. libc++ declares op new with default visibility. We
build with -fvisibility=hidden -fvisibility-inlines-hidden. libc++'s op new ends
up hidden. (good.) tcmalloc has expclit vis(def) annotations. Why do the
tcmalloc versions not end up being exported if they already have this
annotation?

I think in an ideal end state, things linking against tcmalloc would export op
new via tcmalloc (and we'd likely have to add more overrides for c++17) since I
think that's needed for the override to work, and things not linking against
tcmalloc would get the hidden op new from libc++.

[Tom Anderson, 2017-06-14 21:31:27]

On 2017/06/14 20:27:56, Nico wrote:
> On 2017/06/13 22:30:17, Tom Anderson wrote:
> > On 2017/06/13 20:49:37, (unused - use chromium) wrote:
> > > This scares me a bit (for now both libclearkey.so or some other .so and
> chrome
> > > could export this symbol, and conflicts here are abstractly scary).
> > 
> > Right now, in a normal release build, both chrome and libclearkeycdm.so
export
> > new and delete.
> > 
> > > Is the problem that the compiler sees both the libc++ operator new decl
and
> > the
> > > one in tcmalloc, and then when merging assumes hidden visibility for the
one
> > in
> > > tcmalloc? 
> > 
> > Yes
> > 
> > > Would putting explicit visibility("default") on the symbols in
> > > tcmalloc help?
> > 
> > They already have that
> > 
> > I also tried removing the definitions of new/delete from libc++ and
libc++abi
> > (there are macros you can set for this).  But there are executables like
re2c
> > that don't depend on base but do depend on exe_and_shlib_deps
> >
>
https://cs.chromium.org/chromium/src/third_party/yasm/BUILD.gn?rcl=5001f884a3...
> > These will fail to link due to the missing new/delete.
> 
> This is surprising to me. libc++ declares op new with default visibility. We
> build with -fvisibility=hidden -fvisibility-inlines-hidden. libc++'s op new
ends
> up hidden. (good.) tcmalloc has expclit vis(def) annotations. Why do the
> tcmalloc versions not end up being exported if they already have this
> annotation?
> 
> I think in an ideal end state, things linking against tcmalloc would export op
> new via tcmalloc (and we'd likely have to add more overrides for c++17) since
I
> think that's needed for the override to work, and things not linking against
> tcmalloc would get the hidden op new from libc++.

I think you're onto something here.. the chrome binary exports _Znwm but not the
other symbols.

$ objdump -t chrome | grep _Znwm
00000000018cd040 l     F .text	000000000000001f              .hidden
_ZnwmRKSt9nothrow_t
0000000000dd5f40 l     F .text	00000000000000b8              .hidden
_ZnwmSt11align_val_t
0000000000dd6000 l     F .text	000000000000007b              .hidden
_ZnwmSt11align_val_tRKSt9nothrow_t
00000000018ccfc0 g     F .text	000000000000001b              _Znwm

Sure enough, out of the 3 object files that get linked into chrome that have
_Znwm, the libc++ ones have that symbol *not* hidden.

$ objdump -t obj/base/base/allocator_shim.o | grep _Znwm
00000000000002a0 g     F .text	000000000000001b _Znwm
0000000000000320 g     F .text	000000000000001f _ZnwmRKSt9nothrow_t

$ objdump -t obj/buildtools/third_party/libc++/libc++/new.o | grep _Znwm
0000000000000050  w    F .text	000000000000009e _Znwm
00000000000000f0  w    F .text	0000000000000073 .hidden _ZnwmRKSt9nothrow_t
00000000000002e0  w    F .text	00000000000000b8 .hidden _ZnwmSt11align_val_t
00000000000003a0  w    F .text	000000000000007b .hidden
_ZnwmSt11align_val_tRKSt9nothrow_t

$ objdump -t obj/buildtools/third_party/libc++abi/libc++abi/stdlib_new_delete.o
| grep _Znwm
0000000000000000  w    F .text	000000000000009e _Znwm
00000000000000a0  w    F .text	0000000000000073 .hidden _ZnwmRKSt9nothrow_t
0000000000000290  w    F .text	00000000000000b8 .hidden _ZnwmSt11align_val_t
0000000000000350  w    F .text	000000000000007b .hidden
_ZnwmSt11align_val_tRKSt9nothrow_t

The same thing happens for delete.  I'm very confused.  First, why is it just
the regular new/delete that don't get hidden?  And why does the linker make
_ZnwmRKSt9nothrow_t hidden if the strong symbol in allocator_shim.o is not
hidden?  Finally, a local _Znwm is actually what we want, and it's been broken
this entire time??

[Tom Anderson, 2017-06-14 23:23:36]

I have a minimal example of the first thing:

-----test.cpp-----
typedef long unsigned int size_t;
struct nothrow_t {};

void* operator new(size_t size) { return (void*)1; }
void* operator new(size_t size, const nothrow_t&) noexcept { return (void*)1; }
------------------

$ g++ -fvisibility=hidden -nostdinc++ -std=c++11 -c test.cpp -o test.o
$ objdump -t test.o | grep _Znwm
0000000000000000 g     F .text	000000000000000f _Znwm
000000000000000f g     F .text	0000000000000013 .hidden _ZnwmRK9nothrow_t

This occurs with g++4.8 and clang 5.0.0, but not clang 3.4.1.  I don't even know
if this is correct behavior :(

[Tom Anderson, 2017-06-15 02:05:14]

Description was changed from

==========
Make operator new/delete symbols have default visibility

This CL changes the visibility of operator new and operator delete to
default so that the weak symbols exported by libc++ don't interfere
with the strong symbols exported by Chromium by making them hidden
too.

BUG=732548
R=thakis@chromium.org
==========

to

==========
Make operator new/delete symbols have default visibility

This CL changes the visibility of operator new and operator delete to
default so that the weak symbols exported by libc++ don't interfere
with the strong symbols exported by Chromium by making them hidden
too.

BUG=732548
R=thakis@chromium.org
==========

[Tom Anderson, 2017-06-15 02:05:14]

thomasanderson@chromium.org changed reviewers:
+ pcc@chromium.org

[Tom Anderson, 2017-06-15 02:06:39]

If I add 'attribute((visibility("hidden")))' in test.cpp above, clang gives:
test.cpp:4:16: error: visibility does not match previous declaration
__attribute__((visibility("hidden")))
               ^
note: previous attribute is here
1 error generated.


And gcc gives:
test.cpp: In function ‘void* operator new(size_t)’:
test.cpp:5:7: warning: ‘void* operator new(size_t)’: visibility attribute
ignored because it [-Wattributes]
 void* operator new(size_t size) { return (void*)1; }
       ^
<built-in>:0:0: warning: conflicts with previous declaration here [-Wattributes]


+pcc pls help

[pcc1, 2017-06-15 04:13:43]

On 2017/06/15 02:06:39, Tom Anderson wrote:
> If I add 'attribute((visibility("hidden")))' in test.cpp above, clang gives:
> test.cpp:4:16: error: visibility does not match previous declaration
> __attribute__((visibility("hidden")))
>                ^
> note: previous attribute is here
> 1 error generated.
> 
> 
> And gcc gives:
> test.cpp: In function ‘void* operator new(size_t)’:
> test.cpp:5:7: warning: ‘void* operator new(size_t)’: visibility attribute
> ignored because it [-Wattributes]
>  void* operator new(size_t size) { return (void*)1; }
>        ^
> <built-in>:0:0: warning: conflicts with previous declaration here
[-Wattributes]
> 
> 
> +pcc pls help

This behaviour doesn't seem correct (for either compiler). The explicit
visibility attribute on the definition should override any implicit attributes
created by the compiler, and should certainly not cause an error.

In Clang's case at least the implicit behaviour seems very questionable, and is
being caused by this line of code:
http://llvm-cs.pcc.me.uk/tools/clang/lib/Sema/SemaExprCXX.cpp#2668
Despite the comment that reads "// Implicit sized deallocation functions always
have default visibility.", that line of code affects all new/delete functions
that are implicitly declared by the compiler.

We should file a bug against both compilers. In the meantime, you can work
around the issue by adding this line of inline asm (at the top level) to any
translation unit that is linked into the binary:

__asm__(".hidden _Znwm");

> First, why is it just the regular new/delete that don't get hidden?

I think this is because the compiler only has builtin declarations for those
regular new/delete functions. The standard says that only these definitions are
built into the compiler:

void* operator new(std::size_t);
void* operator new[](std::size_t);
void operator delete(void*) noexcept;
void operator delete[](void*) noexcept;
void operator delete(void*, std::size_t) noexcept;
void operator delete[](void*, std::size_t) noexcept;

> And why does the linker make _ZnwmRKSt9nothrow_t hidden if the strong symbol
in allocator_shim.o is not hidden?

Because ELF visibility merging semantics are bonkers. When the linker decides
which visibility to assign a given symbol, it uses the "most hidden" visibility
of *any* symbol that it sees. It doesn't matter whether it is a weak or strong
symbol, or even whether it is defined or undefined (that is why it doesn't
matter where you put the inline asm snippet that I mentioned).

I think what this might mean in practice is that we would somehow need to avoid
linking libc++'s operators into the binary if we are also linking tcmalloc.

[Tom Anderson, 2017-06-15 20:37:09]

On 2017/06/15 04:13:43, pcc1 wrote:
> On 2017/06/15 02:06:39, Tom Anderson wrote:
> > If I add 'attribute((visibility("hidden")))' in test.cpp above, clang gives:
> > test.cpp:4:16: error: visibility does not match previous declaration
> > __attribute__((visibility("hidden")))
> >                ^
> > note: previous attribute is here
> > 1 error generated.
> > 
> > 
> > And gcc gives:
> > test.cpp: In function ‘void* operator new(size_t)’:
> > test.cpp:5:7: warning: ‘void* operator new(size_t)’: visibility attribute
> > ignored because it [-Wattributes]
> >  void* operator new(size_t size) { return (void*)1; }
> >        ^
> > <built-in>:0:0: warning: conflicts with previous declaration here
> [-Wattributes]
> > 
> > 
> > +pcc pls help
> 
> This behaviour doesn't seem correct (for either compiler). The explicit
> visibility attribute on the definition should override any implicit attributes
> created by the compiler, and should certainly not cause an error.
> 
> In Clang's case at least the implicit behaviour seems very questionable, and
is
> being caused by this line of code:
> http://llvm-cs.pcc.me.uk/tools/clang/lib/Sema/SemaExprCXX.cpp#2668
> Despite the comment that reads "// Implicit sized deallocation functions
always
> have default visibility.", that line of code affects all new/delete functions
> that are implicitly declared by the compiler.
> 
> We should file a bug against both compilers. In the meantime, you can work
> around the issue by adding this line of inline asm (at the top level) to any
> translation unit that is linked into the binary:
> 
> __asm__(".hidden _Znwm");
> 
> > First, why is it just the regular new/delete that don't get hidden?
> 
> I think this is because the compiler only has builtin declarations for those
> regular new/delete functions. The standard says that only these definitions
are
> built into the compiler:
> 
> void* operator new(std::size_t);
> void* operator new[](std::size_t);
> void operator delete(void*) noexcept;
> void operator delete[](void*) noexcept;
> void operator delete(void*, std::size_t) noexcept;
> void operator delete[](void*, std::size_t) noexcept;
> 
> > And why does the linker make _ZnwmRKSt9nothrow_t hidden if the strong symbol
> in allocator_shim.o is not hidden?
> 
> Because ELF visibility merging semantics are bonkers. When the linker decides
> which visibility to assign a given symbol, it uses the "most hidden"
visibility
> of *any* symbol that it sees. It doesn't matter whether it is a weak or strong
> symbol, or even whether it is defined or undefined (that is why it doesn't
> matter where you put the inline asm snippet that I mentioned).
> 
> I think what this might mean in practice is that we would somehow need to
avoid
> linking libc++'s operators into the binary if we are also linking tcmalloc.

Thanks for the explanation!  Filed the bugs and a followup patch set is here:
https://codereview.chromium.org/2938913003

[Tom Anderson, 2017-06-16 16:47:05]

thomasanderson@chromium.org changed reviewers:
+ primiano@chromium.org

[Tom Anderson, 2017-06-16 16:47:18]

unclosing and +primiano

[Nico, 2017-06-19 19:22:42]

lgtm to land this as a workaround for now, and to then pursue the long-term fix
outlined below later, as discussed in person. (pcc, shout if that sounds
infeasible.)

https://codereview.chromium.org/2937433003/diff/1/third_party/libc++/BUILD.gn
File third_party/libc++/BUILD.gn (right):

https://codereview.chromium.org/2937433003/diff/1/third_party/libc++/BUILD.gn...
third_party/libc++/BUILD.gn:27:
"_LIBCPP_OVERRIDABLE_FUNC_VIS=__attribute__((__visibility__(\"default\")))",
Can you add a comment here along the lines of "
elf visibility rules require that linkers use the least visible form when
merging, and if this is hidden, then when we merge it with tcmalloc's operator
new, hidden visibility would win. Hoever, tcmalloc needs a visible operator new
to also override operator new references from system libraries.
TODO(lld): Ask lld for a --force-public-visibility flag or similar to that
overrides the default elf merging rules, and make tcmalloc's gn config pass that
to all its dependencies, then remove this override here."

[Tom Anderson, 2017-06-19 19:27:40]

https://codereview.chromium.org/2937433003/diff/1/third_party/libc++/BUILD.gn
File third_party/libc++/BUILD.gn (right):

https://codereview.chromium.org/2937433003/diff/1/third_party/libc++/BUILD.gn...
third_party/libc++/BUILD.gn:27:
"_LIBCPP_OVERRIDABLE_FUNC_VIS=__attribute__((__visibility__(\"default\")))",
On 2017/06/19 19:22:41, Nico wrote:
> Can you add a comment here along the lines of "
> elf visibility rules require that linkers use the least visible form when
> merging, and if this is hidden, then when we merge it with tcmalloc's operator
> new, hidden visibility would win. Hoever, tcmalloc needs a visible operator
new
> to also override operator new references from system libraries.
> TODO(lld): Ask lld for a --force-public-visibility flag or similar to that
> overrides the default elf merging rules, and make tcmalloc's gn config pass
that
> to all its dependencies, then remove this override here."

Done.

[Tom Anderson, 2017-06-19 19:27:49]

Description was changed from

==========
Make operator new/delete symbols have default visibility

This CL changes the visibility of operator new and operator delete to
default so that the weak symbols exported by libc++ don't interfere
with the strong symbols exported by Chromium by making them hidden
too.

BUG=732548
R=thakis@chromium.org
==========

to

==========
Make operator new/delete symbols have default visibility

This CL changes the visibility of operator new and operator delete to
default so that the weak symbols exported by libc++ don't interfere
with the strong symbols exported by Chromium by making them hidden
too.

BUG=732548
R=thakis@chromium.org

Committed: 9a65473a7e8f39f04fd134e69470ab1b48514b0e
==========

[Tom Anderson, 2017-06-19 19:27:50]

Committed patchset #2 (id:20001) manually as
9a65473a7e8f39f04fd134e69470ab1b48514b0e (presubmit successful).