allocator: Fix function type mismatch in allocator function definitions.

allocator: Fix function type mismatch in allocator function definitions.

CL 2697123007 added a context argument to most of the shim allocator
functions. This means that it is no longer valid to define (say) malloc as
an alias of ShimMalloc, because their function types no longer match.

This change defines the allocator functions as real functions that call the
shims passing nullptr as the context.

Found with -fsanitize=cfi-icall.

R=primiano@chromium.org
BUG=696986, 601497
Review-Url: https://codereview.chromium.org/2720703004
Cr-Commit-Position: refs/heads/master@{#453900}
Committed: https://chromium.googlesource.com/chromium/src/+/d4809ef025d50acb50489134273e13444252a8e6

[Primiano Tucci (use gerrit), 2017-02-28 12:18:49]

OMG I am extremely grateful that you spotted this Peter! This was extremely
subtle. 
Bless all these new compiler checks!

Now, from a performance viewpoint, I would be a bit happier if we (read:
erikchen@ :P) just restored the original situation (just filed crbug.com/696986
with a braindump there) as we actually don't need that context in Linux.

However that might take a while or just be harder than what I am imagining,
let's see what Erik says on the bug.

pcc: Is this breaking some bug or workflow? Or did you just find by accident?
If this is blocking something on your side and for any reason my suggestion in
crbug.com/696986 takes too long, I'm happy to LG this (% the small comment
below) and then having me/erik re-address the performance parts separately.

Thanks again for filing this and even taking the time to file a CL!

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

https://codereview.chromium.org/2720703004/diff/1/base/allocator/allocator_sh...
base/allocator/allocator_shim_override_glibc_weak_symbols.h:79:
SHIM_ALWAYS_EXPORT void* __libc_malloc(size_t size) SHIM_ALIAS_SYMBOL(malloc);
honestly if we go down this road (i.e. adding an extra explicit hop) I'd prefer
this to just be an explicit functon call (like the other file) instead of this
alias.
Rationale:
- this alias won't really reduce one hop, just reuse the same hop below
- this makes this code very hard to read and reason about, as one have to
realize that "malloc" is actually redefined in the other file and is just
jumping into shimmalloc.

[pcc1, 2017-02-28 20:12:42]

> OMG I am extremely grateful that you spotted this Peter! This was extremely
> subtle. 
> Bless all these new compiler checks!
> 
> Now, from a performance viewpoint, I would be a bit happier if we (read:
> erikchen@ :P) just restored the original situation (just filed
crbug.com/696986
> with a braindump there) as we actually don't need that context in Linux.
> 
> However that might take a while or just be harder than what I am imagining,
> let's see what Erik says on the bug.
> 
> pcc: Is this breaking some bug or workflow? Or did you just find by accident?
> If this is blocking something on your side and for any reason my suggestion in
> crbug.com/696986 takes too long, I'm happy to LG this (% the small comment
> below) and then having me/erik re-address the performance parts separately.

I'm trying to introduce a build system flag that turns on -fsanitize=cfi-icall
and this issue prevents me from running test suites with that flag enabled.

> Thanks again for filing this and even taking the time to file a CL!
> 
>
https://codereview.chromium.org/2720703004/diff/1/base/allocator/allocator_sh...
> File base/allocator/allocator_shim_override_glibc_weak_symbols.h (right):
> 
>
https://codereview.chromium.org/2720703004/diff/1/base/allocator/allocator_sh...
> base/allocator/allocator_shim_override_glibc_weak_symbols.h:79:
> SHIM_ALWAYS_EXPORT void* __libc_malloc(size_t size) SHIM_ALIAS_SYMBOL(malloc);
> honestly if we go down this road (i.e. adding an extra explicit hop) I'd
prefer
> this to just be an explicit functon call (like the other file) instead of this
> alias.
> Rationale:
> - this alias won't really reduce one hop, just reuse the same hop below
> - this makes this code very hard to read and reason about, as one have to
> realize that "malloc" is actually redefined in the other file and is just
> jumping into shimmalloc.

Can this problem be addressed with comments? I could do as you ask, but we will
end up with another inlined copy of the shim function (and it could inhibit
inlining as the compiler would realise that it would need to emit multiple
copies of the allocator functions).

[pcc1, 2017-02-28 22:27:35]

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

https://codereview.chromium.org/2720703004/diff/1/base/allocator/allocator_sh...
base/allocator/allocator_shim_override_glibc_weak_symbols.h:79:
SHIM_ALWAYS_EXPORT void* __libc_malloc(size_t size) SHIM_ALIAS_SYMBOL(malloc);
Done, as discussed on crbug.com/696986

[pcc1, 2017-02-28 22:27:52]

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

[commit-bot: I haz the power, 2017-02-28 22:28:31]

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-02-28 23:03:58]

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

[commit-bot: I haz the power, 2017-02-28 23:03:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Primiano Tucci (use gerrit), 2017-03-01 10:07:32]

Description was changed from

==========
allocator: Fix function type mismatch in allocator function definitions.

CL 2697123007 added a context argument to most of the shim allocator
functions. This means that it is no longer valid to define (say) malloc as
an alias of ShimMalloc, because their function types no longer match.

This change defines the allocator functions as real functions that call the
shims passing nullptr as the context.

Found with -fsanitize=cfi-icall.

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

to

==========
allocator: Fix function type mismatch in allocator function definitions.

CL 2697123007 added a context argument to most of the shim allocator
functions. This means that it is no longer valid to define (say) malloc as
an alias of ShimMalloc, because their function types no longer match.

This change defines the allocator functions as real functions that call the
shims passing nullptr as the context.

Found with -fsanitize=cfi-icall.

R=primiano@chromium.org
BUG=696986,601497
==========

[Primiano Tucci (use gerrit), 2017-03-01 10:10:27]

awesome, LGTM. I checked the two red bots and the failures seem completely
unrelated (a javac error and a flake unittest).
I took the liberty to update the BUG= in the cl description.

[Primiano Tucci (use gerrit), 2017-03-01 10:11:12]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2017-03-01 10:11:27]

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-01 11:01:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488363072484400,
"parent_rev": "25fe8839796345d1641151d22ae07dce10fd372e", "commit_rev":
"d4809ef025d50acb50489134273e13444252a8e6"}

[commit-bot: I haz the power, 2017-03-01 11:02:38]

Description was changed from

==========
allocator: Fix function type mismatch in allocator function definitions.

CL 2697123007 added a context argument to most of the shim allocator
functions. This means that it is no longer valid to define (say) malloc as
an alias of ShimMalloc, because their function types no longer match.

This change defines the allocator functions as real functions that call the
shims passing nullptr as the context.

Found with -fsanitize=cfi-icall.

R=primiano@chromium.org
BUG=696986,601497
==========

to

==========
allocator: Fix function type mismatch in allocator function definitions.

CL 2697123007 added a context argument to most of the shim allocator
functions. This means that it is no longer valid to define (say) malloc as
an alias of ShimMalloc, because their function types no longer match.

This change defines the allocator functions as real functions that call the
shims passing nullptr as the context.

Found with -fsanitize=cfi-icall.

R=primiano@chromium.org
BUG=696986,601497

Review-Url: https://codereview.chromium.org/2720703004
Cr-Commit-Position: refs/heads/master@{#453900}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4809ef025d50acb50489134273e...
==========

[commit-bot: I haz the power, 2017-03-01 11:02:38]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d4809ef025d50acb50489134273e...