android: Make 64-bit chromes compile with clang.

android: Make 64-bit chromes compile with clang.

Newer NDKs define snprintf() (and other functions) to a macro when using clang,
to implement a kind of _FORTIFY_SOURCE support.  This means it's impossible to
declare your own base::snprintf(), like base/strings/string_util.h does.  A
future NDK will have a better fortify story that doesn't depend on macros,
but for now build with -Dsnprintf=snprintf to tell the NDK that it shouldn't
define snprintf as a macro.  This will disable fortify for snprintf.

This matters for 64-bit only because we use NDK 21 for 64-bit builds but
we use NDK 16 for 32-bit builds.  NDK 16 never does any fortify stuff, so
while this CL does remove fortify checking for snprintf() (but not for any
other function), 64-bit binaries are still better-protected than 32-bit
binaries.  (And ideally this situation is just temporary.)

With this, x64 and mips64el build fine with clang. With this and
https://codereview.chromium.org/2404193003/, arm64 builds fine with
clang.  I verified that a clang-built arm64 Chromium starts up fine
on my Pixel XL and can navigate to a few websites.

BUG=539781
Committed: https://crrev.com/3ae9e5b283ea81dc1605f70970828064deacba2a
Cr-Commit-Position: refs/heads/master@{#424729}

[Nico, 2016-10-11 21:58:26]

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

[Nico, 2016-10-11 21:58:43]

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

[commit-bot: I haz the power, 2016-10-11 21:58:54]

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, 2016-10-11 22:09:12]

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

[commit-bot: I haz the power, 2016-10-11 22:09:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Nico, 2016-10-11 22:25:13]

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

[commit-bot: I haz the power, 2016-10-11 22:25:34]

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, 2016-10-11 23:35:15]

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

[commit-bot: I haz the power, 2016-10-11 23:35:16]

Dry run: This issue passed the CQ dry run.

[hans, 2016-10-12 07:12:18]

hans@chromium.org changed reviewers:
+ hans@chromium.org

[hans, 2016-10-12 07:12:19]

lgtm but maybe you want someone with more Android skills to sign off

[Nico, 2016-10-12 10:47:41]

On 2016/10/12 07:12:19, hans wrote:
> lgtm but maybe you want someone with more Android skills to sign off

Primiano, want to sign off on this?

[Primiano Tucci (use gerrit), 2016-10-12 12:13:32]

Ok I investigated a bit and it seems that your assumption "64-bit targets build
with NDK 21, 32-bit targets with NDK 16" holds also internally.
It seems that the "modern" targets have only a different sdk level but build
against the same .so, so the use the same NDK.

CC-ing agrieve and jbudorick which are likely to be the only ones who might
touch this in future.

The entire bionic "source fortification" strategy is a bit sad. I am pretty sure
it's going to bite us somewhere else in the future but don't have any better
solution really to work around it.
So, LGTM

[Nico, 2016-10-12 13:33:25]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-10-12 13:33:32]

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, 2016-10-12 13:39:09]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-12 13:40:50]

Description was changed from

==========
android: Make 64-bit chromes compile with clang.

Newer NDKs define snprintf() (and other functions) to a macro when using clang,
to implement a kind of _FORTIFY_SOURCE support.  This means it's impossible to
declare your own base::snprintf(), like base/strings/string_util.h does.  A
future NDK will have a better fortify story that doesn't depend on macros,
but for now build with -Dsnprintf=snprintf to tell the NDK that it shouldn't
define snprintf as a macro.  This will disable fortify for snprintf.

This matters for 64-bit only because we use NDK 21 for 64-bit builds but
we use NDK 16 for 32-bit builds.  NDK 16 never does any fortify stuff, so
while this CL does remove fortify checking for snprintf() (but not for any
other function), 64-bit binaries are still better-protected than 32-bit
binaries.  (And ideally this situation is just temporary.)

With this, x64 and mips64el build fine with clang. With this and
https://codereview.chromium.org/2404193003/, arm64 builds fine with
clang.  I verified that a clang-built arm64 Chromium starts up fine
on my Pixel XL and can navigate to a few websites.

BUG=539781
==========

to

==========
android: Make 64-bit chromes compile with clang.

Newer NDKs define snprintf() (and other functions) to a macro when using clang,
to implement a kind of _FORTIFY_SOURCE support.  This means it's impossible to
declare your own base::snprintf(), like base/strings/string_util.h does.  A
future NDK will have a better fortify story that doesn't depend on macros,
but for now build with -Dsnprintf=snprintf to tell the NDK that it shouldn't
define snprintf as a macro.  This will disable fortify for snprintf.

This matters for 64-bit only because we use NDK 21 for 64-bit builds but
we use NDK 16 for 32-bit builds.  NDK 16 never does any fortify stuff, so
while this CL does remove fortify checking for snprintf() (but not for any
other function), 64-bit binaries are still better-protected than 32-bit
binaries.  (And ideally this situation is just temporary.)

With this, x64 and mips64el build fine with clang. With this and
https://codereview.chromium.org/2404193003/, arm64 builds fine with
clang.  I verified that a clang-built arm64 Chromium starts up fine
on my Pixel XL and can navigate to a few websites.

BUG=539781

Committed: https://crrev.com/3ae9e5b283ea81dc1605f70970828064deacba2a
Cr-Commit-Position: refs/heads/master@{#424729}
==========

[commit-bot: I haz the power, 2016-10-12 13:40:51]

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

[agrieve, 2016-10-12 13:52:26]

On 2016/10/12 12:13:32, Primiano Tucci wrote:
> Ok I investigated a bit and it seems that your assumption "64-bit targets
build
> with NDK 21, 32-bit targets with NDK 16" holds also internally.
> It seems that the "modern" targets have only a different sdk level but build
> against the same .so, so the use the same NDK.
> 
> CC-ing agrieve and jbudorick which are likely to be the only ones who might
> touch this in future.
> 
> The entire bionic "source fortification" strategy is a bit sad. I am pretty
sure
> it's going to bite us somewhere else in the future but don't have any better
> solution really to work around it.
> So, LGTM

I believe the rationale for this is that no 64-bit devices exist that don't run
android 21 or above. We could have ChromeModern target 21+ as well, but don't
want to pay the cost of having to compile the .so twice (unless the wins are
worth it). This setup predates me joining the team, and I have no idea if anyone
took the time to even check the "worth it" part.

[Primiano Tucci (use gerrit), 2016-10-12 14:41:19]

On 2016/10/12 13:52:26, agrieve wrote:
> I believe the rationale for this is that no 64-bit devices exist that don't
run
android 21 or above.
Correct. 64 bit was introduced in L.

> We could have ChromeModern target 21+ as well, but don't want to pay the cost
of having to compile the .so twice (unless the wins are worth it)
I am extremely happy about having to think only about one .so thanks :-)
They (chrome vs modern) are already not identical because we still build both
independently (from the same source) and our build not being fully reproducible,
but this is another story. I can guarantee this itself already causes me lot of
headaches on crash reports.