Link libc++_shared with asan=1 on Android.

Link libc++_shared with asan=1 on Android.

ASan needs to interpose operator new/delete symbols to provide good
allocation/deallocation stacks. This requires shared linking of the
C++ standard library.

BUG=515261
Committed: https://crrev.com/301af9347c32ac621ab06b08eb089f8ee643795f
Cr-Commit-Position: refs/heads/master@{#341675}

[eugenis, 2015-07-30 00:29:33]

eugenis@chromium.org changed reviewers:
+ thakis@chromium.org

[eugenis, 2015-07-30 00:33:14]

To clarify, currently we always build for asan on android with
component="shared_library" exactly for the reason listed in the description.
This causes the apks to include dozens of libraries and, we believe,
significantly slows startup. What we really want is link statically everything
except libc++.

This CL should let us use component="static_library" with asan on android.

[eugenis, 2015-07-30 18:42:47]

On 2015/07/30 00:33:14, eugenis wrote:
> To clarify, currently we always build for asan on android with
> component="shared_library" exactly for the reason listed in the description.
> This causes the apks to include dozens of libraries and, we believe,
> significantly slows startup. What we really want is link statically everything
> except libc++.
> 
> This CL should let us use component="static_library" with asan on android.

Updated a few more conditions to not pack relocations in this new
component=static_library but shared libc++ mode.

[inferno, 2015-08-02 15:13:00]

inferno@chromium.org changed reviewers:
+ simonb@chromium.org, yfriedman@chromium.org

[simonb (inactive), 2015-08-03 12:33:40]

https://codereview.chromium.org/1263713005/diff/20001/build/android/pack_relo...
File build/android/pack_relocations.gypi (right):

https://codereview.chromium.org/1263713005/diff/20001/build/android/pack_relo...
build/android/pack_relocations.gypi:70: ['android_libcpp_library ==
"c++_shared"', {
Any reason for just the one test here, rather than 'and' of both old and new as
done elsewhere?  (Also, strip_native_libraries.gypi.)

[eugenis, 2015-08-03 18:14:25]

On 2015/08/03 12:33:40, simonb wrote:
>
https://codereview.chromium.org/1263713005/diff/20001/build/android/pack_relo...
> File build/android/pack_relocations.gypi (right):
> 
>
https://codereview.chromium.org/1263713005/diff/20001/build/android/pack_relo...
> build/android/pack_relocations.gypi:70: ['android_libcpp_library ==
> "c++_shared"', {
> Any reason for just the one test here, rather than 'and' of both old and new
as
> done elsewhere?  (Also, strip_native_libraries.gypi.)

The new condition is equivalent to "asan and old_condition".
I think just checking for android_libcpp_library is better because at the moment
it is the only "system library" that needs all this special treatment. I'll
remove the "components" condition in the other two places.

[eugenis, 2015-08-03 18:20:39]

On 2015/08/03 18:14:25, eugenis wrote:
> On 2015/08/03 12:33:40, simonb wrote:
> >
>
https://codereview.chromium.org/1263713005/diff/20001/build/android/pack_relo...
> > File build/android/pack_relocations.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1263713005/diff/20001/build/android/pack_relo...
> > build/android/pack_relocations.gypi:70: ['android_libcpp_library ==
> > "c++_shared"', {
> > Any reason for just the one test here, rather than 'and' of both old and new
> as
> > done elsewhere?  (Also, strip_native_libraries.gypi.)
> 
> The new condition is equivalent to "asan and old_condition".
> I think just checking for android_libcpp_library is better because at the
moment
> it is the only "system library" that needs all this special treatment. I'll
> remove the "components" condition in the other two places.

Actually, no, those two cases are specifically a workaround for "component
build" issues that happens to be necessary in the shared_libc++ case as well, so
it's probably best to keep both conditions even if one is redundant. WDYT?

[Nico, 2015-08-03 18:21:30]

I don't love this CL; it's going to confuse people when to use
component==shared_library and when to use android_libcpp_library. If you must
add another variable, can you name it after what's interesting here
(android_must_copy_libcpp_so or whatever it is)?

Also, do you need to do the same in gn files too?

[eugenis, 2015-08-03 21:12:24]

On 2015/08/03 18:21:30, Nico (hiding) wrote:
> I don't love this CL; it's going to confuse people when to use
> component==shared_library and when to use android_libcpp_library. If you must
> add another variable, can you name it after what's interesting here
> (android_must_copy_libcpp_so or whatever it is)?

Done. PTAL.
 
> Also, do you need to do the same in gn files too?

GN does not work for ASan on Android at the moment.

[eugenis, 2015-08-03 21:24:22]

On 2015/08/03 21:12:24, eugenis wrote:
> On 2015/08/03 18:21:30, Nico (hiding) wrote:
> > I don't love this CL; it's going to confuse people when to use
> > component==shared_library and when to use android_libcpp_library. If you
must
> > add another variable, can you name it after what's interesting here
> > (android_must_copy_libcpp_so or whatever it is)?
> 
> Done. PTAL.
>  
> > Also, do you need to do the same in gn files too?
> 
> GN does not work for ASan on Android at the moment.

In fact, GN does not even support Clang on Android.

[Nico, 2015-08-03 23:02:26]

lgtm, thanks, that looks much better imho.

https://codereview.chromium.org/1263713005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1263713005/diff/40001/build/common.gypi#newco...
build/common.gypi:1719: ['component=="shared_library" or asan==1', {
Add some kind of

  # ASan needs to dynamically link to libc++ even in static builds so that it
can interopose operator new

kind of comment here please.

[eugenis, 2015-08-03 23:21:24]

https://codereview.chromium.org/1263713005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1263713005/diff/40001/build/common.gypi#newco...
build/common.gypi:1719: ['component=="shared_library" or asan==1', {
On 2015/08/03 23:02:25, Nico (hiding) wrote:
> Add some kind of
> 
>   # ASan needs to dynamically link to libc++ even in static builds so that it
> can interopose operator new
> 
> kind of comment here please.

Done.

[eugenis, 2015-08-03 23:21:29]

The CQ bit was checked by eugenis@chromium.org

[eugenis, 2015-08-03 23:21:29]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1263713005/#ps60001
(title: "added a comment")

[commit-bot: I haz the power, 2015-08-03 23:21:59]

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

[commit-bot: I haz the power, 2015-08-04 00:36:27]

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

[commit-bot: I haz the power, 2015-08-04 00:36:27]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[eugenis, 2015-08-04 00:37:21]

The CQ bit was checked by eugenis@chromium.org

[commit-bot: I haz the power, 2015-08-04 00:38:13]

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

[commit-bot: I haz the power, 2015-08-04 04:58:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-04 04:59:43]

Patchset 4 (id:??) landed as
https://crrev.com/301af9347c32ac621ab06b08eb089f8ee643795f
Cr-Commit-Position: refs/heads/master@{#341675}