Page MenuHomePhabricator

[sancov] generating html report on coverage dump
ClosedPublic

Authored by aizatsky on Jan 20 2016, 1:52 PM.

Details

Summary

If html_cov_report is set and sancov binary is found, the html report would be generated automatically on coverage dump.

Diff Detail

Repository
rL LLVM

Event Timeline

aizatsky updated this revision to Diff 45442.Jan 20 2016, 1:52 PM
aizatsky retitled this revision from to generating html report on coverage dump.
aizatsky updated this object.
aizatsky updated this revision to Diff 45585.Jan 21 2016, 12:54 PM

cleanup round.

aizatsky retitled this revision from generating html report on coverage dump to [sancov] generating html report on coverage dump.Jan 21 2016, 3:12 PM
aizatsky updated this object.
aizatsky added reviewers: kcc, samsonov.
aizatsky added a project: Restricted Project.
aizatsky added a subscriber: llvm-commits.
kcc added inline comments.Jan 21 2016, 3:40 PM
lib/sanitizer_common/sanitizer_common.cc
426 ↗(On Diff #45602)

remove {}

lib/sanitizer_common/sanitizer_common_nolibc.cc
27 ↗(On Diff #45602)

this is gross :)
Put the stubs into windows and mac files instead

lib/sanitizer_common/sanitizer_coverage_libcdep.cc
793 ↗(On Diff #45602)

you need InternalScopedString

830 ↗(On Diff #45602)

put this into a separate function

kcc edited edge metadata.Jan 21 2016, 3:45 PM

also, StartSubprocess deserves a separate test

aizatsky updated this revision to Diff 46035.Jan 26 2016, 12:42 PM
aizatsky updated this object.
aizatsky edited edge metadata.

I've merged in subprocess changes. Please take another look.

aizatsky updated this revision to Diff 46038.Jan 26 2016, 12:53 PM
aizatsky marked an inline comment as done.

extracted function.

samsonov added inline comments.Jan 26 2016, 1:34 PM
lib/sanitizer_common/sanitizer_coverage_libcdep.cc
787 ↗(On Diff #46038)

Consider adding an early return here instead

if (!common_flags()->html_cov_report || sancov_args[0] == nullptr)
800 ↗(On Diff #46038)

Don't you want to log unsuccessful attempt to generate html?

lib/sanitizer_common/sanitizer_flags.inc
206 ↗(On Diff #46038)

Is sancov_path more appropriate flag name?

test/asan/lit.cfg
144 ↗(On Diff #46038)

I think you should use llvm_tools_dir instead. See the way it's handled in test/lit.common.cfg: probably you want to add this functionality there. Also, as LLVM tools directory is in PATH, auto-discovery of "sancov" binary must work in the lit test (in the same way we auto-discover llvm-symbolizer).

147 ↗(On Diff #46038)

You have to explicitly add dependency on "sancov" binary from sanitizer test suites: see test/CMakeLists.txt

aizatsky updated this revision to Diff 46176.Jan 27 2016, 1:43 PM
aizatsky marked 4 inline comments as done.

review

all done.

lib/sanitizer_common/sanitizer_coverage_libcdep.cc
800 ↗(On Diff #46038)

StartSubprocess & WaitForPid have logging inside. I was relying on those.

aizatsky updated this revision to Diff 46178.Jan 27 2016, 1:45 PM

clang-format

samsonov edited edge metadata.Jan 27 2016, 2:00 PM

Looks reasonsable to me. I'll let Kostya sign this off.

test/asan/TestCases/Posix/coverage_html_report.cc
6 ↗(On Diff #46178)

Will this test work even if you don't provide sancov=%sancovcc explicitly?

24 ↗(On Diff #46178)

Should this also be [[PID]].html ?

aizatsky added inline comments.Jan 27 2016, 2:09 PM
test/asan/TestCases/Posix/coverage_html_report.cc
6 ↗(On Diff #46178)

it does

24 ↗(On Diff #46178)

It's a different invocation of FileCheck. Unfortunately PID is not available here.

aizatsky updated this revision to Diff 46179.Jan 27 2016, 2:14 PM
aizatsky edited edge metadata.

removed explicit tool path

samsonov accepted this revision.Jan 27 2016, 3:36 PM
samsonov edited edge metadata.

LGTM

test/asan/TestCases/Posix/coverage_html_report.cc
24 ↗(On Diff #46179)

Ack.

This revision is now accepted and ready to land.Jan 27 2016, 3:36 PM

Thank you for review.

This revision was automatically updated to reflect the committed changes.