Set the default ASan options for executables built with ASan on Linux.

Set the default ASan options for executables built with ASan on Linux.

This CL introduces a module, base/debug/sanitizer_options.cc, which will override
the defaults for various dynamic tools (only ASan at this moment). For every executable
built with a dynamic tool this module will be linked into that executable, providing
weak functions to be called by the tool.

The existing declaration of __asan_default_options() in chrome/app/chrome_exe_main_gtk.cc
has been moved into sanitizer_options.cc (now every binary built with GOOGLE_CHROME_BUILD=1
will have the same options as google-chrome-asan.
The existing declaration of __asan_default_options() in chrome/nacl/nacl_helper_linux.cc
has been kept as is, but we had to remove -Wl,-u_sanitizer_options_link_helper to avoid picking sanitizer_options.cc.

GYP changes for OSX and iOS will be committed separately.

BUG=302040
R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234207

[Alexander Potapenko, 2013-10-25 09:06:21]

Nico, can you please take a look?

[_com_google_glider, 2013-10-30 04:44:30]

Ping?
On Oct 25, 2013 1:06 PM, <glider@chromium.org> wrote:

> Reviewers: Nico,
>
> Message:
> Nico, can you please take a look?
>
> Description:
> Set the default ASan options for executables built with ASan on Linux.
>
> This CL introduces a module, base/debug/sanitizer_options.**cc, which will
> override
> the defaults for various dynamic tools (only ASan at this moment). For
> every
> executable
> built with a dynamic tool this module will be linked into that executable,
> providing
> weak functions to be called by the tool.
>
> The existing declaration of __asan_default_options() in
> chrome/app/chrome_exe_main_**gtk.cc
> has been moved into sanitizer_options.cc (now every binary built with
> GOOGLE_CHROME_BUILD=1
> will have the same options as google-chrome-asan.
> The existing declaration of __asan_default_options() in
> chrome/nacl/nacl_helper_linux.**cc
> has been kept as is, but we had to remove -Wl,-u_sanitizer_options_link_**
> helper
> to avoid picking sanitizer_options.cc.
>
> GYP changes for OSX and iOS will be committed separately.
>
> BUG=302040
>
> Please review this at
https://codereview.chromium.**org/25687005/<https://codereview.chromium.org/2...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files (+83, -25 lines):
>   M     base/base.gyp
>   A     base/debug/sanitizer_options.**cc
>   M     build/common.gypi
>   M     chrome/app/chrome_exe_main_**gtk.cc
>   M     chrome/nacl.gypi
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-10-30 04:50:37]

Cool!

https://codereview.chromium.org/25687005/diff/185001/base/debug/sanitizer_opt...
File base/debug/sanitizer_options.cc (right):

https://codereview.chromium.org/25687005/diff/185001/base/debug/sanitizer_opt...
base/debug/sanitizer_options.cc:32: const char *kAsanDefaultOptions =
const char kAsanDefaultOptions[]?

https://codereview.chromium.org/25687005/diff/185001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/25687005/diff/185001/build/common.gypi#newcod...
build/common.gypi:3291: '<(DEPTH)/base/base.gyp:sanitizer_options',
Do you need the export bit if everything depends on this already?

https://codereview.chromium.org/25687005/diff/185001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

https://codereview.chromium.org/25687005/diff/185001/chrome/nacl.gypi#newcode29
chrome/nacl.gypi:29: # base/debug/sanitizer_options.cc.
why not? (say in the comment)

[Alexander Potapenko, 2013-10-30 14:25:55]

https://codereview.chromium.org/25687005/diff/185001/base/debug/sanitizer_opt...
File base/debug/sanitizer_options.cc (right):

https://codereview.chromium.org/25687005/diff/185001/base/debug/sanitizer_opt...
base/debug/sanitizer_options.cc:32: const char *kAsanDefaultOptions =
On 2013/10/30 04:50:37, Nico wrote:
> const char kAsanDefaultOptions[]?

Done.

https://codereview.chromium.org/25687005/diff/185001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/25687005/diff/185001/build/common.gypi#newcod...
build/common.gypi:3291: '<(DEPTH)/base/base.gyp:sanitizer_options',
On 2013/10/30 04:50:37, Nico wrote:
> Do you need the export bit if everything depends on this already?

Probably no. Removed it.

https://codereview.chromium.org/25687005/diff/185001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

https://codereview.chromium.org/25687005/diff/185001/chrome/nacl.gypi#newcode29
chrome/nacl.gypi:29: # base/debug/sanitizer_options.cc.
On 2013/10/30 04:50:37, Nico wrote:
> why not? (say in the comment)

Done.

[Nico, 2013-10-30 14:29:47]

https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_opt...
File base/debug/sanitizer_options.cc (right):

https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_opt...
base/debug/sanitizer_options.cc:13: // -Wl,-u_sanitizer_options_link_helper
From the man page of `man ld` on OS X:

     -u symbol_name
                 Specified that symbol symbol_name must be defined for the link
to succeed.  This is useful to force selected functions to be loaded from a
static library.


Is this what you mean?

https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_opt...
base/debug/sanitizer_options.cc:15: void _sanitizer_options_link_helper() { }
What does the dynamic linker do when loading several dylibs that all define this
symbol?

Do we want to be able to set different asan options for the main executable and
a helper dylib? Should only executables depend on this file's target?

[Alexander Potapenko, 2013-10-30 14:43:19]

https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_opt...
File base/debug/sanitizer_options.cc (right):

https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_opt...
base/debug/sanitizer_options.cc:13: // -Wl,-u_sanitizer_options_link_helper
Yeah, exactly. From `man ld` on Linux:
      -u symbol
       --undefined=symbol
           Force symbol to be entered in the output file as an undefined symbol.
 Doing this may, for example, trigger linking of additional modules from
standard libraries.

https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_opt...
base/debug/sanitizer_options.cc:15: void _sanitizer_options_link_helper() { }
On 2013/10/30 14:29:47, Nico wrote:
> What does the dynamic linker do when loading several dylibs that all define
this
> symbol?
I'm guessing that only one of those symbols will be visible at every single
moment from every lookup scope. A more interesting question is what'll happen to
the __asan_default_options() symbols, but I don't think it should break
anything.

> 
> Do we want to be able to set different asan options for the main executable
and
> a helper dylib? 
No, it's quite hard and unnecessary. Most options affect the behavior of the
whole program.

> Should only executables depend on this file's target?
I'd love to, but IIRC it's impossible to make only certain target types depend
on a target.

[Nico, 2013-10-30 14:50:34]

On Wed, Oct 30, 2013 at 7:43 AM, <glider@chromium.org> wrote:

>
> https://codereview.chromium.**org/25687005/diff/605001/base/**
>
debug/sanitizer_options.cc<https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_options.cc>
> File base/debug/sanitizer_options.**cc (right):
>
> https://codereview.chromium.**org/25687005/diff/605001/base/**
>
debug/sanitizer_options.cc#**newcode13<https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_options.cc#newcode13>
> base/debug/sanitizer_options.**cc:13: //
> -Wl,-u_sanitizer_options_link_**helper
> Yeah, exactly. From `man ld` on Linux:
>       -u symbol
>        --undefined=symbol
>            Force symbol to be entered in the output file as an undefined
> symbol.  Doing this may, for example, trigger linking of additional
> modules from standard libraries.
>
>
> https://codereview.chromium.**org/25687005/diff/605001/base/**
>
debug/sanitizer_options.cc#**newcode15<https://codereview.chromium.org/25687005/diff/605001/base/debug/sanitizer_options.cc#newcode15>
> base/debug/sanitizer_options.**cc:15: void
> _sanitizer_options_link_**helper() { }
> On 2013/10/30 14:29:47, Nico wrote:
>
>> What does the dynamic linker do when loading several dylibs that all
>>
> define this
>
>> symbol?
>>
> I'm guessing that only one of those symbols will be visible at every
> single moment from every lookup scope. A more interesting question is
> what'll happen to the __asan_default_options() symbols, but I don't
> think it should break anything.
>
>
>
>  Do we want to be able to set different asan options for the main
>>
> executable and
>
>> a helper dylib?
>>
> No, it's quite hard and unnecessary. Most options affect the behavior of
> the whole program.
>
>
>  Should only executables depend on this file's target?
>>
> I'd love to, but IIRC it's impossible to make only certain target types
> depend on a target.
>

Oh, so putting this into a '_type=="executable"' in the target_conditions a
few lines up doesn't work?


>
>
https://codereview.chromium.**org/25687005/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2013-10-30 14:58:10]

> 
> Oh, so putting this into a '_type=="executable"' in the target_conditions a
> few lines up doesn't work?
> 

Looks like GYP just doesn't allow to add a dependency in the target_conditions.
I've tried this in https://codereview.chromium.org/11642018/, but it didn't
work.
Neither did it for alextaran@ in his recent patch
(https://codereview.chromium.org/50423003/)

[Nico, 2013-10-30 15:29:43]

On Wed, Oct 30, 2013 at 7:58 AM, <glider@chromium.org> wrote:

>
>
>  Oh, so putting this into a '_type=="executable"' in the target_conditions
>> a
>> few lines up doesn't work?
>>
>
>
> Looks like GYP just doesn't allow to add a dependency in the
> target_conditions.
> I've tried this in
https://codereview.chromium.**org/11642018/<https://codereview.chromium.org/1...,
> but it didn't
> work.
>

Which comment? I vaguely remember something now, but I don't know where in
that review.

(Other bits of gyp succeed checking _type, I wonder what's different there?)


> Neither did it for alextaran@ in his recent patch
>
(https://codereview.chromium.**org/50423003/<https://codereview.chromium.org/5...
> )
>
>
https://codereview.chromium.**org/25687005/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[_com_google_glider, 2013-10-31 10:44:12]

https://codereview.chromium.org/11642018/#msg11

On Wed, Oct 30, 2013 at 7:29 PM, Nico Weber <thakis@chromium.org> wrote:
> On Wed, Oct 30, 2013 at 7:58 AM, <glider@chromium.org> wrote:
>>
>>
>>
>>> Oh, so putting this into a '_type=="executable"' in the target_conditions
>>> a
>>> few lines up doesn't work?
>>
>>
>>
>> Looks like GYP just doesn't allow to add a dependency in the
>> target_conditions.
>> I've tried this in https://codereview.chromium.org/11642018/, but it
>> didn't
>> work.
>
>
> Which comment? I vaguely remember something now, but I don't know where in
> that review.
>
> (Other bits of gyp succeed checking _type, I wonder what's different there?)
>
>>
>> Neither did it for alextaran@ in his recent patch
>> (https://codereview.chromium.org/50423003/)
>>
>> https://codereview.chromium.org/25687005/
>
>



-- 
Alexander Potapenko
Software Engineer
Google Moscow

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2013-11-01 11:34:49]

FYI I've uploaded Patch Set 9, where I tried putting the dependency into a
'_type=="executable"' in the target_conditions.

Here's the error GYP printed:

Traceback (most recent call last):
  File "src/build/gyp_chromium", line 161, in <module>
    sys.exit(gyp.main(args))
  File "/usr/local/google/chrome-asan/src/tools/gyp/pylib/gyp/__init__.py", line
527, in main
    return gyp_main(args)
  File "/usr/local/google/chrome-asan/src/tools/gyp/pylib/gyp/__init__.py", line
512, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File
"/usr/local/google/chrome-asan/src/tools/gyp/pylib/gyp/generator/ninja.py", line
2133, in GenerateOutput
    pool.map(CallGenerateOutputForConfig, arglists)
  File "/usr/lib/python2.7/multiprocessing/pool.py", line 227, in map
    return self.map_async(func, iterable, chunksize).get()
  File "/usr/lib/python2.7/multiprocessing/pool.py", line 528, in get
    raise self._value
KeyError: '../../../base/base.gyp:sanitizer_options'


IIUC this is because the target dictionary isn't populated at the time target
conditions are checked.

[Alexander Potapenko, 2013-11-01 11:36:59]

Re-uploaded the working version from Patch Set 8 with every target (including
non-executables) depending on the ASan options.

[Alexander Potapenko, 2013-11-01 15:30:43]

Any other comments/concerns?

[Nico, 2013-11-01 23:56:22]

Maybe we can fix that gyp bug one day. Maybe you can file an issue on this (with
the stack) and assign it to me?

Until then, this lgtm.

https://codereview.chromium.org/25687005/diff/815001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/25687005/diff/815001/build/common.gypi#newcod...
build/common.gypi:3297: 'dependencies': [
Can you add a comment like "# This is only needed for executable targets, but
checking _type in a target_condition here makes gyp throw an exception"?

[Alexander Potapenko, 2013-11-11 09:41:40]

Filed https://code.google.com/p/gyp/issues/detail?id=380

https://codereview.chromium.org/25687005/diff/815001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/25687005/diff/815001/build/common.gypi#newcod...
build/common.gypi:3297: 'dependencies': [
On 2013/11/01 23:56:22, Nico (ooo until Nov 12) wrote:
> Can you add a comment like "# This is only needed for executable targets, but
> checking _type in a target_condition here makes gyp throw an exception"?

Done.

[Alexander Potapenko, 2013-11-11 09:42:05]

Committed patchset #10 manually as r234207 (presubmit successful).

[Alexander Potapenko, 2013-11-15 15:17:34]

On 2013/11/11 09:42:05, Alexander Potapenko wrote:
> Committed patchset #10 manually as r234207 (presubmit successful).

This commit resulted in http://crbug.com/317670 and I had to revert it.
In fact I'm completely puzzled with how the problem manifests and don't know how
to proceed.
Perhaps a good thing to do is to fix
https://code.google.com/p/gyp/issues/detail?id=380 -- I'll look into that.