Page MenuHomePhabricator

Introduce sanstats tool and llvm::CreateSanitizerStatReport function.
ClosedPublic

Authored by pcc on Jan 13 2016, 8:12 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

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 44829.Jan 13 2016, 8:12 PM
pcc retitled this revision from to Introduce sanstats tool and llvm::CreateSanitizerStatReport function..
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/Transforms/Utils/SanitizerStats.cpp
54 ↗(On Diff #44829)

Is that a correct error handling for the unknown format? Should we warn the user? May be even fail hard?

tools/sanstats/sanstats.cpp
42 ↗(On Diff #44829)

This expression is hard to read. For example, I had to think thrice, if it increments Begin or the value that Begin points to.

Please, split it into a few lines, like:

while (Begin < End && Pos != Size) {

Result |= uint64_t(uint8_t(*Begin)) << (Pos * 8);
Begin++;
Pos++;

}

Also, if that's a hot function, I would recommend to consider a faster implementation that will read the required number of bytes at once, and then swap the bytes with one of the standard helper functions. If this function is rarely called, feel free to leave it as is (modulo my comment above)

48 ↗(On Diff #44829)

isn't a reimplementation of strnlen?

66 ↗(On Diff #44829)

At this point Begin could be greater than End. May be check in the previous line?
Alternatively, make ReadLE to accept a pointer to Begin and let it to update it.

127 ↗(On Diff #44829)

I highly suggest to check Begin < End and optionally check that Begin == End after the loop.

Otherwise, it's not hard to imaging that a tiny mistake somewhere would just Begin slightly past End and then we'll have to read after the end of the buffer until SEGFAULT stops this heroic attempt to overflow uintptr.

pcc updated this revision to Diff 44919.Jan 14 2016, 1:14 PM
pcc marked an inline comment as done.
  • Address review comments
lib/Transforms/Utils/SanitizerStats.cpp
54 ↗(On Diff #44829)

It would probably be appropriate to call report_fatal_error here.

tools/sanstats/sanstats.cpp
48 ↗(On Diff #44829)

Yes, but strnlen is not part of the C standard, and I think it's clearer to be explicit.

66 ↗(On Diff #44829)

At this point Begin could be greater than End. May be check in the previous line?

We defend against this in ReadLE.

Alternatively, make ReadLE to accept a pointer to Begin and let it to update it.

I considered this, but it's harder to distinguish between short reads and EOF this way.

127 ↗(On Diff #44829)

I added an assertion that should catch this.

krasin accepted this revision.Jan 14 2016, 1:22 PM
krasin edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 14 2016, 1:22 PM
pcc updated this revision to Diff 44935.Jan 14 2016, 3:32 PM
pcc edited edge metadata.
  • Switch to an alternative in-memory representation that avoids the need for linker magic
eugenis accepted this revision.Jan 15 2016, 2:01 PM
eugenis edited edge metadata.
This revision was automatically updated to reflect the committed changes.