This is an archive of the discontinued LLVM Phabricator instance.

Generate error reports when a fuzz target exits.
ClosedPublic

Authored by morehouse on Jul 18 2017, 7:15 PM.

Details

Summary

Implements https://github.com/google/sanitizers/issues/835.

Flush stdout before exiting in test cases.

Since the atexit hook is used for exit reports, pending prints to
stdout can be lost if they aren't flushed before calling exit().

Expect tests to have non-zero exit code if exit() is called.

Event Timeline

morehouse created this revision.Jul 18 2017, 7:15 PM
morehouse edited the summary of this revision. (Show Details)Jul 18 2017, 7:18 PM
kcc accepted this revision.Jul 19 2017, 10:20 AM

The diff is done from $(LLVM)/..
Please make it from ($LLVM) instead
LGTM otherwise.
Also, please apply for the commit access.

llvm/lib/Fuzzer/test/SimpleThreadedTest.cpp
16

redundant change there? (abort, not exit)

This revision is now accepted and ready to land.Jul 19 2017, 10:20 AM
In D35602#814861, @kcc wrote:

The diff is done from $(LLVM)/..
Please make it from ($LLVM) instead

I don't quite understand what this means. Could you explain further?

kcc added a comment.Jul 19 2017, 10:33 AM

When I do "arc patch" locally it works for me only if the paths are like "lib/Fuzzer/test/fuzzer.test", not "llvm/lib/Fuzzer/test/fuzzer.test".
(Maybe I am missing something in how arc might help me... )

morehouse closed this revision.Jul 20 2017, 1:45 PM

Pushed in r308669.

Dor1s added a subscriber: Dor1s.Oct 3 2017, 2:37 PM

Sorry for necroposting, but there are some compatibility issues with Clang coverage (see my comment on FuzzerDriver.cpp#646). What could be a good workaround?

llvm/lib/Fuzzer/FuzzerDriver.cpp
646

This change breaks Clang Source-based Code Coverage. For example, if you run ./fuzzer -runs=0 ./corpus_dir with 123 files in corpus_dir, you'll see LLVMFuzzerTestOneInput executed only once, not 123 times.

We've temporarily rolled libFuzzer back Chromium because of that.

One option may be to add a flag to disable the exit hook and use that when doing coverage builds. Actually I thought I included a flag in this revision, but apparently not.

morehouse added a comment.EditedOct 3 2017, 5:08 PM

Also, are you sure this change is what broke coverage? I can replicate this issue at ToT libFuzzer, but not at r308669.

And if I comment out the atexit hook at ToT, the issue persists for me.

Dor1s added a comment.Oct 4 2017, 7:52 AM

Matt, you are right, this CL is not a culprit. Sorry for the false signal. Now I found the real issue, it is the following revision: https://chromium.googlesource.com/chromium/llvm-project/compiler-rt/lib/fuzzer.git/+/1c0379f93124d95b87f3b62bf61d90882a669387

In particular, the new call added below is causing that:

diff --git a/FuzzerTracePC.h b/FuzzerTracePC.h
index ea6794c..56f1820 100644
--- a/FuzzerTracePC.h
+++ b/FuzzerTracePC.h
@@ -91,6 +91,7 @@
       memset(Counters(), 0, GetNumPCs());
     ClearExtraCounters();
     ClearInlineCounters();
+    ClearClangCounters();
   }

Disabling that with an extra cmd flag doesn't sound as a good solution to me. It would complicate usage for the end user (a regular developer) a little bit more. Can we think of anything else? //cc @kcc

Dor1s added a comment.Oct 6 2017, 7:57 AM

Discussed with Kostya and fixed: https://reviews.llvm.org/rL315029