Restore the -fvisibility=hidden flag for tcmalloc when using allocator shim.

Restore the -fvisibility=hidden flag for tcmalloc when using allocator shim.

Hidden visibility was initially disabled for tcmalloc in
https://codereview.chromium.org/343037 as a workaround for
a lack of symbol visibility annotations in the source. These
annotations were finally added as part of the new allocator shim
(see //base/allocator/allocator_shim_override_glibc_weak_symbols.h
and //base/allocator/allocator_shim_internals.h for the definition of
SHIM_ALWAYS_EXPORT). Hence it is no longer necessary to compile tcmalloc
with default visibility when using the allocator shim.

This fixes an issue where some of tcmalloc's internal classes were being
compiled with default visibility in the implementation and with hidden
visibility in clients. This is technically an ODR violation, but not normally
a significant one. The ODR violation was exposed by an upcoming LLVM change
[1] that enables the relative C++ ABI for (approximately) classes with
hidden visibility.

[1] http://reviews.llvm.org/D20749

BUG=589384
R=primiano@chromium.org, thakis@chromium.org

Committed: https://crrev.com/1110ac7a5c525ab81105a8cf7be2f7b59d669657
Cr-Commit-Position: refs/heads/master@{#397015}

[pcc1, 2016-05-28 01:55:17]

The CQ bit was checked by pcc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-28 01:55:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019183002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019183002/1

[commit-bot: I haz the power, 2016-05-28 04:20:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-28 04:20:13]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-05-31 07:51:36]

This patch makes and LGTM. I never realized that we were exporting, at the final
executable level, so many symbols.
After this patch the chrome binary will stop exposing the following symbols:
https://gist.github.com/primiano/18fc49242275bb9589ae7825bdd93cfd

I just hope that we are not in the situation where some external library we use
today ends up relying on any of this.
The only suspicious symbols I see in that list are mmap/mremap/sbrk. But I
checked the code and it seems that today tcmalloc does just intercept them to
invoke the hooks (which we don't use anymore, after DMP has been deprecated) and
forwards them to glibc.

It seems extremely unlikely and if it happens this still feels to me the right
thing to do. (I just hope that it will not happen)

+wfh in case he has any extra thoughts.

[pcc1, 2016-05-31 20:57:21]

Thanks Primiano.

On 2016/05/31 07:51:36, Primiano Tucci wrote:
> The only suspicious symbols I see in that list are mmap/mremap/sbrk. But I
> checked the code and it seems that today tcmalloc does just intercept them to
> invoke the hooks (which we don't use anymore, after DMP has been deprecated)
and
> forwards them to glibc.

Those hooks do appear to be used to implement tcmalloc's built-in heap profiler.
Do we care about that profiler at all? There does seem to be documentation for
it at
https://chromium.googlesource.com/chromium/src/+/lkgr/docs/linux_profiling.md...
but maybe that's out of date.

If we do care about it, we may want to consider adding default visibility
annotations to those functions, but the downside of not doing so seems to be
small (we'd miss mmap/munmap/sbrk allocations in third-party DSOs) so maybe not
worth worrying about.

[Nico, 2016-05-31 21:08:53]

lgtm

FWIW on OS X we check at build time that we only export a (small) set of known
symbols:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/framewo...

I wish we did this on other platforms too.

[pcc1, 2016-05-31 21:13:59]

The CQ bit was checked by pcc@chromium.org

[commit-bot: I haz the power, 2016-05-31 21:14:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019183002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019183002/1

[commit-bot: I haz the power, 2016-05-31 23:54:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 23:54:08]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pcc1, 2016-05-31 23:54:50]

The CQ bit was checked by pcc@chromium.org

[commit-bot: I haz the power, 2016-05-31 23:55:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019183002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019183002/1

[commit-bot: I haz the power, 2016-06-01 01:25:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-01 01:28:09]

Description was changed from

==========
Restore the -fvisibility=hidden flag for tcmalloc when using allocator shim.

Hidden visibility was initially disabled for tcmalloc in
https://codereview.chromium.org/343037 as a workaround for
a lack of symbol visibility annotations in the source. These
annotations were finally added as part of the new allocator shim
(see //base/allocator/allocator_shim_override_glibc_weak_symbols.h
and //base/allocator/allocator_shim_internals.h for the definition of
SHIM_ALWAYS_EXPORT). Hence it is no longer necessary to compile tcmalloc
with default visibility when using the allocator shim.

This fixes an issue where some of tcmalloc's internal classes were being
compiled with default visibility in the implementation and with hidden
visibility in clients. This is technically an ODR violation, but not normally
a significant one. The ODR violation was exposed by an upcoming LLVM change
[1] that enables the relative C++ ABI for (approximately) classes with
hidden visibility.

[1] http://reviews.llvm.org/D20749

BUG=589384
R=primiano@chromium.org, thakis@chromium.org
==========

to

==========
Restore the -fvisibility=hidden flag for tcmalloc when using allocator shim.

Hidden visibility was initially disabled for tcmalloc in
https://codereview.chromium.org/343037 as a workaround for
a lack of symbol visibility annotations in the source. These
annotations were finally added as part of the new allocator shim
(see //base/allocator/allocator_shim_override_glibc_weak_symbols.h
and //base/allocator/allocator_shim_internals.h for the definition of
SHIM_ALWAYS_EXPORT). Hence it is no longer necessary to compile tcmalloc
with default visibility when using the allocator shim.

This fixes an issue where some of tcmalloc's internal classes were being
compiled with default visibility in the implementation and with hidden
visibility in clients. This is technically an ODR violation, but not normally
a significant one. The ODR violation was exposed by an upcoming LLVM change
[1] that enables the relative C++ ABI for (approximately) classes with
hidden visibility.

[1] http://reviews.llvm.org/D20749

BUG=589384
R=primiano@chromium.org, thakis@chromium.org

Committed: https://crrev.com/1110ac7a5c525ab81105a8cf7be2f7b59d669657
Cr-Commit-Position: refs/heads/master@{#397015}
==========

[commit-bot: I haz the power, 2016-06-01 01:28:10]

Patchset 1 (id:??) landed as
https://crrev.com/1110ac7a5c525ab81105a8cf7be2f7b59d669657
Cr-Commit-Position: refs/heads/master@{#397015}

[Nico, 2016-06-01 16:28:13]

This breaks building base_unittests on the clangtotlinux bot:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5379/...

FAILED: base_unittests 
../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
-fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -fuse-ld=gold
-B../../third_party/binutils/Linux_x64/Release/bin -Wl,--icf=all -pthread -m64
-Wl,-O1 -Wl,--gc-sections -Wl,--as-needed
--sysroot=../../build/linux/debian_wheezy_amd64-sysroot 
-Wl,-rpath-link=../Release -Wl,--disable-new-dtags -Wl,-rpath=\$ORIGIN/.
-Wl,-rpath-link=. -Wl,--export-dynamic -o "./base_unittests" -Wl,--start-group
@"./base_unittests.rsp" ./libbase.so ./libbase_i18n.so ./libmalloc_wrapper.so
./libicui18n.so ./libicuuc.so -Wl,--end-group  -ldl -lrt -lgmodule-2.0
-lgobject-2.0 -lgthread-2.0 -lglib-2.0 
../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
'tc_malloc'
../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
'tc_free'
../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
'tc_free'
../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
'tc_malloc'
../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
'tc_free'
../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
'tc_free'
../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
'tc_malloc'
../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
'tc_malloc'

Can you take a look?

[Primiano Tucci (use gerrit), 2016-06-01 16:51:23]

On 2016/06/01 16:28:13, Nico wrote:
> This breaks building base_unittests on the clangtotlinux bot:
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5379/...
> 
> FAILED: base_unittests 
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
> -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -fuse-ld=gold
> -B../../third_party/binutils/Linux_x64/Release/bin -Wl,--icf=all -pthread -m64
> -Wl,-O1 -Wl,--gc-sections -Wl,--as-needed
> --sysroot=../../build/linux/debian_wheezy_amd64-sysroot 
> -Wl,-rpath-link=../Release -Wl,--disable-new-dtags -Wl,-rpath=\$ORIGIN/.
> -Wl,-rpath-link=. -Wl,--export-dynamic -o "./base_unittests" -Wl,--start-group
> @"./base_unittests.rsp" ./libbase.so ./libbase_i18n.so ./libmalloc_wrapper.so
> ./libicui18n.so ./libicuuc.so -Wl,--end-group  -ldl -lrt -lgmodule-2.0
> -lgobject-2.0 -lgthread-2.0 -lglib-2.0 
> ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> 'tc_malloc'
> ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> 'tc_free'
> ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> 'tc_free'
> ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> 'tc_malloc'
> ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> 'tc_free'
> ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> 'tc_free'
> ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> 'tc_malloc'
> ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> 'tc_malloc'
> 
> Can you take a look?

I see what's happening. tcmalloc_unittest.cc uses tc_malloc and tc_free, which
in a component build are not exported anymore.
I don't see why tcmalloc_unittest.cc doesn't just use malloc() and free(). We
know that if we use tcmalloc they are routed to tc_malloc and tc_free. And if we
don't use tcmalloc there is nothing to test.
I think the right thing to do is:
- enable tcmalloc_unittest.cc (or relevant parts of it) when
defined(USE_TCMALLOC). I think that the trick there is that some parts of that
file are not really tcmalloc specific and we want those tests in any case.
- use malloc/free.

I am happy to fix this but I have to leave the office right now.
So either:
- s/tc_// and TBR me
- comment out all tcmalloc_unittest.cc with a TODO(primiano) and I'll fix it
tomorrow.

[pcc1, 2016-06-01 18:13:30]

On 2016/06/01 16:51:23, Primiano Tucci wrote:
> On 2016/06/01 16:28:13, Nico wrote:
> > This breaks building base_unittests on the clangtotlinux bot:
> >
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5379/...
> > 
> > FAILED: base_unittests 
> > ../../third_party/llvm-build/Release+Asserts/bin/clang++
-Wl,--fatal-warnings
> > -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -fuse-ld=gold
> > -B../../third_party/binutils/Linux_x64/Release/bin -Wl,--icf=all -pthread
-m64
> > -Wl,-O1 -Wl,--gc-sections -Wl,--as-needed
> > --sysroot=../../build/linux/debian_wheezy_amd64-sysroot 
> > -Wl,-rpath-link=../Release -Wl,--disable-new-dtags -Wl,-rpath=\$ORIGIN/.
> > -Wl,-rpath-link=. -Wl,--export-dynamic -o "./base_unittests"
-Wl,--start-group
> > @"./base_unittests.rsp" ./libbase.so ./libbase_i18n.so
./libmalloc_wrapper.so
> > ./libicui18n.so ./libicuuc.so -Wl,--end-group  -ldl -lrt -lgmodule-2.0
> > -lgobject-2.0 -lgthread-2.0 -lglib-2.0 
> > ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> > 'tc_malloc'
> > ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> > 'tc_free'
> > ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> > 'tc_free'
> > ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> > 'tc_malloc'
> > ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> > 'tc_free'
> > ../../base/allocator/tcmalloc_unittest.cc:29: error: undefined reference to
> > 'tc_free'
> > ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> > 'tc_malloc'
> > ../../base/allocator/tcmalloc_unittest.cc:25: error: undefined reference to
> > 'tc_malloc'
> > 
> > Can you take a look?
> 
> I see what's happening. tcmalloc_unittest.cc uses tc_malloc and tc_free, which
> in a component build are not exported anymore.
> I don't see why tcmalloc_unittest.cc doesn't just use malloc() and free(). We
> know that if we use tcmalloc they are routed to tc_malloc and tc_free. And if
we
> don't use tcmalloc there is nothing to test.
> I think the right thing to do is:
> - enable tcmalloc_unittest.cc (or relevant parts of it) when
> defined(USE_TCMALLOC). I think that the trick there is that some parts of that
> file are not really tcmalloc specific and we want those tests in any case.
> - use malloc/free.
> 
> I am happy to fix this but I have to leave the office right now.
> So either:
> - s/tc_// and TBR me
> - comment out all tcmalloc_unittest.cc with a TODO(primiano) and I'll fix it
> tomorrow.

Thanks Primiano, https://codereview.chromium.org/2028183002 does the former.