Page MenuHomePhabricator

[esan] Add __esan_report for mid-run data
ClosedPublic

Authored by bruening on Jul 7 2016, 9:58 AM.

Details

Summary

Adds a new public interface routine __esan_report() which can be used to
request profiling results prior to abnormal termination (e.g., for a server
process killed by its parent where the normal exit does not allow for
normal result reporting).

Implements this for the working-set tool. The cache frag tool is left
unimplemented as it requires missing iteration capabilities.

Adds a new test.

Diff Detail

Repository
rL LLVM

Event Timeline

bruening updated this revision to Diff 63087.Jul 7 2016, 9:58 AM
bruening retitled this revision from to [esan] Add __esan_report for mid-run data.
bruening updated this object.
bruening added a reviewer: aizatsky.
bruening added subscribers: llvm-commits, eugenis, kcc and 2 others.
aizatsky requested changes to this revision.Jul 7 2016, 12:57 PM
aizatsky edited edge metadata.
aizatsky added inline comments.
include/sanitizer/esan_interface.h
26 ↗(On Diff #63087)

do you want this to be weak so that host program could be compiled without esan as well?
Or do you plan to have ifdefs in the host?

lib/esan/esan.cpp
236 ↗(On Diff #63087)

Do you think it makes sense to have classes with virtual methods now?

All these functions perform tool-based dispatch.

lib/esan/working_set.cpp
121 ↗(On Diff #63087)

can you explain why is this a desired behavior? It is not clear to me :)

Maybe making it caller's responsibility is better?

countAndClearShadowValues(u32 BitIdx, uptr ShadowStart,

uptr ShadowEnd, bool ClearBits)
This revision now requires changes to proceed.Jul 7 2016, 12:57 PM
bruening added inline comments.Jul 8 2016, 9:09 AM
include/sanitizer/esan_interface.h
26 ↗(On Diff #63087)

I don't see how it can work without ifdefs simply by being weak? If it's weak and the esan library is not linked in, the caller's call to esan_report() will be a call to NULL and will crash. To avoid that, the caller would have to provide an empty-body implementation of esan_report that was also weak, and we'd have to hope that the esan library's version was found first. I don't see any use of weak in the other *san_interface.h headers and I assume they all rely on ifdefs in the caller -- right? Or am I missing something?

lib/esan/esan.cpp
236 ↗(On Diff #63087)

That is something we have considered and should probably do at some point, but as a separate refactoring from this CL. We'll put it on our short list for improving the code.

lib/esan/working_set.cpp
121 ↗(On Diff #63087)

We only want to clear bits that are measuring just part of the execution: one time period. The top bit is measuring the entire execution, so it's special, and should never be cleared. In the past we only ever called this routine for the top bit at the very end of the run to report the total, but with this new mid-run report we're calling this at arbitrary times and thus need to avoid the clear.

aizatsky accepted this revision.Jul 8 2016, 11:22 AM
aizatsky edited edge metadata.
aizatsky added inline comments.
include/sanitizer/esan_interface.h
26 ↗(On Diff #63087)

No, with weak function the caller could be written as:

if (esan_report)

esan_report();

And compile/link/run fine with and without esan and without the use of preprocessor.

lib/esan/working_set.cpp
121 ↗(On Diff #63087)

If you could put this into one-line comment next to clear that would be awesome.

// high bit measure access during entire execution, should never be cleared.

This revision is now accepted and ready to land.Jul 8 2016, 11:22 AM
bruening added inline comments.Jul 8 2016, 11:53 AM
include/sanitizer/esan_interface.h
26 ↗(On Diff #63087)

Right, that would work. I don't see any precedent for this, as no other sanitizer does it in their public interfaces, but I don't see any downside to making it weak (unintended symbol collision is quite unlikely) so I'll go ahead and do so.

lib/esan/working_set.cpp
121 ↗(On Diff #63087)

Will do.

bruening marked an inline comment as done.Jul 8 2016, 7:25 PM
This revision was automatically updated to reflect the committed changes.