Initial implementation of a record-level bitcode fuzzer.

Initial implementation of a record-level bitcode fuzzer.

Initial implementation of a record-level bitcod fuzzer for PNaCl
bitcode files. Uses "rand" for the random number generator, as well as
some simplistic rules for generating values for the bitcode records.

The simplistic rules use lists of weighted values, and values are
chosen from the list based on the corresponding weighted distribution.

Also fixes minor bug in the bitstream writer where no checks were
applied to see if an abbreviation definition was outside any scope.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=4169
R=jvoung@chromium.org

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/4dd3b8a933b8eb4fa65ee21051e6e48cfdbdb4bf

[Karl, 2015-05-26 17:28:48]

kschimpf@google.com changed reviewers:
+ dschuff@chromium.org, jvoung@chromium.org, kcc@google.com

[Karl, 2015-05-26 17:31:49]

This is the initial implementation of fuzzing PNaCl bitcode records. Please
review. Thanks.

[Martin Barbella, 2015-05-26 20:06:40]

mbarbella@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Martin Barbella, 2015-05-26 20:06:41]

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:315: if
(Bitcode.getBaseRecords().size() > RAND_MAX)
What sort of sizes are we expecting to see here?

[kcc2, 2015-05-26 20:38:31]

kcc@chromium.org changed reviewers:
+ kcc@chromium.org

[kcc2, 2015-05-26 20:38:32]

https://codereview.chromium.org/1156103003/diff/40001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClFuzz.h (right):

https://codereview.chromium.org/1156103003/diff/40001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:54: /// \brief Generates a new fuzzing of
the bitcode, using the a random
Do you mean "generates a random mutation"?

https://codereview.chromium.org/1156103003/diff/40001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:62: virtual bool fuzz(float MinPercentage)
= 0;
float? 
mmmm. I don't like floats except for when you need floating point arithmetic.

[Karl, 2015-05-26 20:39:21]

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:315: if
(Bitcode.getBaseRecords().size() > RAND_MAX)
On 2015/05/26 20:06:41, mbarbella wrote:
> What sort of sizes are we expecting to see here?

The main reason for this comment is that the documentation states that it only
guarantees that RAND_MAX >= 32767. At that size, multi-megabyte files will have
too many records.

However, I decided not to worry about this for this CL for the following
reasons:

1) When I find a better random number generator (suggestions appreciated), it
would be easy to modify this code.

2) When I tested the version of rand() on my machine, RAND_MAX = 2147480021.
This number is more than large enough to handle several gigabyte sized files,
which is really quite large.

[Martin Barbella, 2015-05-26 20:42:09]

On 2015/05/26 20:39:21, Karl wrote:
>
https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
> File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):
> 
>
https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
> lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:315: if
> (Bitcode.getBaseRecords().size() > RAND_MAX)
> On 2015/05/26 20:06:41, mbarbella wrote:
> > What sort of sizes are we expecting to see here?
> 
> The main reason for this comment is that the documentation states that it only
> guarantees that RAND_MAX >= 32767. At that size, multi-megabyte files will
have
> too many records.
> 
> However, I decided not to worry about this for this CL for the following
> reasons:
> 
> 1) When I find a better random number generator (suggestions appreciated), it
> would be easy to modify this code.

https://code.google.com/p/chromium/codesearch#chromium/src/tools/ipc_fuzzer/f...
might be a good starting point.

> 
> 2) When I tested the version of rand() on my machine, RAND_MAX = 2147480021.
> This number is more than large enough to handle several gigabyte sized files,
> which is really quite large.

[jvoung (off chromium), 2015-05-26 20:43:14]

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:315: if
(Bitcode.getBaseRecords().size() > RAND_MAX)
On 2015/05/26 20:39:21, Karl wrote:
> On 2015/05/26 20:06:41, mbarbella wrote:
> > What sort of sizes are we expecting to see here?
> 
> The main reason for this comment is that the documentation states that it only
> guarantees that RAND_MAX >= 32767. At that size, multi-megabyte files will
have
> too many records.
> 
> However, I decided not to worry about this for this CL for the following
> reasons:
> 
> 1) When I find a better random number generator (suggestions appreciated), it
> would be easy to modify this code.
> 
> 2) When I tested the version of rand() on my machine, RAND_MAX = 2147480021.
> This number is more than large enough to handle several gigabyte sized files,
> which is really quite large.

http://llvm.org/docs/doxygen/html/RandomNumberGenerator_8h_source.html

?

[Karl, 2015-05-29 20:59:34]

https://codereview.chromium.org/1156103003/diff/40001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClFuzz.h (right):

https://codereview.chromium.org/1156103003/diff/40001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:54: /// \brief Generates a new fuzzing of
the bitcode, using the a random
On 2015/05/26 20:38:32, kcc2 wrote:
> Do you mean "generates a random mutation"?

Done.

https://codereview.chromium.org/1156103003/diff/40001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:62: virtual bool fuzz(float MinPercentage)
= 0;
On 2015/05/26 20:38:32, kcc2 wrote:
> float? 
> mmmm. I don't like floats except for when you need floating point arithmetic. 

Changed to pass in Count/Base (integer) values to define percentage.

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:315: if
(Bitcode.getBaseRecords().size() > RAND_MAX)
On 2015/05/26 20:43:14, jvoung wrote:
> On 2015/05/26 20:39:21, Karl wrote:
> > On 2015/05/26 20:06:41, mbarbella wrote:
> > > What sort of sizes are we expecting to see here?
> > 
> > The main reason for this comment is that the documentation states that it
only
> > guarantees that RAND_MAX >= 32767. At that size, multi-megabyte files will
> have
> > too many records.
> > 
> > However, I decided not to worry about this for this CL for the following
> > reasons:
> > 
> > 1) When I find a better random number generator (suggestions appreciated),
it
> > would be easy to modify this code.
> > 
> > 2) When I tested the version of rand() on my machine, RAND_MAX = 2147480021.
> > This number is more than large enough to handle several gigabyte sized
files,
> > which is really quite large.
> 
> http://llvm.org/docs/doxygen/html/RandomNumberGenerator_8h_source.html
> 
> ?

Modified API to allow the random number generator to be passed in. Also defined
a default random number generator based on C++ <random>, which follows
implmentation suggested by Jan.

[jvoung (off chromium), 2015-06-01 17:26:36]

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClFuzz.h (right):

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:13: // number generator.  As a result, this
code is not thread safe.
Close the header description with another line of

//===----------------------------------------------------------------------===//

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:58: /// Many be called an arbitrary number
of times. Results are left in
Many be -> May be

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:80: explicit RecordFuzzer(NaClMungedBitcode
&Bitcode,
No longer need explicit w/ two params.

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClRandNumGen.h (right):

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClRandNumGen.h:14: // thread safe.
same

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClRandNumGen.h:44:
DefaultRandomNumberGenerator(std::string Seed);
StringRef or const std::string &

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClRandNumGen.h:46: virtual
~DefaultRandomNumberGenerator() {}
override instead of virtual?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:1: //===- NaClFuzz.h - Fuzz PNaCl
bitcode records ------------------*- C++ -*-===//
NaClFuzz.cpp

No need to have the -*- C++... stuff in a .cpp file (just the .h).

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:10: // This file implements a basic
fuzzer for a list of PNaCl bitcode records.
//===----------------------------------------------------------------------===//

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:37: } // end of namespace naclfuzz
This is anonymous namespace instead of namespace naclfuzz

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp (right):

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:1: //===- NaClRandNumGen.cpp -
Random number generator -------------*- C++ -*-===//
no need for -*- C++ -*-

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:10: // This file provides a
default implementation of a random number generator.
//===----------------------------------------------------------------------===//

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:19: // Put destructor in .cpp file
go guarantee vtable construction.
"go guarantee" -> "to guarantee"

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:24: setSeed(0);
How about call the method saltSeed() (can salt be a verb too) or setSalt()?
Otherwise you Seed(Seed) followed by setSeed(0), which may look odd if you
didn't read the comment on what setSeed does.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:42: } // end of namespace nacl
naclfuzz

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:1: //===-
NaClSimpleRecordFuzzer.cpp - Simple fuzzer of bitcode ----*- C++ -*-===//
No need for -*- C++...

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:10: // This file
implements a simple fuzzer for a list of PNaCl bitcode records.
//===----------------------------------------------------------------------===//

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:69: // Association weights
with values. Used to generate weighted
"Association weights with values" -> Association of weights with values" or
"Associates weights with values" ?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:124: // The number of
weighed values in the distribution.
The number of weighted values ?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:133: // chosen element.
"""
// ... of the distribution.
// chosen element."
"""

Missing some words in last sentence?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:170:
this->Generator.chooseInRange(Range.limit - Range.min + 1);
Just checking -- Should this really have + 1?

The comments say Range is [min..limit). So for example, for integers in [1..2)
will only have the value 1, but chooseInRange() will get (2 - 1 + 1) and choose
between 2 things?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:222: // Weighted ranges
for nonegative values in records.
nonegative -> non-negative

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:252: // each record code
appeared (or 1 if less than 1 thousand).
There is some overlap in the way the codes are numbered (e.g.,
TYPE_CODE_NUMENTRY == FUNC_CODE_DECLAREBLOCKS), but I guess that shouldn't throw
off the distribution much even if the fuzzer didn't limit choices to the
appropriate block.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:304: explicit
SimpleRecordFuzzer(NaClMungedBitcode &Bitcode,
no need for explicit

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:317: for (const auto
RecordCode : RecordCodeWeight)
"auto &" or ? Can't tell if it's already a reference or a copy.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:319: 
extra blank line

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:378: SignedValueType
SignedValue = static_cast<SignedValueType>(Value);
I think technically, if Value > INT_MAX then casting from unsigned -> signed is
implementation-defined behavior.

choosePositiveValue/PosValueDist usually won't go beyond INT_MAX, but might be
worth asserting?

Or perhaps you can do the two's complement negation in the unsigned type, and
stick with ValueType and not use SignedValueType?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:401: // TODO: Do something
smarter than just changing a value
TODO(kschimpf) ?

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:461: // Returns
corresponding percenage defined by Count/Total, in a form
 percenage ->  percentage

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:494: } // end of anonymous
"end of anonymous namespace" ? Don't remember the exact style...

https://codereview.chromium.org/1156103003/diff/80001/tools/CMakeLists.txt
File tools/CMakeLists.txt (right):

https://codereview.chromium.org/1156103003/diff/80001/tools/CMakeLists.txt#ne...
tools/CMakeLists.txt:62: add_llvm_tool_subdirectory(pnacl-bcfuzz)
keep alphabetical -- modulo llvm stuff?

https://codereview.chromium.org/1156103003/diff/80001/tools/LLVMBuild.txt
File tools/LLVMBuild.txt (right):

https://codereview.chromium.org/1156103003/diff/80001/tools/LLVMBuild.txt#new...
tools/LLVMBuild.txt:19: subdirectories = pnacl-llc pnacl-abicheck
pnacl-bcanalyzer pnacl-bccompress pnacl-bcdis pnacl-freeze pnacl-bcfuzz
pnacl-thaw bugpoint llc lli llvm-ar llvm-as llvm-bcanalyzer llvm-cov llvm-diff
llvm-dis llvm-dwarfdump llvm-extract llvm-jitlistener llvm-link llvm-lto llvm-mc
llvm-nm llvm-objdump llvm-pdbdump llvm-profdata llvm-rtdyld llvm-size macho-dump
opt llvm-mcmarkup verify-uselistorder dsymutil
alphabetical, modulo llvm stuff?

https://codereview.chromium.org/1156103003/diff/80001/tools/Makefile
File tools/Makefile (right):

https://codereview.chromium.org/1156103003/diff/80001/tools/Makefile#newcode34
tools/Makefile:34: pnacl-bcfuzz pnacl-thaw pnacl-bccompress pnacl-bcdis \
I don't see a new pnacl-bcfuzz directory

[Karl, 2015-06-01 22:40:56]

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClFuzz.h (right):

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:13: // number generator.  As a result, this
code is not thread safe.
On 2015/06/01 17:26:35, jvoung wrote:
> Close the header description with another line of
> 
>
//===----------------------------------------------------------------------===//

Done.

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:58: /// Many be called an arbitrary number
of times. Results are left in
On 2015/06/01 17:26:34, jvoung wrote:
> Many be -> May be

Done.

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClFuzz.h:80: explicit RecordFuzzer(NaClMungedBitcode
&Bitcode,
On 2015/06/01 17:26:34, jvoung wrote:
> No longer need explicit w/ two params.

Done.

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClRandNumGen.h (right):

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClRandNumGen.h:14: // thread safe.
On 2015/06/01 17:26:35, jvoung wrote:
> same

Done.

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClRandNumGen.h:44:
DefaultRandomNumberGenerator(std::string Seed);
On 2015/06/01 17:26:35, jvoung wrote:
> StringRef or const std::string &

Done.

https://codereview.chromium.org/1156103003/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClRandNumGen.h:46: virtual
~DefaultRandomNumberGenerator() {}
On 2015/06/01 17:26:35, jvoung wrote:
> override instead of virtual?

Changed to final.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:1: //===- NaClFuzz.h - Fuzz PNaCl
bitcode records ------------------*- C++ -*-===//
On 2015/06/01 17:26:35, jvoung wrote:
> NaClFuzz.cpp
> 
> No need to have the -*- C++... stuff in a .cpp file (just the .h).

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:10: // This file implements a basic
fuzzer for a list of PNaCl bitcode records.
On 2015/06/01 17:26:35, jvoung wrote:
>
//===----------------------------------------------------------------------===//

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:37: } // end of namespace naclfuzz
On 2015/06/01 17:26:35, jvoung wrote:
> This is anonymous namespace instead of namespace naclfuzz

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp (right):

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:1: //===- NaClRandNumGen.cpp -
Random number generator -------------*- C++ -*-===//
On 2015/06/01 17:26:35, jvoung wrote:
> no need for -*- C++ -*-

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:10: // This file provides a
default implementation of a random number generator.
On 2015/06/01 17:26:35, jvoung wrote:
>
//===----------------------------------------------------------------------===//

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:19: // Put destructor in .cpp file
go guarantee vtable construction.
On 2015/06/01 17:26:35, jvoung wrote:
> "go guarantee" -> "to guarantee"

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:24: setSeed(0);
On 2015/06/01 17:26:35, jvoung wrote:
> How about call the method saltSeed() (can salt be a verb too) or setSalt()?
> Otherwise you Seed(Seed) followed by setSeed(0), which may look odd if you
> didn't read the comment on what setSeed does.

Changed to saltSeed.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClRandNumGen.cpp:42: } // end of namespace nacl
On 2015/06/01 17:26:35, jvoung wrote:
> naclfuzz

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:1: //===-
NaClSimpleRecordFuzzer.cpp - Simple fuzzer of bitcode ----*- C++ -*-===//
On 2015/06/01 17:26:36, jvoung wrote:
> No need for -*- C++...

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:10: // This file
implements a simple fuzzer for a list of PNaCl bitcode records.
On 2015/06/01 17:26:36, jvoung wrote:
>
//===----------------------------------------------------------------------===//

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:69: // Association weights
with values. Used to generate weighted
On 2015/06/01 17:26:36, jvoung wrote:
> "Association weights with values" -> Association of weights with values" or
> "Associates weights with values" ?
Chose "Associates weights with values".

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:124: // The number of
weighed values in the distribution.
On 2015/06/01 17:26:36, jvoung wrote:
> The number of weighted values ?

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:133: // chosen element.
On 2015/06/01 17:26:35, jvoung wrote:
> """
> // ... of the distribution.
> // chosen element."
> """
> 
> Missing some words in last sentence?

Removed last sentence.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:170:
this->Generator.chooseInRange(Range.limit - Range.min + 1);
On 2015/06/01 17:26:36, jvoung wrote:
> Just checking -- Should this really have + 1?
> 
> The comments say Range is [min..limit). So for example, for integers in [1..2)
> will only have the value 1, but chooseInRange() will get (2 - 1 + 1) and
choose
> between 2 things?

Hmm. I guess I meant a range is [min..max] rather than [min..limit). The uses
match
the form [min..max]. Changing code to match.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:222: // Weighted ranges
for nonegative values in records.
On 2015/06/01 17:26:36, jvoung wrote:
> nonegative -> non-negative

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:252: // each record code
appeared (or 1 if less than 1 thousand).
On 2015/06/01 17:26:35, jvoung wrote:
> There is some overlap in the way the codes are numbered (e.g.,
> TYPE_CODE_NUMENTRY == FUNC_CODE_DECLAREBLOCKS), but I guess that shouldn't
throw
> off the distribution much even if the fuzzer didn't limit choices to the
> appropriate block.

I agree that there are overlapping entries, since they cross block boundaries.
It still shouldn't effect the distribution much. I see a possible future
extension where we might associate record form information with record codes. In
such cases, we are already set up for that (by using chooseIndex).

It also meant that I did not have to figure out which record codes should be
merged because they have the same value.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:304: explicit
SimpleRecordFuzzer(NaClMungedBitcode &Bitcode,
On 2015/06/01 17:26:36, jvoung wrote:
> no need for explicit

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:317: for (const auto
RecordCode : RecordCodeWeight)
On 2015/06/01 17:26:36, jvoung wrote:
> "auto &" or ? Can't tell if it's already a reference or a copy.

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:319: 
On 2015/06/01 17:26:36, jvoung wrote:
> extra blank line

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:378: SignedValueType
SignedValue = static_cast<SignedValueType>(Value);
On 2015/06/01 17:26:35, jvoung wrote:
> I think technically, if Value > INT_MAX then casting from unsigned -> signed
is
> implementation-defined behavior.
> 
> choosePositiveValue/PosValueDist usually won't go beyond INT_MAX, but might be
> worth asserting?
> 
> Or perhaps you can do the two's complement negation in the unsigned type, and
> stick with ValueType and not use SignedValueType?

Choosing to use two's complement negation.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:401: // TODO: Do something
smarter than just changing a value
On 2015/06/01 17:26:35, jvoung wrote:
> TODO(kschimpf) ?

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:461: // Returns
corresponding percenage defined by Count/Total, in a form
On 2015/06/01 17:26:35, jvoung wrote:
>  percenage ->  percentage

Done.

https://codereview.chromium.org/1156103003/diff/80001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:494: } // end of anonymous
On 2015/06/01 17:26:36, jvoung wrote:
> "end of anonymous namespace" ? Don't remember the exact style...

Done.

https://codereview.chromium.org/1156103003/diff/80001/tools/CMakeLists.txt
File tools/CMakeLists.txt (right):

https://codereview.chromium.org/1156103003/diff/80001/tools/CMakeLists.txt#ne...
tools/CMakeLists.txt:62: add_llvm_tool_subdirectory(pnacl-bcfuzz)
On 2015/06/01 17:26:36, jvoung wrote:
> keep alphabetical -- modulo llvm stuff?

Done.

https://codereview.chromium.org/1156103003/diff/80001/tools/LLVMBuild.txt
File tools/LLVMBuild.txt (right):

https://codereview.chromium.org/1156103003/diff/80001/tools/LLVMBuild.txt#new...
tools/LLVMBuild.txt:19: subdirectories = pnacl-llc pnacl-abicheck
pnacl-bcanalyzer pnacl-bccompress pnacl-bcdis pnacl-freeze pnacl-bcfuzz
pnacl-thaw bugpoint llc lli llvm-ar llvm-as llvm-bcanalyzer llvm-cov llvm-diff
llvm-dis llvm-dwarfdump llvm-extract llvm-jitlistener llvm-link llvm-lto llvm-mc
llvm-nm llvm-objdump llvm-pdbdump llvm-profdata llvm-rtdyld llvm-size macho-dump
opt llvm-mcmarkup verify-uselistorder dsymutil
On 2015/06/01 17:26:36, jvoung wrote:
> alphabetical, modulo llvm stuff?

Done.

https://codereview.chromium.org/1156103003/diff/80001/tools/Makefile
File tools/Makefile (right):

https://codereview.chromium.org/1156103003/diff/80001/tools/Makefile#newcode34
tools/Makefile:34: pnacl-bcfuzz pnacl-thaw pnacl-bccompress pnacl-bcdis \
On 2015/06/01 17:26:36, jvoung wrote:
> I don't see a new pnacl-bcfuzz directory

Oops. Fixing.

[jvoung (off chromium), 2015-06-02 00:32:06]

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/CMa...
File tools/pnacl-bcfuzz/CMakeLists.txt (right):

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/CMa...
tools/pnacl-bcfuzz/CMakeLists.txt:2: BitWriter
NaClBitWriter ?

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
File tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:69: std::string OutputFilename) {
could be a StringRef or std::string &

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:95: if (!(0.0 <= PercentageToEdit  &&
PercentageToEdit <= 1.0)) {
no longer a float between 0 and 1... there's a base?

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:107:
EC(MemoryBuffer::getFile(InputFilename));
Is there a better variable name than "EC" (EC is more like error code than some
input memory buffer)?

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:122: DefaultRandomNumberGenerator
Generator(RandomSeed);
RandSeed

[Karl, 2015-06-02 15:42:32]

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/CMa...
File tools/pnacl-bcfuzz/CMakeLists.txt (right):

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/CMa...
tools/pnacl-bcfuzz/CMakeLists.txt:2: BitWriter
On 2015/06/02 00:32:06, jvoung wrote:
> NaClBitWriter ?

Good catch! Fixing.

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
File tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:69: std::string OutputFilename) {
On 2015/06/02 00:32:06, jvoung wrote:
> could be a StringRef or std::string &

Done.

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:95: if (!(0.0 <= PercentageToEdit  &&
PercentageToEdit <= 1.0)) {
On 2015/06/02 00:32:06, jvoung wrote:
> no longer a float between 0 and 1... there's a base?

Fixed.

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:107:
EC(MemoryBuffer::getFile(InputFilename));
On 2015/06/02 00:32:06, jvoung wrote:
> Is there a better variable name than "EC" (EC is more like error code than
some
> input memory buffer)?

Done.

https://codereview.chromium.org/1156103003/diff/120001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:122: DefaultRandomNumberGenerator
Generator(RandomSeed);
On 2015/06/02 00:32:06, jvoung wrote:
> RandSeed

Done.

[jvoung (off chromium), 2015-06-02 16:14:03]

https://codereview.chromium.org/1156103003/diff/140001/include/llvm/Bitcode/N...
File include/llvm/Bitcode/NaCl/NaClFuzz.h (right):

https://codereview.chromium.org/1156103003/diff/140001/include/llvm/Bitcode/N...
include/llvm/Bitcode/NaCl/NaClFuzz.h:24: #include <random>
llvm/Bitcode/NaCl/NaClRandNumGen.h already includes <random>

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
File lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:1: //===- NaClFuzz.h - Fuzz PNaCl
bitcode records ----------------------------===//
NaClFuzz.cpp

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:16: #include "llvm/Support/Format.h"
can some of this be trimmed?

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:21: #include <map>
where is <map> <set> <cmath> etc. used?

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:26: using namespace naclfuzz;
Do you really need "using namespace naclfuzz"? Most of the source here is
already under namespace naclfuzz { }.

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:18: #include <array>
std::array isn't actually used?

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:21: #include <map>
where is <map> used?

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/CMa...
File tools/pnacl-bcfuzz/CMakeLists.txt (right):

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/CMa...
tools/pnacl-bcfuzz/CMakeLists.txt:2: NaClBitWriter
sort?

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/LLV...
File tools/pnacl-bcfuzz/LLVMBuild.txt (right):

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/LLV...
tools/pnacl-bcfuzz/LLVMBuild.txt:2:
;===------------------------------------------------------------------------===;
This extra ;===------===; line seems extra.

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/pna...
File tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:2: //
//                     The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:98: << PercentageToEdit << "\n";
"... must not exceed: " << PercentageBase

or something.

[Karl, 2015-06-02 16:40:23]

https://codereview.chromium.org/1156103003/diff/140001/include/llvm/Bitcode/N...
File include/llvm/Bitcode/NaCl/NaClFuzz.h (right):

https://codereview.chromium.org/1156103003/diff/140001/include/llvm/Bitcode/N...
include/llvm/Bitcode/NaCl/NaClFuzz.h:24: #include <random>
On 2015/06/02 16:14:03, jvoung wrote:
> llvm/Bitcode/NaCl/NaClRandNumGen.h already includes <random>

Removed. Also removed SmallVector.h

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
File lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:1: //===- NaClFuzz.h - Fuzz PNaCl
bitcode records ----------------------------===//
On 2015/06/02 16:14:03, jvoung wrote:
> NaClFuzz.cpp

Done.

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:16: #include "llvm/Support/Format.h"
On 2015/06/02 16:14:03, jvoung wrote:
> can some of this be trimmed?

Done. Removed Format.h

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:21: #include <map>
On 2015/06/02 16:14:03, jvoung wrote:
> where is <map> <set> <cmath> etc. used?

Removed all <...> includes.

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClFuzz.cpp:26: using namespace naclfuzz;
On 2015/06/02 16:14:03, jvoung wrote:
> Do you really need "using namespace naclfuzz"? Most of the source here is
> already under namespace naclfuzz { }.

Sorry for all the unnecessary boilerplate. Most of the code that necessitated
this was moved to NaClSimpleRecordFuzzer.cpp. I simply forgot to clean this file
up after the move.

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
File lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp (right):

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:18: #include <array>
On 2015/06/02 16:14:03, jvoung wrote:
> std::array isn't actually used?

Done.

https://codereview.chromium.org/1156103003/diff/140001/lib/Bitcode/NaCl/TestU...
lib/Bitcode/NaCl/TestUtils/NaClSimpleRecordFuzzer.cpp:21: #include <map>
On 2015/06/02 16:14:03, jvoung wrote:
> where is <map> used?

Got rid of all <...> but <set>

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/CMa...
File tools/pnacl-bcfuzz/CMakeLists.txt (right):

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/CMa...
tools/pnacl-bcfuzz/CMakeLists.txt:2: NaClBitWriter
On 2015/06/02 16:14:03, jvoung wrote:
> sort?

Done.

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/LLV...
File tools/pnacl-bcfuzz/LLVMBuild.txt (right):

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/LLV...
tools/pnacl-bcfuzz/LLVMBuild.txt:2:
;===------------------------------------------------------------------------===;
On 2015/06/02 16:14:03, jvoung wrote:
> This extra ;===------===; line seems extra.

Done.

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/pna...
File tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp (right):

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:2: //
On 2015/06/02 16:14:03, jvoung wrote:
> //                     The LLVM Compiler Infrastructure
> //
> // This file is distributed under the University of Illinois Open Source
> // License. See LICENSE.TXT for details.

Done.

https://codereview.chromium.org/1156103003/diff/140001/tools/pnacl-bcfuzz/pna...
tools/pnacl-bcfuzz/pnacl-bcfuzz.cpp:98: << PercentageToEdit << "\n";
On 2015/06/02 16:14:03, jvoung wrote:
> "... must not exceed: " << PercentageBase
> 
> or something.

Done.

[jvoung (off chromium), 2015-06-02 16:47:21]

lgtm

[Karl, 2015-06-02 16:53:36]

Committed patchset #9 (id:160001) manually as
4dd3b8a933b8eb4fa65ee21051e6e48cfdbdb4bf (presubmit successful).