[coverage] Enable sanitizer coverage.

[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
NOTRY=true
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test

Committed: https://crrev.com/33ffced5ccd4b33f0c59e580e624cb28653a8c01
Cr-Commit-Position: refs/heads/master@{#34578}

[Michael Achenbach, 2016-03-03 15:48:22]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

BUG=
==========

to

==========
[coverage] Enable sanitizer coverage.

BUG=chromium:568949
LOG=n
==========

[Michael Achenbach, 2016-03-03 16:32:49]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

BUG=chromium:568949
LOG=n
==========

to

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

[Michael Achenbach, 2016-03-03 16:32:50]

machenbach@chromium.org changed reviewers:
+ kjellander@chromium.org, tandrii@chromium.org

[Michael Achenbach, 2016-03-03 16:38:38]

PTA preliminary Look

Not sure where to begin reviewing this. Maybe a high-level look at the tests and
the general design. I might refactor a bit more, but I'd be happy for
suggestions.

The sancov file that'll be used and that inspired parts of this CL is here for
reference:
https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/s...

I tested this locally and also inspected the intermediate results for
correctness manually. Otherwise, I don't see a good way how to system-test this
automatically, other than creating a fake, test executable. But that'd be a
heavy process...

[Michael Achenbach, 2016-03-03 16:40:11]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

to

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

[Michael Achenbach, 2016-03-03 16:49:43]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

to

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

[Michael Achenbach, 2016-03-04 09:03:31]

Hold on, will give it another docu/refactoring round now.

[Michael Achenbach, 2016-03-04 10:02:46]

PTAL now. Documented all methods in patch 11.

[kjellander_chromium, 2016-03-04 14:35:22]

Sorry I didn't have time to finish the review now. I might be able to take a
look later tonight, otherwise Monday.

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper...
build/coverage_wrapper.py:21: def remove_failsafe(l, item):
rename variable "l" to something more describing, like "string_list"

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:39: from multiprocessing import Pool,
cpu_count
nit: move down the from-import: "Imports should be grouped with the order being
most generic to least generic"
https://google.github.io/styleguide/pyguide.html?showone=Imports_formatting#I...

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:65: OUTPUT_PATH_PREFIX =
os.path.join(BUILD_DIR, '..', '..', '')
it seems more readable to append os.sep than joining with an empty string, but I
guess this is fine.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:204: # Merge muliprocessing results and
prepare output data.
multiprocessing.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter_test.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:6: import coverage
Can coverage be assumed to be present? Since it's not a standard module...

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:122: 
nit: +1 blank line before top-level class.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:127: cls._cov = coverage.coverage(
I guess coverage was used mostly for development to ensure you cover your code?
Does it make sense to leave it in here?

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:137: print cls._cov.report()
I don't think you should print things to stdout by default in a unit test? Maybe
use logging instead in order to keep the code around for future debugging?

[Michael Achenbach, 2016-03-04 15:00:26]

First review done.

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper...
build/coverage_wrapper.py:21: def remove_failsafe(l, item):
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> rename variable "l" to something more describing, like "string_list" 

Done.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:39: from multiprocessing import Pool,
cpu_count
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> nit: move down the from-import: "Imports should be grouped with the order
being
> most generic to least generic"
>
https://google.github.io/styleguide/pyguide.html?showone=Imports_formatting#I...

Done.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:204: # Merge muliprocessing results and
prepare output data.
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> multiprocessing.

Done.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter_test.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:6: import coverage
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> Can coverage be assumed to be present? Since it's not a standard module...

Added a comment. I assume it's installed by whoever runs this test. (we don't
run this on bots nor on presubmit).

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:122: 
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> nit: +1 blank line before top-level class.

Done.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:127: cls._cov = coverage.coverage(
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> I guess coverage was used mostly for development to ensure you cover your
code?
> Does it make sense to leave it in here?

Don't know. Development isn't over yet :) This is only a prototype after all. If
it turns out to be feasible and useful I might work more on this to get more
speed and add new features.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:137: print cls._cov.report()
Just using the raw reporting now. It's tricky to get this working with logging
as report() already prints (putting it into a print like I did is useless). It
can chose a stream, but it's not easy to set this up with logging.

[tandrii(chromium), 2016-03-07 15:10:22]

LGTM

I think the pipeline is the sanest it could have been.

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper...
build/coverage_wrapper.py:21: def remove_failsafe(l, item):
nit: maybe remove_if_exists?

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:6: """
nit: afair, first line of doc shouldn't be empty, though the idea is primarily
for nice doc rendering.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:104: Returns: A mapping of file names to
lists of lines. The file names have
here and next line: s/lines/line numbers

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:114: yield line[OUTPUT_PATH_OFFSET:]
nit: i'd use s/OUTPUT_PATH_OFFSET/len(OUTPUT_PATH_PREFIX)
as it's easier to read.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:134: return {k:sorted(list(file_map[k]))
for k in file_map if keep(k)}
nit: space after :
also, omit "list", sorted works on any iterables.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:192: 'files': {k:map(lambda x: [x, 0],
sorted(result[k])) for k in result},
suggestion: s/k/f   and   s/x/l    (at least that's a bit easier to read since f
= filename, l = line number..

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:259: assert len(exe_list) <= 32, 'Max 32
different tests are supported.'
why 32?

ftr, python works just fine even with 67 bit integers:
>>> bin((1<<66) | 0x15)
'0b1000000000000000000000000000000000000000000000000000000000000010101'

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:272: i = 0
small suggestion: I personally avoid using index and do this:

while old_lines[0][0] < l:
  old_lines = old_lines[1:]

but maybe that's too much C++ with ranges.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:346: parser.add_argument('action',
add arg 
choices = ('all', 'merge')

https://docs.python.org/3/library/argparse.html#choices

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter_test.py (right):

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:124: 'src/baz/bar.h': [[0, 0], [3, 3],
[7, 3], [8, 0]],
I'd use 0b11 instead of bitmask 3, but it's tests, so just FYI

>> 0b11
3

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_merger.py (right):

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_merger.py:92: for executable, files in
file_map.iteritems():
before today's discussion in person, I thought this splitting has dubious value
because I thought there could be many executables, some with a lot and some with
a few files such as {"exe1": 100, "exe2": 2,...}. In such a case, I don't see
much value in splitting exe2.

However, given that you have just a few executables with likely tons of files to
merge for each, this code is OK. Yet, consider adding this bit to the doc.

[Michael Achenbach, 2016-03-07 16:07:21]

Done

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/200001/build/coverage_wrapper...
build/coverage_wrapper.py:21: def remove_failsafe(l, item):
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> nit: maybe remove_if_exists?

Done.

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:6: """
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> nit: afair, first line of doc shouldn't be empty, though the idea is primarily
> for nice doc rendering.

Done.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:104: Returns: A mapping of file names to
lists of lines. The file names have
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> here and next line: s/lines/line numbers

Done.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:114: yield line[OUTPUT_PATH_OFFSET:]
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> nit: i'd use s/OUTPUT_PATH_OFFSET/len(OUTPUT_PATH_PREFIX)
> as it's easier to read.

Done.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:134: return {k:sorted(list(file_map[k]))
for k in file_map if keep(k)}
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> nit: space after :
> also, omit "list", sorted works on any iterables.

Done.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:192: 'files': {k:map(lambda x: [x, 0],
sorted(result[k])) for k in result},
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> suggestion: s/k/f   and   s/x/l    (at least that's a bit easier to read since
f
> = filename, l = line number..

Done.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:259: assert len(exe_list) <= 32, 'Max 32
different tests are supported.'
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> why 32?
> 
> ftr, python works just fine even with 67 bit integers:
> >>> bin((1<<66) | 0x15)
> '0b1000000000000000000000000000000000000000000000000000000000000010101'

ok - but not sure if there's a limit on the JS side for parsing a number of that
size from JSON. So, I'd keep it like that for now.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:272: i = 0
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> small suggestion: I personally avoid using index and do this:
> 
> while old_lines[0][0] < l:
>   old_lines = old_lines[1:]
> 
> but maybe that's too much C++ with ranges.

ok - somewhat more readable, but also more expensive. I'd keep the old version
for speed here, since this is a hot function.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:346: parser.add_argument('action',
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> add arg 
> choices = ('all', 'merge')
> 
> https://docs.python.org/3/library/argparse.html#choices

Done.

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter_test.py (right):

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter_test.py:124: 'src/baz/bar.h': [[0, 0], [3, 3],
[7, 3], [8, 0]],
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> I'd use 0b11 instead of bitmask 3, but it's tests, so just FYI
> 
> >> 0b11
> 3

Good idea. I initially just copied the stdout to feed the test data :)

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_merger.py (right):

https://codereview.chromium.org/1737263003/diff/220001/tools/sanitizers/sanco...
tools/sanitizers/sancov_merger.py:92: for executable, files in
file_map.iteritems():
On 2016/03/07 15:10:22, tandrii(chromium) wrote:
> before today's discussion in person, I thought this splitting has dubious
value
> because I thought there could be many executables, some with a lot and some
with
> a few files such as {"exe1": 100, "exe2": 2,...}. In such a case, I don't see
> much value in splitting exe2.
> 
> However, given that you have just a few executables with likely tons of files
to
> merge for each, this code is OK. Yet, consider adding this bit to the doc.

Right, this code is not optimal for that case. I've added a comment for now and
if
I have time, I'll implement the more general case and see if it's measurable.

[Michael Achenbach, 2016-03-07 16:07:30]

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

[commit-bot: I haz the power, 2016-03-07 16:07:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737263003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737263003/240001

[commit-bot: I haz the power, 2016-03-07 16:26:18]

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

[commit-bot: I haz the power, 2016-03-07 16:26:18]

Dry run: This issue passed the CQ dry run.

[kjellander_chromium, 2016-03-08 04:48:33]

lgtm, although I'd like to see my comments addressed/answered.

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper...
build/coverage_wrapper.py:22: try:
I guess it's faster to do
if item in string_list:
  string_list.remove(item)
instead of allowing an exception to be thrown? Assuming exceptions are as
expensive as in C++ and Java. Not a big deal here though, but if you want :)

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:257: assert len(exe_list) <= 32, 'Max 32
different tests are supported.'
Maybe this limitation should be documented more visibly? And why even have it?
Many tests = slow seems like a fair consequence.

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_merger.py (right):

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
tools/sanitizers/sancov_merger.py:67: output, _ = process.communicate()
what about process.returncode?

[Michael Achenbach, 2016-03-08 10:05:46]

All done. Thanks for the reviews!

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/200001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:65: OUTPUT_PATH_PREFIX =
os.path.join(BUILD_DIR, '..', '..', '')
On 2016/03/04 14:35:22, kjellander (chromium) wrote:
> it seems more readable to append os.sep than joining with an empty string, but
I
> guess this is fine.

I leave it as it is now - I think joining os.sep is also weird, as os.sep is the
join operator in os.path.join. I'd need an extra string concatenation.

Since this whole tool is linux-only, I think the most readable would simply be:
'%s/../../' % BUILD_DIR

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper...
build/coverage_wrapper.py:22: try:
On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> I guess it's faster to do
> if item in string_list:
>   string_list.remove(item)
> instead of allowing an exception to be thrown? Assuming exceptions are as
> expensive as in C++ and Java. Not a big deal here though, but if you want :)

Done. Your way is definitely more readable and I don't care for speed here. This
is bounded by the number of objects, which are max 1000 for v8. And each list
contains maybe ~30 items.

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_formatter.py (right):

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
tools/sanitizers/sancov_formatter.py:257: assert len(exe_list) <= 32, 'Max 32
different tests are supported.'
On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> Maybe this limitation should be documented more visibly? And why even have it?
> Many tests = slow seems like a fair consequence.

I'll add a comment here and in the top-level doc. I updated the number to 52
after reading the JS specs and trying around. The number of tests is restricted
to 52, to allow javascript JSON parsing of the bitsets encoded as numbers. JS
max safe int is (1 << 53) - 1, because JS represents bigger numbers as doubles.
Beyond (1 << 53) - 1, precision is lost.

In order to support more tests (or better test contexts) in a future version, we
need a different encoding, e.g. hex strings or something.

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
File tools/sanitizers/sancov_merger.py (right):

https://codereview.chromium.org/1737263003/diff/240001/tools/sanitizers/sanco...
tools/sanitizers/sancov_merger.py:67: output, _ = process.communicate()
On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> what about process.returncode?

Good point! Added asserts to not let a bad return code go by undetected.

[Michael Achenbach, 2016-03-08 10:39:27]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2016-03-08 10:39:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
tandrii@chromium.org, kjellander@chromium.org
Link to the patchset: https://codereview.chromium.org/1737263003/#ps280001
(title: "Logging + exe blacklist")

[commit-bot: I haz the power, 2016-03-08 10:39:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737263003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737263003/280001

[Michael Achenbach, 2016-03-08 10:40:40]

Added a bit sanity logging in patch 15 to ease debugging later. Also blacklisted
the executables that happen to hang around in the build output, but for which we
don't run tests.

[Michael Achenbach, 2016-03-08 10:42:27]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

to

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
NOTRY=true
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

[Michael Achenbach, 2016-03-08 10:42:35]

The CQ bit was unchecked by machenbach@chromium.org

[Michael Achenbach, 2016-03-08 10:42:40]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2016-03-08 10:42:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737263003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737263003/280001

[commit-bot: I haz the power, 2016-03-08 10:48:06]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
NOTRY=true
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

to

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
NOTRY=true
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

[commit-bot: I haz the power, 2016-03-08 10:48:07]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-03-08 10:48:47]

Description was changed from

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
NOTRY=true
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test
==========

to

==========
[coverage] Enable sanitizer coverage.

This adds sanitizer-coverage compilation, test-runner
features and post processing.

Sanitizer coverage is expected to be used together with
asan.

During test runner execution, the produced sancov files
are disambiguated and match the pattern:
<executable name>.test.<test id>.sancov.

Two additional scripts are added for merging raw sancov
files and for generating json data containing all
instrumented lines + all covered lines from merged sancov
files. Both scripts use multiprocessing for speed.

The json data will later be uploaded to google storage
for further use, e.g. to show coverage data in rietveld.

Sancov documentation:
http://clang.llvm.org/docs/SanitizerCoverage.html

BUG=chromium:568949
LOG=n
NOTRY=true
TEST=python -m unittest sancov_formatter_test
TEST=python -m unittest sancov_merger_test

Committed: https://crrev.com/33ffced5ccd4b33f0c59e580e624cb28653a8c01
Cr-Commit-Position: refs/heads/master@{#34578}
==========

[commit-bot: I haz the power, 2016-03-08 10:48:48]

Patchset 15 (id:??) landed as
https://crrev.com/33ffced5ccd4b33f0c59e580e624cb28653a8c01
Cr-Commit-Position: refs/heads/master@{#34578}

[tandrii(chromium), 2016-03-08 14:48:57]

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper...
build/coverage_wrapper.py:22: try:
On 2016/03/08 10:05:46, Michael Achenbach wrote:
> On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> > I guess it's faster to do
> > if item in string_list:
> >   string_list.remove(item)
> > instead of allowing an exception to be thrown? Assuming exceptions are as
> > expensive as in C++ and Java. Not a big deal here though, but if you want :)
> 
> Done. Your way is definitely more readable and I don't care for speed here.
This
> is bounded by the number of objects, which are max 1000 for v8. And each list
> contains maybe ~30 items.

Your honor, I object! First, 
try .. except ValueError: .. 
is the Pythonic way to do stuff.
Second, here each line takes O(n):
1) if x in list
2)   list.remove(x)

So, there is no way try..catch is slower than code above.

[Michael Achenbach, 2016-03-08 14:54:00]

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper.py
File build/coverage_wrapper.py (right):

https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper...
build/coverage_wrapper.py:22: try:
On 2016/03/08 14:48:57, tandrii(chromium) wrote:
> On 2016/03/08 10:05:46, Michael Achenbach wrote:
> > On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> > > I guess it's faster to do
> > > if item in string_list:
> > >   string_list.remove(item)
> > > instead of allowing an exception to be thrown? Assuming exceptions are as
> > > expensive as in C++ and Java. Not a big deal here though, but if you want
:)
> > 
> > Done. Your way is definitely more readable and I don't care for speed here.
> This
> > is bounded by the number of objects, which are max 1000 for v8. And each
list
> > contains maybe ~30 items.
> 
> Your honor, I object! First, 
> try .. except ValueError: .. 
> is the Pythonic way to do stuff.
> Second, here each line takes O(n):
> 1) if x in list
> 2)   list.remove(x)
> 
> So, there is no way try..catch is slower than code above.

Right. Either way, I think it's hardly measurable given the small number of
objects. Maybe I change to more pythonic again in next CL.

[kjellander_chromium, 2016-03-08 15:08:48]

On 2016/03/08 14:54:00, Michael Achenbach wrote:
>
https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper.py
> File build/coverage_wrapper.py (right):
> 
>
https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper...
> build/coverage_wrapper.py:22: try:
> On 2016/03/08 14:48:57, tandrii(chromium) wrote:
> > On 2016/03/08 10:05:46, Michael Achenbach wrote:
> > > On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> > > > I guess it's faster to do
> > > > if item in string_list:
> > > >   string_list.remove(item)
> > > > instead of allowing an exception to be thrown? Assuming exceptions are
as
> > > > expensive as in C++ and Java. Not a big deal here though, but if you
want
> :)
> > > 
> > > Done. Your way is definitely more readable and I don't care for speed
here.
> > This
> > > is bounded by the number of objects, which are max 1000 for v8. And each
> list
> > > contains maybe ~30 items.
> > 
> > Your honor, I object! First, 
> > try .. except ValueError: .. 
> > is the Pythonic way to do stuff.
> > Second, here each line takes O(n):
> > 1) if x in list
> > 2)   list.remove(x)
> > 
> > So, there is no way try..catch is slower than code above.
> 
> Right. Either way, I think it's hardly measurable given the small number of
> objects. Maybe I change to more pythonic again in next CL.

I didn't know that, but then I also don't have Python readability :)
Thanks for educating me.

[tandrii(chromium), 2016-03-08 15:14:44]

On 2016/03/08 15:08:48, kjellander (chromium) wrote:
> On 2016/03/08 14:54:00, Michael Achenbach wrote:
> >
>
https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper.py
> > File build/coverage_wrapper.py (right):
> > 
> >
>
https://codereview.chromium.org/1737263003/diff/240001/build/coverage_wrapper...
> > build/coverage_wrapper.py:22: try:
> > On 2016/03/08 14:48:57, tandrii(chromium) wrote:
> > > On 2016/03/08 10:05:46, Michael Achenbach wrote:
> > > > On 2016/03/08 04:48:33, kjellander (chromium) wrote:
> > > > > I guess it's faster to do
> > > > > if item in string_list:
> > > > >   string_list.remove(item)
> > > > > instead of allowing an exception to be thrown? Assuming exceptions are
> as
> > > > > expensive as in C++ and Java. Not a big deal here though, but if you
> want
> > :)
> > > > 
> > > > Done. Your way is definitely more readable and I don't care for speed
> here.
> > > This
> > > > is bounded by the number of objects, which are max 1000 for v8. And each
> > list
> > > > contains maybe ~30 items.
> > > 
> > > Your honor, I object! First, 
> > > try .. except ValueError: .. 
> > > is the Pythonic way to do stuff.
> > > Second, here each line takes O(n):
> > > 1) if x in list
> > > 2)   list.remove(x)
> > > 
> > > So, there is no way try..catch is slower than code above.
> > 
> > Right. Either way, I think it's hardly measurable given the small number of
> > objects. Maybe I change to more pythonic again in next CL.
> 
> I didn't know that, but then I also don't have Python readability :)
> Thanks for educating me.

Michael: nah, don't bother :)
Henrik: No readability either. But Python is so slow by itself that exceptions
don't really add much slowness (though as someone optimizing pure Python code
for quite a bit, I agree they are slower than if .. else statement).
Java/C++ are of course much much faster, and so I'd argue that on non hot-path
the cost of exceptions is neglible, and readability is the only concern.
Of course, at Google C++ exceptions are banned, though the main surviving reason
is "existing code isn't exception safe".