This is an archive of the discontinued LLVM Phabricator instance.

[RFC] Change cmp instrumentation to distinguish comparisons with const operands
ClosedPublic

Authored by tch1bo on Aug 8 2017, 6:45 AM.

Details

Reviewers
glider
kcc
dvyukov
Summary

This implementation of SanitizerCoverage instrumentation inserts different callbacks depending on constantness of operands:

  1. If both operands are non-const, then a usual __sanitizer_cov_trace_cmp[1248] call is inserted.
  2. If exactly one operand is const, then a __sanitizer_cov_trace_const_cmp[1248] call is inserted. The first argument of the call is always the constant one.
  3. If both operands are const, then no callback is inserted.

This separation comes useful in fuzzing when tasks like "find one operand of the comparison in input arguments and replace it with the other one" have to be done. The new instrumentation allows us to not waste time on searching the constant operands in the input.

Diff Detail

Event Timeline

tch1bo created this revision.Aug 8 2017, 6:45 AM
glider added inline comments.Aug 8 2017, 7:31 AM
lib/Transforms/Instrumentation/SanitizerCoverage.cpp
675

Pay attention to the comment style: single-line comments start with a capital letter and end with a period.

677

Ditto.

679

Nested code should be indented with two spaces.

680

You don't need braces for one-line if body.

test/Instrumentation/SanitizerCoverage/const-cmp-tracing.ll
31

Please add tests for swapped arguments, e.g. "icmp slt 1, i8 %x"

glider edited edge metadata.

Kostya, Dima,

Do we need to put this code behind a flag (probably yes, if we don't want to break the API for the existing users)?
Any other comments?

dvyukov edited edge metadata.Aug 8 2017, 7:51 AM

Kostya, Dima,

Do we need to put this code behind a flag (probably yes, if we don't want to break the API for the existing users)?
Any other comments?

We don't yet have support for this in kernel. When we add it, we add both const and non-const versions. So API issues only affect user-space. So the question goes to Kostya.

kcc edited edge metadata.Aug 8 2017, 11:11 AM

I am fine with breaking the API. Whoever uses this API will need to add a tiny bit of code to work with the new compiler (and that code will be compatible with the old version).
It's better to break the API this way than to introduce the flags or weak aliases, etc

Please update the documentation in this patch. (mention that these functions are emitted by clang >= 2017-08-XX)

Make sure to run 'ninja check-fuzzer'. Most likely it will break due to the API change, fix it in libFuzzer (in a simple way, by calling __sanitizer_cov_trace_cmpN)

Find all uses of __sanitizer_cov_trace_cmp1 in projects/compiler-rt/lib/sanitizer_common and update those files.

test/Instrumentation/SanitizerCoverage/const-cmp-tracing.ll
22

Adding CHECK-NEXT is a good idea, do it here and below

27

Add
CHECK: icmp slt i32 1, 0

In D36465#835650, @kcc wrote:

I am fine with breaking the API. Whoever uses this API will need to add a tiny bit of code to work with the new compiler (and that code will be compatible with the old version).
It's better to break the API this way than to introduce the flags or weak aliases, etc

Please update the documentation in this patch. (mention that these functions are emitted by clang >= 2017-08-XX)

Make sure to run 'ninja check-fuzzer'. Most likely it will break due to the API change, fix it in libFuzzer (in a simple way, by calling __sanitizer_cov_trace_cmpN)

Find all uses of __sanitizer_cov_trace_cmp1 in projects/compiler-rt/lib/sanitizer_common and update those files.

To save one hop, who do you want this to be split in patches?

kcc added a comment.Aug 8 2017, 11:37 AM

To save one hop, who do you want this to be split in patches?

Up to you. But please make sure to not break the bots.

tch1bo updated this revision to Diff 110386.Aug 9 2017, 7:13 AM
glider added inline comments.Aug 9 2017, 7:49 AM
tools/clang/docs/SanitizerCoverage.rst
207

Change to "Called before a comparison instruction with non-constant arguments."

231

No need to write the dates here, this isn't a changelog.

243

These are already listed at line 209. I suggest you move the above block there.

247

Extra newline, please remove.

tch1bo updated this revision to Diff 110413.Aug 9 2017, 8:58 AM
tch1bo marked 11 inline comments as done.
kcc accepted this revision.Aug 9 2017, 1:04 PM

LGTM with nits, thanks!

tools/clang/docs/SanitizerCoverage.rst
235

Move this inside the first block, just under __sanitizer_cov_trace_cmp8

Add a comment:
Arg1 and Arg2 are arguments of the comparison, Arg1 is a compile-time constant.
These callbacks are emitted by -fsanitize-coverage=trace-cmp since 2011-08-XX

242

this comment is redundant.

This revision is now accepted and ready to land.Aug 9 2017, 1:04 PM
tch1bo updated this revision to Diff 110540.Aug 10 2017, 2:29 AM
glider closed this revision.Aug 10 2017, 8:01 AM

Landed r310600.
Good job!