If the 'asan' GYP variable is set to 1 on Mac, do not strip the __asan_init symbol from the binar...

If the 'asan' GYP variable is set to 1 on Mac, do not strip the __asan_init symbol from the binaries.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99371

[Alexander Potapenko, 2011-09-02 11:33:45]

Hope I'm finally there.
Can you please take a look?

[Mark Mentovai, 2011-09-02 13:37:21]

LG otherwise.

http://codereview.chromium.org/7828043/diff/7/build/asan.saves
File build/asan.saves (right):

http://codereview.chromium.org/7828043/diff/7/build/asan.saves#newcode5
build/asan.saves:5: # This file lists symbols that should not be stripped from
the binaries built
This is not in a mac directory, so the comment should indicate that this file is
Mac-specific. That way, someone who just finds the file will be able to tell
exactly what it’s for.

http://codereview.chromium.org/7828043/diff/7/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7828043/diff/7/build/common.gypi#newcode1739
build/common.gypi:1739: 'conditions': [
There is no way to disable asan on a per-target basis, right?

If you wanted for there to be a way to do so, this would become
target_conditions.

http://codereview.chromium.org/7828043/diff/7/chrome/app/app_asan.saves
File chrome/app/app_asan.saves (right):

http://codereview.chromium.org/7828043/diff/7/chrome/app/app_asan.saves#newcode7
chrome/app/app_asan.saves:7: #
(http://dev.chromium.org/developers/testing/addresssanitizer).
This comment should refer back to chrome/app/app.saves so that people know the
difference between this file and that one.

It should also refer to build/asan.saves so that people know the difference
between this file and THAT one.

http://codereview.chromium.org/7828043/diff/7/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7828043/diff/7/chrome/chrome.gyp#newcode1043
chrome/chrome.gyp:1043: 'CHROMIUM_STRIP_SAVE_FILE': 'app/app_asan.saves',
This also deserves a comment saying that it replaces the outer definition of
CHROMIUM_STRIP_SAVE_FILE.

http://codereview.chromium.org/7828043/diff/7/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7828043/diff/7/chrome/chrome_exe.gypi#newcode84
chrome/chrome_exe.gypi:84: 'CHROMIUM_STRIP_SAVE_FILE': 'app/app_asan.saves',
Ditto.

[Alexander Potapenko, 2011-09-02 14:33:42]

I'll submit the CL when the try job passes.

http://codereview.chromium.org/7828043/diff/7/build/asan.saves
File build/asan.saves (right):

http://codereview.chromium.org/7828043/diff/7/build/asan.saves#newcode5
build/asan.saves:5: # This file lists symbols that should not be stripped from
the binaries built
On 2011/09/02 13:37:21, Mark Mentovai wrote:
> This is not in a mac directory, so the comment should indicate that this file
is
> Mac-specific. That way, someone who just finds the file will be able to tell
> exactly what it’s for.

Actually chrome/app/ is not a mac directory, but you're right.
Done

http://codereview.chromium.org/7828043/diff/7/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7828043/diff/7/build/common.gypi#newcode1739
build/common.gypi:1739: 'conditions': [
On 2011/09/02 13:37:21, Mark Mentovai wrote:
> There is no way to disable asan on a per-target basis, right?
> 
No, there is not. If, say, base/ is instrumented with ASan, targets depending on
it should be built with ASan as well, otherwise they'll crash at startup.

http://codereview.chromium.org/7828043/diff/7/chrome/app/app_asan.saves
File chrome/app/app_asan.saves (right):

http://codereview.chromium.org/7828043/diff/7/chrome/app/app_asan.saves#newcode7
chrome/app/app_asan.saves:7: #
(http://dev.chromium.org/developers/testing/addresssanitizer).
On 2011/09/02 13:37:21, Mark Mentovai wrote:
> This comment should refer back to chrome/app/app.saves so that people know the
> difference between this file and that one.
> 
> It should also refer to build/asan.saves so that people know the difference
> between this file and THAT one.

Done.

http://codereview.chromium.org/7828043/diff/7/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7828043/diff/7/chrome/chrome.gyp#newcode1043
chrome/chrome.gyp:1043: 'CHROMIUM_STRIP_SAVE_FILE': 'app/app_asan.saves',
On 2011/09/02 13:37:21, Mark Mentovai wrote:
> This also deserves a comment saying that it replaces the outer definition of
> CHROMIUM_STRIP_SAVE_FILE.

Done.

http://codereview.chromium.org/7828043/diff/7/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7828043/diff/7/chrome/chrome_exe.gypi#newcode84
chrome/chrome_exe.gypi:84: 'CHROMIUM_STRIP_SAVE_FILE': 'app/app_asan.saves',
On 2011/09/02 13:37:21, Mark Mentovai wrote:
> Ditto.

Done.

[Mark Mentovai, 2011-09-02 14:38:14]

http://codereview.chromium.org/7828043/diff/7/build/asan.saves
File build/asan.saves (right):

http://codereview.chromium.org/7828043/diff/7/build/asan.saves#newcode5
build/asan.saves:5: # This file lists symbols that should not be stripped from
the binaries built
Alexander Potapenko wrote:
> Actually chrome/app/ is not a mac directory, but you're right.

Right, the point was that since the existing file in chrome/app isn’t in a
Mac-specific directory, it ha a comment (already) stating that it’s
Mac-specific. I only said “mac directory” because such a comment would probably
be irrelevant if the file was already in a mac directory.

http://codereview.chromium.org/7828043/diff/5002/build/asan.saves
File build/asan.saves (right):

http://codereview.chromium.org/7828043/diff/5002/build/asan.saves#newcode5
build/asan.saves:5: # This file lists symbols that should not be stripped by
XCode from the binaries
Xcode has a lowercase c.

http://codereview.chromium.org/7828043/diff/5002/chrome/app/app_asan.saves
File chrome/app/app_asan.saves (right):

http://codereview.chromium.org/7828043/diff/5002/chrome/app/app_asan.saves#ne...
chrome/app/app_asan.saves:9: # The list of browser-specific symbols should be
the same as that
“Browser” is the wrong term here, it’s too specific. As the comment above
indicates, this file is used for the browser and helpers.

The same applies to the use of “browser” on line 14.

[_com_google_glider, 2011-09-02 14:48:53]

Uploaded patch set 4

http://codereview.chromium.org/7828043/diff/7/build/asan.saves
File build/asan.saves (right):

http://codereview.chromium.org/7828043/diff/7/build/asan.saves#newcode5
build/asan.saves:5: # This file lists symbols that should not be stripped from
the binaries built
Actually chrome/app/ is not a mac directory either, but, yes, you're right.
Done.

http://codereview.chromium.org/7828043/diff/7/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7828043/diff/7/build/common.gypi#newcode1739
build/common.gypi:1739: 'conditions': [
On 2011/09/02 13:37:21, Mark Mentovai wrote:
> There is no way to disable asan on a per-target basis, right?

No, there will not. If, say, base/ is instrumented using ASan, the targets
depending on it should be built with ASan as well, otherwise they'll crash at
startup.

http://codereview.chromium.org/7828043/diff/5002/build/asan.saves
File build/asan.saves (right):

http://codereview.chromium.org/7828043/diff/5002/build/asan.saves#newcode5
build/asan.saves:5: # This file lists symbols that should not be stripped by
XCode from the binaries
On 2011/09/02 14:38:14, Mark Mentovai wrote:
> Xcode has a lowercase c.

Done.

http://codereview.chromium.org/7828043/diff/5002/chrome/app/app_asan.saves
File chrome/app/app_asan.saves (right):

http://codereview.chromium.org/7828043/diff/5002/chrome/app/app_asan.saves#ne...
chrome/app/app_asan.saves:9: # The list of browser-specific symbols should be
the same as that
On 2011/09/02 14:38:14, Mark Mentovai wrote:
> “Browser” is the wrong term here, it’s too specific. As the comment above
> indicates, this file is used for the browser and helpers.
> 
> The same applies to the use of “browser” on line 14.

Chromium applications/Chromium?

[Mark Mentovai, 2011-09-02 14:56:24]

One minor nit of a letter to remove, and LGTM

http://codereview.chromium.org/7828043/diff/15/chrome/app/app_asan.saves
File chrome/app/app_asan.saves (right):

http://codereview.chromium.org/7828043/diff/15/chrome/app/app_asan.saves#newc...
chrome/app/app_asan.saves:9: # The list of Chromium applications-specific
symbols should be the same as that
Chromium application (no s)

[_com_google_glider, 2011-09-02 15:06:08]

http://codereview.chromium.org/7828043/diff/15/chrome/app/app_asan.saves
File chrome/app/app_asan.saves (right):

http://codereview.chromium.org/7828043/diff/15/chrome/app/app_asan.saves#newc...
chrome/app/app_asan.saves:9: # The list of Chromium applications-specific
symbols should be the same as that
On 2011/09/02 14:56:24, Mark Mentovai wrote:
> Chromium application (no s)

Done.

[Mark Mentovai, 2011-09-02 15:41:40]

LGTM