Support MSan/TSan in test isolation and test_env.py

Support MSan/TSan in test isolation and test_env.py

BUG=454828
R=maruel@chromium.org,glider@chromium.org
TBR=jam@chromium.org

Committed: https://crrev.com/eeb0653038f092b18a9ca9b865a2fe80fe625a41
Cr-Commit-Position: refs/heads/master@{#314587}

[earthdok, 2015-02-03 17:56:52]

please take a look (M-A on the swarming side, Alex on the sanitizer side)

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py
File testing/test_env.py (right):

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode87
testing/test_env.py:87: extra_env['ASAN_OPTIONS'] = ' '.join(asan_options)
Alex: do we need replace_intrin/strict_memcmp here?

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode96
testing/test_env.py:96: if lsan:
Currently we run LSan on top of ASan. The logic here, which I copied from
runtest.py, also supports:
- running LSan stand-alone (which is possible but not done in Chromium) and
- running LSan on top of MSan (which is in our plans but not yet implemented).

I think it's cool to have the option of using those modes without having to
patch the scripts again. However, one could argue that simplicity is more
important. I don't have a strong opinion.

The difference is about 5 LOC.

[M-A Ruel, 2015-02-03 18:54:13]

isolate part lgtm with nits.

It's kind of sad to have to copy paste the flags to all comands but I don't see
a quick way to fix that yet so we'll have to live with that for now.

https://codereview.chromium.org/895923002/diff/20001/base/base.isolate
File base/base.isolate (right):

https://codereview.chromium.org/895923002/diff/20001/base/base.isolate#newcode40
base/base.isolate:40: ['OS=="linux" and (asan==1 or lsan==1 or msan==1 or
tsan==1)', {
:)

https://codereview.chromium.org/895923002/diff/20001/build/android/pylib/util...
File build/android/pylib/utils/isolator.py (right):

https://codereview.chromium.org/895923002/diff/20001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:31: 'lsan': '0',
still prefer to keep the list sorted.

https://codereview.chromium.org/895923002/diff/20001/build/isolate.gypi
File build/isolate.gypi (right):

https://codereview.chromium.org/895923002/diff/20001/build/isolate.gypi#newco...
build/isolate.gypi:80: '--config-variable', 'lsan=<(lsan)',
keep sorted here too.

[earthdok, 2015-02-03 19:07:45]

https://codereview.chromium.org/895923002/diff/20001/build/android/pylib/util...
File build/android/pylib/utils/isolator.py (right):

https://codereview.chromium.org/895923002/diff/20001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:31: 'lsan': '0',
On 2015/02/03 18:54:13, M-A Ruel wrote:
> still prefer to keep the list sorted.

Done.

https://codereview.chromium.org/895923002/diff/20001/build/isolate.gypi
File build/isolate.gypi (right):

https://codereview.chromium.org/895923002/diff/20001/build/isolate.gypi#newco...
build/isolate.gypi:80: '--config-variable', 'lsan=<(lsan)',
On 2015/02/03 18:54:13, M-A Ruel wrote:
> keep sorted here too.

sorted the entire list

[Alexander Potapenko, 2015-02-04 10:51:46]

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py
File testing/test_env.py (right):

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode38
testing/test_env.py:38: internal_flags = frozenset(['--asan=0', '--asan=1',
'--lsan=0', '--lsan=1',
Shall we replace this with a list comprehension?

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode53
testing/test_env.py:53: def get_sanitizer_env(cmd, asan, lsan, msan, tsan):
I think having a separate var for each sanitizer sucks.

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode87
testing/test_env.py:87: extra_env['ASAN_OPTIONS'] = ' '.join(asan_options)
On 2015/02/03 17:56:52, earthdok wrote:
> Alex: do we need replace_intrin/strict_memcmp here?

I think it's fine to omit them. Linux and Mac already have them by default. No
need to complicate all the scripts with a special case for CrOS, which doesn't
affect correctness.

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode96
testing/test_env.py:96: if lsan:
I don't think we ever want to run LSan without ASan.
I'm fine with having this option, but please reflect the difference between
modes in the comments.

[earthdok, 2015-02-04 14:34:47]

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py
File testing/test_env.py (right):

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode38
testing/test_env.py:38: internal_flags = frozenset(['--asan=0', '--asan=1',
'--lsan=0', '--lsan=1',
On 2015/02/04 10:51:46, Alexander Potapenko wrote:
> Shall we replace this with a list comprehension?

Done.

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode53
testing/test_env.py:53: def get_sanitizer_env(cmd, asan, lsan, msan, tsan):
On 2015/02/04 10:51:46, Alexander Potapenko wrote:
> I think having a separate var for each sanitizer sucks.

Your suggestion?

We could use a dict, but that would only simplify the function signature at the
expense of complicating everything else.

[Alexander Potapenko, 2015-02-04 16:31:04]

lgtm

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py
File testing/test_env.py (right):

https://codereview.chromium.org/895923002/diff/1/testing/test_env.py#newcode53
testing/test_env.py:53: def get_sanitizer_env(cmd, asan, lsan, msan, tsan):
On 2015/02/04 14:34:47, earthdok wrote:
> On 2015/02/04 10:51:46, Alexander Potapenko wrote:
> > I think having a separate var for each sanitizer sucks.
> 
> Your suggestion?
> 
> We could use a dict, but that would only simplify the function signature at
the
> expense of complicating everything else.

Acknowledged.

[Alexander Potapenko, 2015-02-04 16:32:46]

https://codereview.chromium.org/895923002/diff/50001/testing/test_env.py
File testing/test_env.py (right):

https://codereview.chromium.org/895923002/diff/50001/testing/test_env.py#newc...
testing/test_env.py:102: lsan_options += symbolization_options[:]
Please fix

[earthdok, 2015-02-04 16:36:03]

New patchsets have been uploaded after l-g-t-m from glider@chromium.org

[earthdok, 2015-02-04 16:40:12]

The CQ bit was checked by earthdok@chromium.org

[commit-bot: I haz the power, 2015-02-04 16:41:20]

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

[commit-bot: I haz the power, 2015-02-04 16:48:28]

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

[commit-bot: I haz the power, 2015-02-04 16:48:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[earthdok, 2015-02-04 16:50:09]

earthdok@chromium.org changed reviewers:
+ jam@chromium.org

[earthdok, 2015-02-04 16:50:24]

On 2015/02/04 16:48:29, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

+jam@ for .isolate ownership

[earthdok, 2015-02-04 16:50:32]

The CQ bit was checked by earthdok@chromium.org

[commit-bot: I haz the power, 2015-02-04 16:51:10]

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

[commit-bot: I haz the power, 2015-02-04 18:18:34]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-02-04 18:21:57]

Patchset 5 (id:??) landed as
https://crrev.com/eeb0653038f092b18a9ca9b865a2fe80fe625a41
Cr-Commit-Position: refs/heads/master@{#314587}