This is an archive of the discontinued LLVM Phabricator instance.

[asancov] Write coverage directly to a memory-mapped file
ClosedPublic

Authored by eugenis on May 22 2014, 6:13 AM.

Details

Reviewers
kcc
dvyukov
Summary

This way does not require a __sanitizer_cov_dump() call. That's important on Android, where apps can be killed at arbitrary time.

We write raw PCs to disk instead of module offsets; we also write memory layout to a separate file. This increases dump size by the factor of 2 on 64-bit systems.

Diff Detail

Event Timeline

eugenis updated this revision to Diff 9693.May 22 2014, 6:13 AM
eugenis retitled this revision from to [asancov] Write coverage directly to a memory-mapped file.
eugenis updated this object.
eugenis edited the test plan for this revision. (Show Details)
eugenis added reviewers: kcc, dvyukov.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: Unknown Object (MLST).
kcc added inline comments.May 22 2014, 7:01 AM
lib/sanitizer_common/sanitizer_coverage_direct.cc
1 ↗(On Diff #9693)

This file deserves an extended comment.

eugenis updated this revision to Diff 9695.May 22 2014, 7:13 AM
eugenis added inline comments.
lib/sanitizer_common/sanitizer_coverage_direct.cc
1 ↗(On Diff #9693)

done

kcc added inline comments.May 23 2014, 1:27 AM
lib/sanitizer_common/sanitizer_coverage_direct.cc
17 ↗(On Diff #9695)

You mean, 32 or 64?

73 ↗(On Diff #9695)

Just in case, I would replace this with

for (int i = 0; i < (1<<30); i++) {
      ....
}
CHECK(0 && "CoverageMap::TryAdd wanted to loop forever")
87 ↗(On Diff #9695)

I wonder if this thing is AS-safe.
This at least deserves a comment.

test/asan/TestCases/Linux/coverage-direct-large.cc
20

don't you need to cleanup after the test, as other coverage tests do?

eugenis updated this revision to Diff 9760.May 23 2014, 7:52 AM
eugenis updated this revision to Diff 9761.May 23 2014, 7:55 AM
eugenis added inline comments.
lib/sanitizer_common/sanitizer_coverage_direct.cc
17 ↗(On Diff #9695)

sure, I expanded the comment
we need this to parse the .sancov.raw file from a 32-bit process on a 64-bit machine.

73 ↗(On Diff #9695)

This is gone in the new patch.

87 ↗(On Diff #9695)

It was not. Good point. The new implementation is.

test/asan/TestCases/Linux/coverage-direct-large.cc
20

I think it's better to cleanup before the test, and leave test artifacts (under the Output dir) for inspection.

This way an interrupted test won't mess up the next run.

See D3898 for instrumentation-side changes.
PTAL.

kcc added inline comments.May 26 2014, 1:30 AM
lib/sanitizer_common/sanitizer_coverage.cc
120

this function deserves a comment.

eugenis updated this revision to Diff 9803.May 26 2014, 5:53 AM
eugenis added inline comments.
lib/sanitizer_common/sanitizer_coverage.cc
120

done

kcc accepted this revision.May 26 2014, 6:05 AM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 26 2014, 6:05 AM
eugenis updated this revision to Diff 9826.May 27 2014, 2:15 AM
eugenis edited edge metadata.

I've had to move a bunch of functions to *_libcdep.cc because now coverage depends on GetListOfModules, which is implemented via dl_iterate_get_phdr.
Also, dlopen/dlclose interceptors are special in that they are not affected by and ignores and blacklists, so I added another macro in sanitizer_common_interceptors to handle them.

dvyukov added inline comments.May 27 2014, 2:43 AM
lib/sanitizer_common/sanitizer_coverage_libcdep.cc
94 ↗(On Diff #9826)

same comment as for sancov.map

129 ↗(On Diff #9826)

You need to increment it after MapWritableFileToMemory. Otherwise the CHECK in Add does not work.

130 ↗(On Diff #9826)

Why do you need this to be atomic? It's accessed only in Init or under the mutex.

138 ↗(On Diff #9826)

please print a readable error message with errno and Die
this is not a programming error, it can happen due to out of disk space of networking error

153 ↗(On Diff #9826)

memory_order_acquire in CHECK looks suspicious
I think it needs to be just memory_order_relaxed

lib/sanitizer_common/sanitizer_coverage_mapping_libcdep.cc
43 ↗(On Diff #9826)

It's just a "zd.sancov.map" string, right?
64 would be enough. 1024 makes me think that it's a full path, while it is not.

46 ↗(On Diff #9826)

If the process is killed in the middle of this function, then we will get broken cov profile. And working in the presence of sudden kills was the main motivation for this changes.
I would write the data to ancov.map.tmp and then rename it to ancov.map.

lib/sanitizer_common/sanitizer_flags.cc
133

please explain when it can be useful (if the process is suddenly killed, it will leave some valid coverage file)

lib/tsan/rtl/tsan_interceptors.cc
211

debug leftover?

eugenis updated this revision to Diff 9830.May 27 2014, 4:50 AM
eugenis added inline comments.
lib/sanitizer_common/sanitizer_coverage_libcdep.cc
94 ↗(On Diff #9826)

done

129 ↗(On Diff #9826)

done

130 ↗(On Diff #9826)

done

138 ↗(On Diff #9826)

done

153 ↗(On Diff #9826)

But then release-store to pc_array_size has no corresponding acquire-load.
Looks fine to me: this ordering is only needed for the CHECK to work, fetch_add one line above can be relaxed.

lib/sanitizer_common/sanitizer_coverage_mapping_libcdep.cc
43 ↗(On Diff #9826)

done. but I'll increase it back to 1024 with the next commit anyway.

46 ↗(On Diff #9826)

done

lib/sanitizer_common/sanitizer_flags.cc
133

done

dvyukov accepted this revision.May 27 2014, 5:31 AM
dvyukov edited edge metadata.

LGTM

Eugene.Zelenko closed this revision.Oct 3 2016, 1:34 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL209653.