Set noinline attribute on exported shim layer functions.

Set noinline attribute on exported shim layer functions.

If an exported symbol is linked into a DSO, it may be preempted by a
definition in the main executable. If this happens to an allocator symbol, it
will mean that the DSO will use the main executable's allocator. This is
normally relatively harmless -- regular allocations should all use the same
allocator, but if the DSO tries to hook the allocator it will not see any
allocations.

However, if LLVM LTO is enabled, the compiler may inline the shim layer
symbols into callers. The end result is that allocator calls in DSOs may use
either the main executable's allocator or the DSO's allocator, depending on
whether the call was inlined. This is arguably a bug in LLVM caused by its
somewhat irregular handling of symbol interposition (see llvm.org/PR23501).
To work around the bug we use noinline to prevent the symbols from being
inlined.

In the long run we probably want to avoid linking the allocator bits into
DSOs altogether. This will save a little space and stop giving DSOs the false
impression that they can hook the allocator.

BUG=617732
R=primiano@chromium.org

Review-Url: https://codereview.chromium.org/2748083004
Cr-Commit-Position: refs/heads/master@{#457103}
Committed: https://chromium.googlesource.com/chromium/src/+/5ba86e8e4101bf5cd483c0cc6da5264db3bc2464

[pcc1, 2017-03-15 03:10:51]

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

[commit-bot: I haz the power, 2017-03-15 03:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 04:39:19]

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

[commit-bot: I haz the power, 2017-03-15 04:39:20]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-03-15 11:42:59]

LGTM
To be clear, is this a workaround about the clearkeycdm adapter?

In all honesty there is a thing that worries me a bit about the behavior of
compiler/linker here (NOT about this CL, which is fine anyways).
I though that compiler/linker should never inline a function that is marked as
globally visible as that breaks symbol interposition.
Essentially, it seems to me that the compiler/linker here is taking the liberty
of making some functions "-Bsymbolic"
I am not brave enough to call this a linker "bug", because I never understood
what is expected loader behavior and what just happens to be the way linux
linker has been implemented.
But maybe this is what the bug you linked is talking about (sorry I am super
backlogged today, I tried to quickly glance to it, but couldn't figure out if
that is talking about the same problem)

https://codereview.chromium.org/2748083004/diff/1/base/allocator/allocator_sh...
File base/allocator/allocator_shim_override_glibc_weak_symbols.h (right):

https://codereview.chromium.org/2748083004/diff/1/base/allocator/allocator_sh...
base/allocator/allocator_shim_override_glibc_weak_symbols.h:59:
__attribute__((visibility("default"))) void* (
Why are you changing this?
If is just about performance I wouldn't bother doing this.
These hooks are really safety belts for very odd code that we should hit
extremely rarely.
The reason of this entire file is an ancient bug about some RedHat6 libraries
that were relying directly on __glibc_malloc (so bypassing the malloc() symbol
redefinition).
i just didn't feel brave enough to take the risk of finding out if the bug was
fixed and just kept these functions when writing the shim.

In other words, I don't think anybody would realize even if you put a
usleep(50000) here :)

[pcc1, 2017-03-15 16:20:52]

On 2017/03/15 11:42:59, Primiano Tucci (slow - perf) wrote:
> LGTM
> To be clear, is this a workaround about the clearkeycdm adapter?

Not only that but libppapi_tests.so and a bunch of other test DSOs.

> In all honesty there is a thing that worries me a bit about the behavior of
> compiler/linker here (NOT about this CL, which is fine anyways).
> I though that compiler/linker should never inline a function that is marked as
> globally visible as that breaks symbol interposition.
> Essentially, it seems to me that the compiler/linker here is taking the
liberty
> of making some functions "-Bsymbolic"
> I am not brave enough to call this a linker "bug", because I never understood
> what is expected loader behavior and what just happens to be the way linux
> linker has been implemented.
> But maybe this is what the bug you linked is talking about (sorry I am super
> backlogged today, I tried to quickly glance to it, but couldn't figure out if
> that is talking about the same problem)

Yes, that is what the bug is about. This is something where LLVM has
historically diverged from GCC in that the optimizer will assume that symbol
interposition can never happen.

>
https://codereview.chromium.org/2748083004/diff/1/base/allocator/allocator_sh...
> File base/allocator/allocator_shim_override_glibc_weak_symbols.h (right):
> 
>
https://codereview.chromium.org/2748083004/diff/1/base/allocator/allocator_sh...
> base/allocator/allocator_shim_override_glibc_weak_symbols.h:59:
> __attribute__((visibility("default"))) void* (
> Why are you changing this?
> If is just about performance I wouldn't bother doing this.
> These hooks are really safety belts for very odd code that we should hit
> extremely rarely.
> The reason of this entire file is an ancient bug about some RedHat6 libraries
> that were relying directly on __glibc_malloc (so bypassing the malloc() symbol
> redefinition).
> i just didn't feel brave enough to take the risk of finding out if the bug was
> fixed and just kept these functions when writing the shim.
> 
> In other words, I don't think anybody would realize even if you put a
> usleep(50000) here :)

I needed to change this because the noinline attribute can only be applied to
functions.

[pcc1, 2017-03-15 16:21:04]

The CQ bit was checked by pcc@chromium.org

[commit-bot: I haz the power, 2017-03-15 16:21:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 16:33:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489594864919340, "parent_rev":
"ac3c520f3184bcb73512038b93957fd25e6977a0", "commit_rev":
"5ba86e8e4101bf5cd483c0cc6da5264db3bc2464"}

[commit-bot: I haz the power, 2017-03-15 16:34:24]

Description was changed from

==========
Set noinline attribute on exported shim layer functions.

If an exported symbol is linked into a DSO, it may be preempted by a
definition in the main executable. If this happens to an allocator symbol, it
will mean that the DSO will use the main executable's allocator. This is
normally relatively harmless -- regular allocations should all use the same
allocator, but if the DSO tries to hook the allocator it will not see any
allocations.

However, if LLVM LTO is enabled, the compiler may inline the shim layer
symbols into callers. The end result is that allocator calls in DSOs may use
either the main executable's allocator or the DSO's allocator, depending on
whether the call was inlined. This is arguably a bug in LLVM caused by its
somewhat irregular handling of symbol interposition (see llvm.org/PR23501).
To work around the bug we use noinline to prevent the symbols from being
inlined.

In the long run we probably want to avoid linking the allocator bits into
DSOs altogether. This will save a little space and stop giving DSOs the false
impression that they can hook the allocator.

BUG=617732
R=primiano@chromium.org
==========

to

==========
Set noinline attribute on exported shim layer functions.

If an exported symbol is linked into a DSO, it may be preempted by a
definition in the main executable. If this happens to an allocator symbol, it
will mean that the DSO will use the main executable's allocator. This is
normally relatively harmless -- regular allocations should all use the same
allocator, but if the DSO tries to hook the allocator it will not see any
allocations.

However, if LLVM LTO is enabled, the compiler may inline the shim layer
symbols into callers. The end result is that allocator calls in DSOs may use
either the main executable's allocator or the DSO's allocator, depending on
whether the call was inlined. This is arguably a bug in LLVM caused by its
somewhat irregular handling of symbol interposition (see llvm.org/PR23501).
To work around the bug we use noinline to prevent the symbols from being
inlined.

In the long run we probably want to avoid linking the allocator bits into
DSOs altogether. This will save a little space and stop giving DSOs the false
impression that they can hook the allocator.

BUG=617732
R=primiano@chromium.org

Review-Url: https://codereview.chromium.org/2748083004
Cr-Commit-Position: refs/heads/master@{#457103}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ba86e8e4101bf5cd483c0cc6da5...
==========

[commit-bot: I haz the power, 2017-03-15 16:34:25]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/5ba86e8e4101bf5cd483c0cc6da5...

[Primiano Tucci (use gerrit), 2017-03-15 16:50:43]

On 2017/03/15 16:20:52, pcc1 wrote:
> On 2017/03/15 11:42:59, Primiano Tucci (slow - perf) wrote:
> > LGTM
> > To be clear, is this a workaround about the clearkeycdm adapter?
> 
> Not only that but libppapi_tests.so and a bunch of other test DSOs.
> 
> > In all honesty there is a thing that worries me a bit about the behavior of
> > compiler/linker here (NOT about this CL, which is fine anyways).
> > I though that compiler/linker should never inline a function that is marked
as
> > globally visible as that breaks symbol interposition.
> > Essentially, it seems to me that the compiler/linker here is taking the
> liberty
> > of making some functions "-Bsymbolic"
> > I am not brave enough to call this a linker "bug", because I never
understood
> > what is expected loader behavior and what just happens to be the way linux
> > linker has been implemented.
> > But maybe this is what the bug you linked is talking about (sorry I am super
> > backlogged today, I tried to quickly glance to it, but couldn't figure out
if
> > that is talking about the same problem)
> 
> Yes, that is what the bug is about. This is something where LLVM has
> historically diverged from GCC in that the optimizer will assume that symbol
> interposition can never happen.

Thanks for spoiling yet another thing I though I could rely on :)

> 
> >
>
https://codereview.chromium.org/2748083004/diff/1/base/allocator/allocator_sh...
> > File base/allocator/allocator_shim_override_glibc_weak_symbols.h (right):
> > 
> >
>
https://codereview.chromium.org/2748083004/diff/1/base/allocator/allocator_sh...
> > base/allocator/allocator_shim_override_glibc_weak_symbols.h:59:
> > __attribute__((visibility("default"))) void* (
> > Why are you changing this?
> > If is just about performance I wouldn't bother doing this.
> > These hooks are really safety belts for very odd code that we should hit
> > extremely rarely.
> > The reason of this entire file is an ancient bug about some RedHat6
libraries
> > that were relying directly on __glibc_malloc (so bypassing the malloc()
symbol
> > redefinition).
> > i just didn't feel brave enough to take the risk of finding out if the bug
was
> > fixed and just kept these functions when writing the shim.
> > 
> > In other words, I don't think anybody would realize even if you put a
> > usleep(50000) here :)
> 
> I needed to change this because the noinline attribute can only be applied to
> functions.

Ah right. ack.