Force hidden symbol visibility for allocation operators

Force hidden symbol visibility for allocation operators

Clang and gcc don't currently allow declaring allocation operators
like operator new with hidden visibliity [1] [2].  This CL adds a
workaround thanks to pcc@ [3] to force hidden visibility of these
symbols on static builds.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81107
[2] https://bugs.llvm.org/show_bug.cgi?id=33473
[3] https://codereview.chromium.org/2937433003/#msg14

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

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

thakis and pcc ptal

[Nico, 2017-06-15 20:47:59]

Cool!

If I understood the comments on the other CL right, doesn't this mean that
tcmalloc's op new now also ends up with hidden visibility? If so, doesn't this
break malloc interception?

[Tom Anderson, 2017-06-15 21:06:45]

On 2017/06/15 20:47:59, Nico wrote:
> Cool!
> 
> If I understood the comments on the other CL right, doesn't this mean that
> tcmalloc's op new now also ends up with hidden visibility?

Yes, I thought that's what we wanted

>  If so, doesn't this break malloc interception?

It's only added on non-instrumented builds which don't need to intercept malloc.

[Nico, 2017-06-15 21:14:43]

On 2017/06/15 21:06:45, Tom Anderson wrote:
> On 2017/06/15 20:47:59, Nico wrote:
> > Cool!
> > 
> > If I understood the comments on the other CL right, doesn't this mean that
> > tcmalloc's op new now also ends up with hidden visibility?
> 
> Yes, I thought that's what we wanted

I thought we wanted (I might be wrong!):

- binaries dependending on tcmalloc get public op new etc via interceptor in
tcmalloc
- binaries not depending on tcmalloc get hidden op new

> 
> >  If so, doesn't this break malloc interception?
> 
> It's only added on non-instrumented builds which don't need to intercept
malloc.

Again, I might be wrong here, but I thought that we use tcmalloc's interception
for two things:

1.) For instrumentation, in instrumented builds
2.) To let tcmalloc manage our performance, for performance


(+primiano)

[Tom Anderson, 2017-06-15 21:43:17]

On 2017/06/15 21:14:43, Nico wrote:
> On 2017/06/15 21:06:45, Tom Anderson wrote:
> > On 2017/06/15 20:47:59, Nico wrote:
> > > Cool!
> > > 
> > > If I understood the comments on the other CL right, doesn't this mean that
> > > tcmalloc's op new now also ends up with hidden visibility?
> > 
> > Yes, I thought that's what we wanted
> 
> I thought we wanted (I might be wrong!):
> 
> - binaries dependending on tcmalloc get public op new etc via interceptor in
> tcmalloc

binaries depending on tcmalloc will get the tcmalloc version (due to the strong
symbol in tcmalloc / weak symbol in libc++)

> - binaries not depending on tcmalloc get hidden op new

binaries that don't depend on base (and therefore don't depend on tcmalloc) will
get the libc++ impl

Additionally, 3rd party libraries that use c++ will not have access to Chrome's
impls since they're hidden.  They will have to dynamically load the system
libc++ or libstdc++

> 
> > 
> > >  If so, doesn't this break malloc interception?
> > 
> > It's only added on non-instrumented builds which don't need to intercept
> malloc.
> 
> Again, I might be wrong here, but I thought that we use tcmalloc's
interception
> for two things:
> 
> 1.) For instrumentation, in instrumented builds
> 2.) To let tcmalloc manage our performance, for performance
> 
> 
> (+primiano)

[Tom Anderson, 2017-06-15 21:49:47]

On 2017/06/15 21:43:17, Tom Anderson wrote:
> On 2017/06/15 21:14:43, Nico wrote:
> > On 2017/06/15 21:06:45, Tom Anderson wrote:
> > > On 2017/06/15 20:47:59, Nico wrote:
> > > > Cool!
> > > > 
> > > > If I understood the comments on the other CL right, doesn't this mean
that
> > > > tcmalloc's op new now also ends up with hidden visibility?
> > > 
> > > Yes, I thought that's what we wanted
> > 
> > I thought we wanted (I might be wrong!):
> > 
> > - binaries dependending on tcmalloc get public op new etc via interceptor in
> > tcmalloc
> 
> binaries depending on tcmalloc will get the tcmalloc version (due to the
strong
> symbol in tcmalloc / weak symbol in libc++)
> 
> > - binaries not depending on tcmalloc get hidden op new
> 
> binaries that don't depend on base (and therefore don't depend on tcmalloc)
will
> get the libc++ impl

Oh I also want to point out that libc++abi provides new/delete as well as
libc++.  This is bad because there are 2 weak symbols and the linker can choose
either one.

I think we should remove the libc++abi ones by unconditionally removing
stdlib_new_delete.cpp from sources.  wdyt?

> Additionally, 3rd party libraries that use c++ will not have access to
Chrome's
> impls since they're hidden.  They will have to dynamically load the system
> libc++ or libstdc++
> 
> > 
> > > 
> > > >  If so, doesn't this break malloc interception?
> > > 
> > > It's only added on non-instrumented builds which don't need to intercept
> > malloc.
> > 
> > Again, I might be wrong here, but I thought that we use tcmalloc's
> interception
> > for two things:
> > 
> > 1.) For instrumentation, in instrumented builds
> > 2.) To let tcmalloc manage our performance, for performance
> > 
> > 
> > (+primiano)

[Primiano Tucci (use gerrit), 2017-06-16 10:39:15]

On 2017/06/15 21:14:43, Nico wrote:
> On 2017/06/15 21:06:45, Tom Anderson wrote:
> > On 2017/06/15 20:47:59, Nico wrote:
> > > Cool!
> > > 
> > > If I understood the comments on the other CL right, doesn't this mean that
> > > tcmalloc's op new now also ends up with hidden visibility?
> > 
> > Yes, I thought that's what we wanted
> 
> I thought we wanted (I might be wrong!):
> 
> - binaries dependending on tcmalloc get public op new etc via interceptor in
> tcmalloc
> - binaries not depending on tcmalloc get hidden op new
> 
> > 
> > >  If so, doesn't this break malloc interception?
> > 
> > It's only added on non-instrumented builds which don't need to intercept
> malloc.
> 
> Again, I might be wrong here, but I thought that we use tcmalloc's
interception
> for two things:
> 
> 1.) For instrumentation, in instrumented builds
> 2.) To let tcmalloc manage our performance, for performance
> (+primiano)

On top of what Nico said, there are other 2 biggish deals:
3) security
We deliberately want to leak (public global visibility) the allocator symbols,
so that we intercept allocations also for the 3rd party libraries we use
(libcups and whatnot). Before we get into tcmalloc there are a bunch of security
checks we do (we don't allow allocations >2GB as they might cause subtle
int-conversion bugs, we force suicide on allocation failure, regardless of
malloc vs new, regardless of exceptions)

4) stability (no cross heaps)
The heap used by third party libraries (libcups and whatnot) has to match the
default (malloc/new) heap that we use in chrome. I think there are a bunch of
places where we get something allocated in a third party library and freed in
chrome. Something similar for strdup() that happens inside libc.

Having said this, in all honesty, I never understood if we actually need to
override and leak operator new(), or if malloc/free are enough. I just carefully
maintained what tcmalloc is doing (overriding both malloc and new) and have
honestly no idea on what to expect if we stop overriding just new.
if this is just about CdmAdapterTest would be nice to try to figure out some
less invasive change for CdmAdapter.
So in essence: I am not saying that the current change is wrong. I am just
saying that the current change is risky, and the risk might not be worth it if
this is only to fix the CdmAdapter test.

[Tom Anderson, 2017-06-16 16:46:55]

On 2017/06/16 10:39:15, Primiano Tucci wrote:
> On 2017/06/15 21:14:43, Nico wrote:
> > On 2017/06/15 21:06:45, Tom Anderson wrote:
> > > On 2017/06/15 20:47:59, Nico wrote:
> > > > Cool!
> > > > 
> > > > If I understood the comments on the other CL right, doesn't this mean
that
> > > > tcmalloc's op new now also ends up with hidden visibility?
> > > 
> > > Yes, I thought that's what we wanted
> > 
> > I thought we wanted (I might be wrong!):
> > 
> > - binaries dependending on tcmalloc get public op new etc via interceptor in
> > tcmalloc
> > - binaries not depending on tcmalloc get hidden op new
> > 
> > > 
> > > >  If so, doesn't this break malloc interception?
> > > 
> > > It's only added on non-instrumented builds which don't need to intercept
> > malloc.
> > 
> > Again, I might be wrong here, but I thought that we use tcmalloc's
> interception
> > for two things:
> > 
> > 1.) For instrumentation, in instrumented builds
> > 2.) To let tcmalloc manage our performance, for performance
> > (+primiano)
> 
> On top of what Nico said, there are other 2 biggish deals:
> 3) security
> We deliberately want to leak (public global visibility) the allocator symbols,
> so that we intercept allocations also for the 3rd party libraries we use
> (libcups and whatnot). Before we get into tcmalloc there are a bunch of
security
> checks we do (we don't allow allocations >2GB as they might cause subtle
> int-conversion bugs, we force suicide on allocation failure, regardless of
> malloc vs new, regardless of exceptions)
> 
> 4) stability (no cross heaps)
> The heap used by third party libraries (libcups and whatnot) has to match the
> default (malloc/new) heap that we use in chrome. I think there are a bunch of
> places where we get something allocated in a third party library and freed in
> chrome. Something similar for strdup() that happens inside libc.
> 
> Having said this, in all honesty, I never understood if we actually need to
> override and leak operator new(), or if malloc/free are enough. I just
carefully
> maintained what tcmalloc is doing (overriding both malloc and new) and have
> honestly no idea on what to expect if we stop overriding just new.
> if this is just about CdmAdapterTest would be nice to try to figure out some
> less invasive change for CdmAdapter.
> So in essence: I am not saying that the current change is wrong. I am just
> saying that the current change is risky, and the risk might not be worth it if
> this is only to fix the CdmAdapter test.

Ok, sounds like the original CL is what we want then:
https://codereview.chromium.org/2937433003/

[Tom Anderson, 2017-06-19 19:21:03]

Description was changed from

==========
Force hidden symbol visibility for allocation operators

Clang and gcc don't currently allow declaring allocation operators
like operator new with hidden visibliity [1] [2].  This CL adds a
workaround thanks to pcc@ [3] to force hidden visibility of these
symbols on static builds.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81107
[2] https://bugs.llvm.org/show_bug.cgi?id=33473
[3] https://codereview.chromium.org/2937433003/#msg14

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

to

==========
Force hidden symbol visibility for allocation operators

Clang and gcc don't currently allow declaring allocation operators
like operator new with hidden visibliity [1] [2].  This CL adds a
workaround thanks to pcc@ [3] to force hidden visibility of these
symbols on static builds.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81107
[2] https://bugs.llvm.org/show_bug.cgi?id=33473
[3] https://codereview.chromium.org/2937433003/#msg14

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