Define _FORTIFY_SOURCE=2 under the same conditions in gn as gyp.

Define _FORTIFY_SOURCE=2 under the same conditions in gn as gyp.

Committed: https://crrev.com/614906ed827f53299ce6de6bbd66c63b55d313ff
Cr-Commit-Position: refs/heads/master@{#330685}

[Sam McNally, 2015-05-19 08:19:34]

sammc@chromium.org changed reviewers:
+ dpranke@chromium.org

[Sam McNally, 2015-05-19 08:19:35]

The corresponding gyp is at
https://code.google.com/p/chromium/codesearch/#chromium/src/build/common.gypi...

[Dirk Pranke, 2015-05-19 19:53:30]

https://codereview.chromium.org/1127993006/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1127993006/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:665: if (!using_sanitizer && (!is_linux ||
!is_clang || is_official_build)) {
I can see this matches the logic in GYP, but trying to parse this to figure out
when this actually will get applied is a little weird:

!is_linux => mac or android
!is_clang => android

so we want this on android or on official (non-win) builds; is that right? This
seems like a confusing way to write this, and it's not clear to me whether we
actually want to set this in clang builds (which most of the official builds
will be?)

[Sam McNally, 2015-05-20 01:40:29]

https://codereview.chromium.org/1127993006/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1127993006/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:665: if (!using_sanitizer && (!is_linux ||
!is_clang || is_official_build)) {
On 2015/05/19 19:53:30, Dirk Pranke wrote:
> I can see this matches the logic in GYP, but trying to parse this to figure
out
> when this actually will get applied is a little weird:
> 
> !is_linux => mac or android
> !is_clang => android
> 
> so we want this on android or on official (non-win) builds; is that right?
This
> seems like a confusing way to write this, and it's not clear to me whether we
> actually want to set this in clang builds (which most of the official builds
> will be?)

My understanding from the comments is that we want this always on for
non-Windows, but clang doesn't work with this on recent versions of linux. That
special case then has its own special case because the bots used for official
builds are running Ubuntu 12.04 which is old enough to avoid the clang bug.

How about !(is_linux && is_clang && !is_official_build) instead? Or one step
further: !(using_sanitizer || (is_linux && is_clang && !is_official_build)).
These make the "on by default unless..." intent a bit clearer.

[Dirk Pranke, 2015-05-20 02:00:25]

dpranke@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, thakis@chromium.org

[Dirk Pranke, 2015-05-20 02:00:25]

My concern is that the comments may have accreted over time and maybe don't make
much sense now.

I'll approve this as-is (since it would be good to have GN match GYP), but can
you file a bug and cc me, phajdan.jr, and thakis@ to confirm that this logic is
correct?

lgtm.

[Sam McNally, 2015-05-20 02:48:06]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2015-05-20 02:48:47]

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

[commit-bot: I haz the power, 2015-05-20 02:53:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-20 02:54:25]

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

[Nico, 2015-05-21 19:19:55]

https://codereview.chromium.org/1127993006/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1127993006/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:665: if (!using_sanitizer && (!is_linux ||
!is_clang || is_official_build)) {
On 2015/05/20 01:40:29, Sam McNally wrote:
> On 2015/05/19 19:53:30, Dirk Pranke wrote:
> > I can see this matches the logic in GYP, but trying to parse this to figure
> out
> > when this actually will get applied is a little weird:
> > 
> > !is_linux => mac or android
> > !is_clang => android
> > 
> > so we want this on android or on official (non-win) builds; is that right?
> This
> > seems like a confusing way to write this, and it's not clear to me whether
we
> > actually want to set this in clang builds (which most of the official builds
> > will be?)
> 
> My understanding from the comments is that we want this always on for
> non-Windows, but clang doesn't work with this on recent versions of linux.
That
> special case then has its own special case because the bots used for official
> builds are running Ubuntu 12.04 which is old enough to avoid the clang bug.

Yes, this is correct.

> How about !(is_linux && is_clang && !is_official_build) instead? Or one step
> further: !(using_sanitizer || (is_linux && is_clang && !is_official_build)).
> These make the "on by default unless..." intent a bit clearer.