This is an archive of the discontinued LLVM Phabricator instance.

Introduce stats and stats_client libraries.
ClosedPublic

Authored by pcc on Jan 13 2016, 8:13 PM.

Details

Summary

This is part of a new statistics gathering feature for the sanitizers.
See clang/docs/SanitizerStats.rst for further info and docs.

Diff Detail

Event Timeline

pcc updated this revision to Diff 44831.Jan 13 2016, 8:13 PM
pcc retitled this revision from to Introduce stats and stats_client libraries..
pcc updated this object.
pcc added reviewers: kcc, eugenis.
pcc added a subscriber: llvm-commits.
pcc added a reviewer: krasin.Jan 14 2016, 10:45 AM
krasin1 added inline comments.
lib/sanitizer_common/sanitizer_flags.cc
51

Are we restricted to use C++ strings here? The function is super scary right now.

99

I don't think that the flag value is limited by kMaxPathLength. Can you please choose a better limit? Or somehow check that the value fits the buffer?

lib/stats/stats.cc
40

s/unsigned/size_t/ maybe?

47

Probably, it makes sense to check that start != NULL to fail cleanly if WriteModuleReport is called on an unregistered module.

eugenis added inline comments.Jan 14 2016, 11:51 AM
lib/stats/stats.cc
50

This lazy file opening just complicates things. Maybe move to a separate function and call it from WriteFullReport ?

lib/stats/stats_client.cc
13

Please mention that it is intended to be linked into each individual module, and that it can not use the sanitizer_common infrastructure.

95

Why dlsym and not just declare an external function and call it directly?

test/cfi/stats.cpp
2

Does this work with the cross-dso mode?

pcc updated this revision to Diff 44937.Jan 14 2016, 3:32 PM
  • Switch to an alternative in-memory representation that avoids the need for linker magic
pcc updated this revision to Diff 44942.Jan 14 2016, 4:21 PM
pcc marked 2 inline comments as done.
  • Address review comments
lib/stats/stats.cc
41

It doesn't matter, this will never be more than 8.

48

Added a CHECK.

lib/stats/stats_client.cc
81

This won't work on Windows where DSOs can't directly call into the main executable.

test/cfi/stats.cpp
3

Almost; this initially gave duplicate symbol errors. To avoid a combinatorial explosion in the number of static sanitizer libraries I decided to start improving how we link them. First step: link stats without -whole-archive, and use -u to pull in the stats runtime. Also added an integration test here.

pcc updated this revision to Diff 44943.Jan 14 2016, 4:26 PM
  • Add CHECK to SubstituteForFlagValue
lib/sanitizer_common/sanitizer_flags.cc
51

I don't think we can use std::string here.

99

kMaxPathLength is 4096, which is probably enough (we use it in a bunch of other places elsewhere). I don't think this code matters enough to worry about using a variable-size buffer.

I've added a CHECK to the above function.

krasin1 added inline comments.Jan 14 2016, 4:38 PM
lib/sanitizer_common/sanitizer_flags.cc
51

why? not that I insist, I just probably don't understand the constraints and interested to learn about that.

pcc added inline comments.Jan 14 2016, 4:46 PM
lib/sanitizer_common/sanitizer_flags.cc
51

If the C++ standard library is compiled with sanitizer instrumentation, we would have a circular dependency between sanitizer_common and the stdlib.

krasin accepted this revision.Jan 14 2016, 4:48 PM
krasin edited edge metadata.
krasin added inline comments.
lib/sanitizer_common/sanitizer_flags.cc
51

You're right. Thank you for the reminder.

This revision is now accepted and ready to land.Jan 14 2016, 4:48 PM
eugenis added inline comments.Jan 15 2016, 11:02 AM
lib/stats/stats.cc
87

There could be a data race if this is called from concurrent dlopen-ed module constructors.

pcc updated this revision to Diff 45020.Jan 15 2016, 12:17 PM
pcc edited edge metadata.
  • Add mutex to fix data race
eugenis accepted this revision.Jan 15 2016, 1:56 PM
eugenis edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.