This is an archive of the discontinued LLVM Phabricator instance.

Use std::piecewise_constant_distribution instead of ad-hoc binary search.
ClosedPublic

Authored by krasin on Jan 21 2016, 10:49 AM.

Details

Summary

Fix the issue with the most recently discovered unit receiving much less attention.

Note: I had to change the seed for one test to make it pass. Alternatively,
the number of runs could be increased. I believe that the average time of
'foo' discovery is not increased, just seed=1 was particularly convenient
for the previous PRNG scheme used.

Diff Detail

Event Timeline

krasin updated this revision to Diff 45566.Jan 21 2016, 10:49 AM
krasin retitled this revision from to Use std::piecewise_constant_distribution instead of ad-hoc binary search..
krasin updated this object.
krasin added a reviewer: aizatsky.
krasin added subscribers: kcc, llvm-commits.
krasin updated this revision to Diff 45568.Jan 21 2016, 10:52 AM

Added a comment.

kcc added inline comments.Jan 21 2016, 11:22 AM
lib/Fuzzer/FuzzerInternal.h
287

We should not be using yet another random generator, we already have one.
Please use it. (USF.GetRand())

krasin added inline comments.Jan 21 2016, 11:28 AM
lib/Fuzzer/FuzzerInternal.h
287

USF.GetRand does not meet the requirements for UniformRandomNumberGenerator: http://en.cppreference.com/w/cpp/concept/UniformRandomNumberGenerator and that is needed to use std::piecewise_constant_distribution.

Should I change USF.GetRand definition to be more standard-compliant?

kcc added inline comments.Jan 21 2016, 11:55 AM
lib/Fuzzer/FuzzerInternal.h
287

Can you *extend* it to be more standard-compliant?

krasin updated this revision to Diff 45594.Jan 21 2016, 2:22 PM

Extend FuzzerRandomBase to satisfy UniformRandomNumberGenerator requirement.

krasin added inline comments.Jan 21 2016, 2:25 PM
lib/Fuzzer/FuzzerInternal.h
287

Done, but I have made an assumption about the max value of Rand(). I imply it's RAND_MAX.

If that could be not the case, the implementation is broken. Please, complain loudly, if so.

kcc added inline comments.Jan 21 2016, 2:37 PM
lib/Fuzzer/FuzzerLoop.cpp
417–419

please use an explicit cast operator.
Are you sure it will ever return the corner values (0 and N-1)?

507

why do you need two vectors here, given that their values are the same?

please avoid sp many push backs, just create a properly sized vector.

the following loop looks like std::iota, please replace with such.

krasin updated this revision to Diff 45613.Jan 21 2016, 4:21 PM

Addressed comments.

Please, take another look.

lib/Fuzzer/FuzzerLoop.cpp
417–419

I've taken a closer look at the generated values. One bug found (not here, though).
This is what I get (histogram for a corpus of size 15)

16606 0
33392 1
49472 2
66160 3
82898 4
99791 5

116215 6
132234 7
149854 8
166098 9
183056 10
199628 11
216078 12
232118 13
248708 14

507

why do you need two vectors here, given that their values are the same?

I need two vectors because one is for intervals and another one for weights.
They are coincidentally similar. That will not be the case, when dynamic weights are introduced.
See http://en.cppreference.com/w/cpp/numeric/random/piecewise_constant_distribution

please avoid sp many push backs, just create a properly sized vector.

Done. Thank you.

the following loop looks like std::iota, please replace with such.

Sorry, I don't understand the request. Can you please rephrase the request or give an example?

kcc added inline comments.Jan 21 2016, 4:31 PM
lib/Fuzzer/FuzzerLoop.cpp
507

a loop like

for (size_t i = 0; i < Corpus.size(); i++)  x[i] = i + 1

can be replaced with
std::iota(x.begin(), x.end(), 1);

krasin updated this revision to Diff 45618.Jan 21 2016, 4:58 PM

Added a unit test for the CorpusDistribution.

krasin added inline comments.Jan 21 2016, 5:01 PM
lib/Fuzzer/FuzzerLoop.cpp
507

ugh. Obscure!

Let me know, if you really want it.

kcc added a comment.Jan 21 2016, 5:08 PM

almost LGTM

lib/Fuzzer/FuzzerLoop.cpp
511

yep, iota would be nicer.
Also, the current code does not initialize Intervals[0]

lib/Fuzzer/test/FuzzerUnittest.cpp
410

why cast from double from size_t.
Better just write 1 << 20.

krasin updated this revision to Diff 45623.Jan 21 2016, 5:20 PM

Use std::iota

lib/Fuzzer/FuzzerLoop.cpp
511

yep, iota would be nicer.

Done

Also, the current code does not initialize Intervals[0]

explicit vector(size_type count) constructor initializes Intervals with all zeros.

lib/Fuzzer/test/FuzzerUnittest.cpp
410

Sorry, I don't understand which cast do you mean here. The function does not cast from double to size_t anywhere.

kcc accepted this revision.Jan 21 2016, 5:23 PM
kcc added a reviewer: kcc.

LGTM with a nit

lib/Fuzzer/test/FuzzerUnittest.cpp
410

1E6 is a floating point constant

This revision is now accepted and ready to land.Jan 21 2016, 5:23 PM
kcc added a comment.Jan 21 2016, 5:23 PM

LGTM with a nit

krasin updated this revision to Diff 45628.Jan 21 2016, 5:29 PM
krasin edited edge metadata.

1E6 -> 1<<20.

lib/Fuzzer/test/FuzzerUnittest.cpp
410

Done.

Thank you. Golang has typeless constants (which get types at the time of assignment). I totally forgot that C does not. :)

krasin closed this revision.Jan 21 2016, 5:36 PM