i#1734 Dr. Fuzz: add code coverage based fuzzing framework

Commit log for first patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_algo to specify different fuzzing algorithm
- add drfuzz_mutator_feedback in drfuzz to allow client provide feedback
  to drfuzz mutator
- add fuzzer_bb_event to track the number of new basic blocks seen each
  fuzzing iteration
- provide feedback to drfuzz in post_fuzz
---------------

[zhaoqin, 2015-12-15 16:50:47]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_algo to specify different fuzzing algorithms
- add drfuzz_mutator_feedback in drfuzz to allow client provide feedback
  to drfuzz mutator
- add fuzzer_bb_event to track the number of new basic blocks seen each
  fuzzing iteration
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[zhaoqin, 2015-12-15 16:57:22]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_algo to specify different fuzzing algorithms
- add drfuzz_mutator_feedback in drfuzz to allow client provide feedback
  to drfuzz mutator
- add fuzzer_bb_event to track the number of new basic blocks seen each
  fuzzing iteration
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[bruening, 2015-12-15 21:08:33]

It seems like this is a key part of many fuzzing frameworks and that it should
be inside drfuzz, rather than in drmemory.  WDYT?

[zhaoqin, 2015-12-15 21:37:35]

On 2015/12/15 21:08:33, bruening wrote:
> It seems like this is a key part of many fuzzing frameworks and that it should
> be inside drfuzz, rather than in drmemory.  WDYT?

I thought about it. How do you think we should put it in drfuzz?
You mean the drfuzz itself register the bb event and collect code coverage?
In that case, what if we want to add the edge coverage?
Do we want to add the feedback api, which would be more flexible for client
providing different feedback?
If we provide the feedback api, and drfuzz itself collect code coverage, how do
we handle the conflict?

[bruening, 2015-12-15 21:39:51]

On 2015/12/15 21:37:35, zhaoqin wrote:
> On 2015/12/15 21:08:33, bruening wrote:
> > It seems like this is a key part of many fuzzing frameworks and that it
should
> > be inside drfuzz, rather than in drmemory.  WDYT?
> 
> I thought about it. How do you think we should put it in drfuzz?
> You mean the drfuzz itself register the bb event and collect code coverage?

Yes.

> In that case, what if we want to add the edge coverage?

If it's "we", we add it to drfuzz.  If it's a user, the user turns off default
coverage and implements edge coverage in his own client and passes its info to
the mutator via the feedback API.

> Do we want to add the feedback api, which would be more flexible for client
> providing different feedback?

Yes, the mutator is separated component of drfuzz that the user can replace.

> If we provide the feedback api, and drfuzz itself collect code coverage, how
do
> we handle the conflict?

I do not understand the question.  If default coverage is turned on, drfuzz will
invoke the mutator's feedback routine.

[zhaoqin, 2015-12-15 21:44:14]

On 2015/12/15 21:39:51, bruening wrote:
> On 2015/12/15 21:37:35, zhaoqin wrote:
> > On 2015/12/15 21:08:33, bruening wrote:
> > > It seems like this is a key part of many fuzzing frameworks and that it
> should
> > > be inside drfuzz, rather than in drmemory.  WDYT?
> > 
> > I thought about it. How do you think we should put it in drfuzz?
> > You mean the drfuzz itself register the bb event and collect code coverage?
> 
> Yes.

ok, in that case, we need add something in the options to explicitly say code
coverage is needed.
A new algo?

> 
> > In that case, what if we want to add the edge coverage?
> 
> If it's "we", we add it to drfuzz.  If it's a user, the user turns off default
> coverage and implements edge coverage in his own client and passes its info to
> the mutator via the feedback API.
> 
> > Do we want to add the feedback api, which would be more flexible for client
> > providing different feedback?
> 
> Yes, the mutator is separated component of drfuzz that the user can replace.
> 
> > If we provide the feedback api, and drfuzz itself collect code coverage, how
> do
> > we handle the conflict?
> 
> I do not understand the question.  If default coverage is turned on, drfuzz
will
> invoke the mutator's feedback routine.

[bruening, 2015-12-15 21:46:45]

On 2015/12/15 21:44:14, zhaoqin wrote:
> On 2015/12/15 21:39:51, bruening wrote:
> > On 2015/12/15 21:37:35, zhaoqin wrote:
> > > On 2015/12/15 21:08:33, bruening wrote:
> > > > It seems like this is a key part of many fuzzing frameworks and that it
> > should
> > > > be inside drfuzz, rather than in drmemory.  WDYT?
> > > 
> > > I thought about it. How do you think we should put it in drfuzz?
> > > You mean the drfuzz itself register the bb event and collect code
coverage?
> > 
> > Yes.
> 
> ok, in that case, we need add something in the options to explicitly say code
> coverage is needed.
> A new algo?

If you mean a mutator sub-option, I would say no, it should be separate.  Some
separate option like -fuzz_use_cov or sthg.

[zhaoqin, 2015-12-15 21:59:52]

On 2015/12/15 21:46:45, bruening wrote:
> On 2015/12/15 21:44:14, zhaoqin wrote:
> > On 2015/12/15 21:39:51, bruening wrote:
> > > On 2015/12/15 21:37:35, zhaoqin wrote:
> > > > On 2015/12/15 21:08:33, bruening wrote:
> > > > > It seems like this is a key part of many fuzzing frameworks and that
it
> > > should
> > > > > be inside drfuzz, rather than in drmemory.  WDYT?
> > > > 
> > > > I thought about it. How do you think we should put it in drfuzz?
> > > > You mean the drfuzz itself register the bb event and collect code
> coverage?
> > > 
> > > Yes.
> > 
> > ok, in that case, we need add something in the options to explicitly say
code
> > coverage is needed.
> > A new algo?
> 
> If you mean a mutator sub-option, I would say no, it should be separate.  Some
> separate option like -fuzz_use_cov or sthg.

I mean the drfuzz_mutator_options_t, there has to be a place to tell drfuzz use
cov.

[bruening, 2015-12-15 22:01:44]

On 2015/12/15 21:59:52, zhaoqin wrote:
> On 2015/12/15 21:46:45, bruening wrote:
> > On 2015/12/15 21:44:14, zhaoqin wrote:
> > > On 2015/12/15 21:39:51, bruening wrote:
> > > > On 2015/12/15 21:37:35, zhaoqin wrote:
> > > > > On 2015/12/15 21:08:33, bruening wrote:
> > > > > > It seems like this is a key part of many fuzzing frameworks and that
> it
> > > > should
> > > > > > be inside drfuzz, rather than in drmemory.  WDYT?
> > > > > 
> > > > > I thought about it. How do you think we should put it in drfuzz?
> > > > > You mean the drfuzz itself register the bb event and collect code
> > coverage?
> > > > 
> > > > Yes.
> > > 
> > > ok, in that case, we need add something in the options to explicitly say
> code
> > > coverage is needed.
> > > A new algo?
> > 
> > If you mean a mutator sub-option, I would say no, it should be separate. 
Some
> > separate option like -fuzz_use_cov or sthg.
> 
> I mean the drfuzz_mutator_options_t, there has to be a place to tell drfuzz
use
> cov.

drfuzz_mutator_options_t is being deleted, and this should not be part of the
mutator options anyway.

Maybe drfuzz_init should take in some flags or options.

[zhaoqin, 2015-12-15 22:03:53]

On 2015/12/15 22:01:44, bruening wrote:
> On 2015/12/15 21:59:52, zhaoqin wrote:
> > On 2015/12/15 21:46:45, bruening wrote:
> > > On 2015/12/15 21:44:14, zhaoqin wrote:
> > > > On 2015/12/15 21:39:51, bruening wrote:
> > > > > On 2015/12/15 21:37:35, zhaoqin wrote:
> > > > > > On 2015/12/15 21:08:33, bruening wrote:
> > > > > > > It seems like this is a key part of many fuzzing frameworks and
that
> > it
> > > > > should
> > > > > > > be inside drfuzz, rather than in drmemory.  WDYT?
> > > > > > 
> > > > > > I thought about it. How do you think we should put it in drfuzz?
> > > > > > You mean the drfuzz itself register the bb event and collect code
> > > coverage?
> > > > > 
> > > > > Yes.
> > > > 
> > > > ok, in that case, we need add something in the options to explicitly say
> > code
> > > > coverage is needed.
> > > > A new algo?
> > > 
> > > If you mean a mutator sub-option, I would say no, it should be separate. 
> Some
> > > separate option like -fuzz_use_cov or sthg.
> > 
> > I mean the drfuzz_mutator_options_t, there has to be a place to tell drfuzz
> use
> > cov.
> 
> drfuzz_mutator_options_t is being deleted, and this should not be part of the
> mutator options anyway.
> 
> Maybe drfuzz_init should take in some flags or options.

hmm, I feel that we may have conflicts on updating the code, should I wait till
you commit your code?

[bruening, 2015-12-15 22:59:11]

On 2015/12/15 22:03:53, zhaoqin wrote:
> On 2015/12/15 22:01:44, bruening wrote:
> > On 2015/12/15 21:59:52, zhaoqin wrote:
> > > On 2015/12/15 21:46:45, bruening wrote:
> > > > On 2015/12/15 21:44:14, zhaoqin wrote:
> > > > > On 2015/12/15 21:39:51, bruening wrote:
> > > > > > On 2015/12/15 21:37:35, zhaoqin wrote:
> > > > > > > On 2015/12/15 21:08:33, bruening wrote:
> > > > > > > > It seems like this is a key part of many fuzzing frameworks and
> that
> > > it
> > > > > > should
> > > > > > > > be inside drfuzz, rather than in drmemory.  WDYT?
> > > > > > > 
> > > > > > > I thought about it. How do you think we should put it in drfuzz?
> > > > > > > You mean the drfuzz itself register the bb event and collect code
> > > > coverage?
> > > > > > 
> > > > > > Yes.
> > > > > 
> > > > > ok, in that case, we need add something in the options to explicitly
say
> > > code
> > > > > coverage is needed.
> > > > > A new algo?
> > > > 
> > > > If you mean a mutator sub-option, I would say no, it should be separate.

> > Some
> > > > separate option like -fuzz_use_cov or sthg.
> > > 
> > > I mean the drfuzz_mutator_options_t, there has to be a place to tell
drfuzz
> > use
> > > cov.
> > 
> > drfuzz_mutator_options_t is being deleted, and this should not be part of
the
> > mutator options anyway.
> > 
> > Maybe drfuzz_init should take in some flags or options.
> 
> hmm, I feel that we may have conflicts on updating the code, should I wait
till
> you commit your code?

There will be minor conflicts, but my changes are getting larger and will not be
finished today, so either way.

[zhaoqin, 2015-12-16 19:47:17]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_algo to specify different fuzzing algorithms
- add drfuzz_mutator_feedback in drfuzz to allow client provide feedback
  to drfuzz mutator
- add bb_event in drfuzz to track the number of new basic blocks seen
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[zhaoqin, 2015-12-16 19:49:57]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_algo to specify different fuzzing algorithms
- add drfuzz_mutator_feedback in drfuzz to allow client provide feedback
  to drfuzz mutator
- add bb_event in drfuzz to track the number of basic blocks seen
- add drfuzz_get_target_num_bbs to return number of basic blocks
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[bruening, 2015-12-16 23:16:21]

On 2015/12/16 19:49:57, zhaoqin wrote:
> - add option -fuzz_algo to specify different fuzzing algorithms

I would vote for "alg" rather than "algo"

> - add drfuzz_mutator_feedback in drfuzz to allow client provide feedback

Grammar: "to"

[bruening, 2015-12-16 23:41:14]

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.c
File drfuzz/drfuzz.c (right):

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.c#newcode362
drfuzz/drfuzz.c:362: /* update num_bbs for each live targets */
grammar

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.c#newcode366
drfuzz/drfuzz.c:366: DRFUZZ_LOG(3, "%llu(th) basic block "PFX" during fuzzing
"PFX"\n",
bug: use the cross-platform format code so this will build on Windows

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode373
drfuzz/drfuzz.h:373: * Get the totoal number of basic block seen during fuzzing
\p target_pc.
spelling and multiple grammar issues

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode376
drfuzz/drfuzz.h:376: *                        number of basic blocks during
execution is returned.
bug: this should fail to build b/c doxygen will complain about a missing arg

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode376
drfuzz/drfuzz.h:376: *                        number of basic blocks during
execution is returned.
"seen" or sthg

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode378
drfuzz/drfuzz.h:378: * \note: Basic blocks might be counted multiple times due
to code cache managment.
Also talk about thread-private caches, nested targets and multiple threads,
etc.?

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode381
drfuzz/drfuzz.h:381: drfuzz_get_target_num_bbs(generic_func_t target_pc, uint64
*num_bbs);
Use the IN and OUT markers that the other functions in this header use

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz_mutator.h
File drfuzz/drfuzz_mutator.h (right):

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz_mutator.h#n...
drfuzz/drfuzz_mutator.h:200: * Provides feedback to the mutator about the effect
of the last mutation:
s/:/./

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz_mutator.h#n...
drfuzz/drfuzz_mutator.h:203: * and the higher \p feedback, the better effect of
the last mutation is.
grammar issues

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode96
drmemory/fuzzer.c:96: uint64 num_bbs;          /* number of basic block seen */
s

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode137
drmemory/fuzzer.c:137: bool bbcov;             /* use basic block coverage info
for mutation */
suggest: use_bbcov?

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode237
drmemory/fuzzer.c:237: LOG(1, LOG_PREFIX" %llu basic blocks seen during
execution.\n", num_bbs);
bug: use cross-platform format code

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode311
drmemory/fuzzer.c:311: fuzz_target.mutator_options->alg = MUTATOR_ALG_RANDOM;
I added -fuzz_mutator_alg which takes "random" and "ordered".  But isn't whether
coverage feedback is used relevant to random, but not to ordered?  So I don't
understand your "else if" here.

Would it make more sense to add coverage as a flag that can affect each
algorithm, rather than making it its own algorithm?  BTW, maybe the flags should
be named at the frontend or sthg.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode313
drmemory/fuzzer.c:313: if (strcmp(options.fuzz_algo, "bitflip") != 0)
I don't get it: many algorithms perform some kind of "bitflip".  What alg is
this?

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode...
drmemory/fuzzer.c:1043: LOG(2, "\n"LOG_PREFIX" %llu basic blocks seen during
fuzzing.\n", num_bbs);
ditto

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "    The possible calling convention codes are:"NL
This lined up with some other options, so best not to change just one: have to
change everything at this indentation level

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:661: OPTION_CLIENT_STRING(drmemscope, fuzz_algo, "bitflip",
This will need to be merged w/ what I added.  Also see prior discussion on
whether coverage feedback should be a flag that can affect multiple algorithms.

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:666: "Specify the fuzzing algirhtm."
!!

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:668: "&nbsp;&nbsp;&nbsp;&nbsp;<code>bitflip = flip through
all bits of input data in fixed order</code>\n"
This style I find really hard to read.  IMHO it is better to use my original
style of using <ul> and @@.

[zhaoqin, 2015-12-17 19:32:09]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_bbcov to enable basic block coverage guided fuzzing
- add drfuzz_mutator_feedback in drfuzz to allow client provide feedback
  to drfuzz mutator
- add bb_event in drfuzz to track the number of basic blocks seen
- add drfuzz_get_target_num_bbs to return number of basic blocks
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[zhaoqin, 2015-12-17 19:37:56]

sorry about the merge, I have to merge to make test the changes.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.c
File drfuzz/drfuzz.c (right):

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.c#newcode362
drfuzz/drfuzz.c:362: /* update num_bbs for each live targets */
On 2015/12/16 23:41:13, bruening wrote:
> grammar

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.c#newcode366
drfuzz/drfuzz.c:366: DRFUZZ_LOG(3, "%llu(th) basic block "PFX" during fuzzing
"PFX"\n",
On 2015/12/16 23:41:13, bruening wrote:
> bug: use the cross-platform format code so this will build on Windows

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode373
drfuzz/drfuzz.h:373: * Get the totoal number of basic block seen during fuzzing
\p target_pc.
On 2015/12/16 23:41:13, bruening wrote:
> spelling and multiple grammar issues

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode376
drfuzz/drfuzz.h:376: *                        number of basic blocks during
execution is returned.
On 2015/12/16 23:41:13, bruening wrote:
> "seen" or sthg

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode376
drfuzz/drfuzz.h:376: *                        number of basic blocks during
execution is returned.
On 2015/12/16 23:41:13, bruening wrote:
> bug: this should fail to build b/c doxygen will complain about a missing arg

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode378
drfuzz/drfuzz.h:378: * \note: Basic blocks might be counted multiple times due
to code cache managment.
On 2015/12/16 23:41:13, bruening wrote:
> Also talk about thread-private caches, nested targets and multiple threads,
> etc.?

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz.h#newcode381
drfuzz/drfuzz.h:381: drfuzz_get_target_num_bbs(generic_func_t target_pc, uint64
*num_bbs);
On 2015/12/16 23:41:13, bruening wrote:
> Use the IN and OUT markers that the other functions in this header use

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz_mutator.h
File drfuzz/drfuzz_mutator.h (right):

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz_mutator.h#n...
drfuzz/drfuzz_mutator.h:200: * Provides feedback to the mutator about the effect
of the last mutation:
On 2015/12/16 23:41:13, bruening wrote:
> s/:/./

Done.

https://codereview.appspot.com/281880043/diff/80001/drfuzz/drfuzz_mutator.h#n...
drfuzz/drfuzz_mutator.h:203: * and the higher \p feedback, the better effect of
the last mutation is.
On 2015/12/16 23:41:13, bruening wrote:
> grammar issues

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode96
drmemory/fuzzer.c:96: uint64 num_bbs;          /* number of basic block seen */
On 2015/12/16 23:41:13, bruening wrote:
> s

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode137
drmemory/fuzzer.c:137: bool bbcov;             /* use basic block coverage info
for mutation */
On 2015/12/16 23:41:13, bruening wrote:
> suggest: use_bbcov?

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode237
drmemory/fuzzer.c:237: LOG(1, LOG_PREFIX" %llu basic blocks seen during
execution.\n", num_bbs);
On 2015/12/16 23:41:13, bruening wrote:
> bug: use cross-platform format code

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode311
drmemory/fuzzer.c:311: fuzz_target.mutator_options->alg = MUTATOR_ALG_RANDOM;
On 2015/12/16 23:41:13, bruening wrote:
> I added -fuzz_mutator_alg which takes "random" and "ordered".  But isn't
whether
> coverage feedback is used relevant to random, but not to ordered?  So I don't
> understand your "else if" here.
> 
> Would it make more sense to add coverage as a flag that can affect each
> algorithm, rather than making it its own algorithm?  BTW, maybe the flags
should
> be named at the frontend or sthg.

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode313
drmemory/fuzzer.c:313: if (strcmp(options.fuzz_algo, "bitflip") != 0)
On 2015/12/16 23:41:13, bruening wrote:
> I don't get it: many algorithms perform some kind of "bitflip".  What alg is
> this?

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/fuzzer.c#newcode...
drmemory/fuzzer.c:1043: LOG(2, "\n"LOG_PREFIX" %llu basic blocks seen during
fuzzing.\n", num_bbs);
On 2015/12/16 23:41:13, bruening wrote:
> ditto

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "    The possible calling convention codes are:"NL
On 2015/12/16 23:41:14, bruening wrote:
> This lined up with some other options, so best not to change just one: have to
> change everything at this indentation level

Done.

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:661: OPTION_CLIENT_STRING(drmemscope, fuzz_algo, "bitflip",
On 2015/12/16 23:41:14, bruening wrote:
> This will need to be merged w/ what I added.  Also see prior discussion on
> whether coverage feedback should be a flag that can affect multiple
algorithms.

change it to a flag

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:666: "Specify the fuzzing algirhtm."
On 2015/12/16 23:41:14, bruening wrote:
> !!

removed

https://codereview.appspot.com/281880043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:668: "&nbsp;&nbsp;&nbsp;&nbsp;<code>bitflip = flip through
all bits of input data in fixed order</code>\n"
On 2015/12/16 23:41:14, bruening wrote:
> This style I find really hard to read.  IMHO it is better to use my original
> style of using <ul> and @@.

removed

[bruening, 2015-12-17 19:44:12]

On 2015/12/17 19:32:09, zhaoqin wrote:
> - add drfuzz_mutator_feedback in drfuzz to allow client provide feedback

to

[zhaoqin, 2015-12-17 19:46:35]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_bbcov to enable basic block coverage guided fuzzing
- add drfuzz_mutator_feedback to drfuzz to allow client provide feedback
  to drfuzz mutator
- add bb_event in drfuzz to track the number of basic blocks seen
- add drfuzz_get_target_num_bbs to return number of basic blocks
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[bruening, 2015-12-17 19:46:37]

https://codereview.appspot.com/281880043/diff/100001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/281880043/diff/100001/drfuzz/drfuzz.h#newcode380
drfuzz/drfuzz.h:380: * e.g., code cache management, thread shared code cache for
multi-threaded applicaitons,
Please look through the other comments for errors like this (esp in user-exposed
docs)

[zhaoqin, 2015-12-17 19:55:54]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add code coverage based fuzzing framework

- add option -fuzz_bbcov to enable basic block coverage guided fuzzing
- add drfuzz_mutator_feedback to drfuzz to allow client provide feedback
  to drfuzz mutator
- add bb_event in drfuzz to track the number of basic blocks seen
- add drfuzz_get_target_num_bbs to return number of basic blocks
- provide feedback to drfuzz in post_fuzz

Review-URL: https://codereview.appspot.com/281880043
---------------

[bruening, 2015-12-18 18:08:39]

I would make it more clear in the commit msg that the coverage feedback doesn't
do anything yet.

LGTM w/ comments

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz.h#newcode373
drfuzz/drfuzz.h:373: * Get the total number of basic blocks seen during \p
target_pc fuzzing.
Please clarify that this is NEW blocks, not, say, the total # of blocks executed
regardless of when created

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz.h#newcode381
drfuzz/drfuzz.h:381: * applications, or nested target functions.
I would elaborate here b/c this is not obvious why: the multi-thread problem is
that if one thread is in the target func, any new bb in any other thread is
counted as a new fuzz bb, right?  IMHO this should be explained.  I also think
"accurate" should be qualified: you mean, the precise number of new blocks that
are a direct result of the target function's execution.

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz_mutator.h
File drfuzz/drfuzz_mutator.h (right):

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz_mutator.h#...
drfuzz/drfuzz_mutator.h:142: * If the meaning of \p feedback is not specified, 0
means neutral;
if you're going to use "and" you should "," instead of ";"

https://codereview.appspot.com/281880043/diff/140001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281880043/diff/140001/drmemory/fuzzer.c#newcod...
drmemory/fuzzer.c:1103: LOG(2, "\n"LOG_PREFIX" "UINT64_FORMAT_STRING" basic
blocks seen during fuzzing.\n",
Why start with a newline?

https://codereview.appspot.com/281880043/diff/140001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281880043/diff/140001/drmemory/optionsx.h#newc...
drmemory/optionsx.h:663: "Enable basic block coverage guided fuzzing.")
One would expect something longer that elaborates for the html docs -- though
the coverage is not really implemented as the feedback routine does nothing yet

[zhaoqin, 2015-12-18 21:05:51]

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz.h#newcode373
drfuzz/drfuzz.h:373: * Get the total number of basic blocks seen during \p
target_pc fuzzing.
On 2015/12/18 18:08:39, bruening wrote:
> Please clarify that this is NEW blocks, not, say, the total # of blocks
executed
> regardless of when created

If target_pc is NULL, it is total # of blocks regardless of when created.
If target_pc is not NULL, it is the total # of basic block after the first
target_pc is first executed.

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz.h#newcode381
drfuzz/drfuzz.h:381: * applications, or nested target functions.
On 2015/12/18 18:08:39, bruening wrote:
> I would elaborate here b/c this is not obvious why: the multi-thread problem
is
> that if one thread is in the target func, any new bb in any other thread is
> counted as a new fuzz bb, right?  IMHO this should be explained.  I also think
> "accurate" should be qualified: you mean, the precise number of new blocks
that
> are a direct result of the target function's execution.

There are many cases:
1. for code management, a basic block might be counted multiple times due to
reset, eviction, ...
2. for multi-threaded: in fact, we won't count bb for other thread, because we
use the per-thread live target structure to update num_bbs, so only a bb in
fuzzing thread will be counted. However, we may miss some basic blocks if a
non-fuzzing thread (e.g., slave thread) execute it.
3. nested target is even messier, the num_bbs in the out fuzzing loop is not the
result of its mutation, but probably result of its inner fuzzing loop. Should it
be really counted as the out fuzz loop?
Hmm, when reading this, I feel it makes more sense to not count the inner loop
for outer loop, change the code to do so.

Those are the cases that I can think of, will list them all.

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz_mutator.h
File drfuzz/drfuzz_mutator.h (right):

https://codereview.appspot.com/281880043/diff/140001/drfuzz/drfuzz_mutator.h#...
drfuzz/drfuzz_mutator.h:142: * If the meaning of \p feedback is not specified, 0
means neutral;
On 2015/12/18 18:08:39, bruening wrote:
> if you're going to use "and" you should "," instead of ";"

Done.

https://codereview.appspot.com/281880043/diff/140001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281880043/diff/140001/drmemory/fuzzer.c#newcod...
drmemory/fuzzer.c:1103: LOG(2, "\n"LOG_PREFIX" "UINT64_FORMAT_STRING" basic
blocks seen during fuzzing.\n",
On 2015/12/18 18:08:39, bruening wrote:
> Why start with a newline?

copied from line 1085.
removed

https://codereview.appspot.com/281880043/diff/140001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281880043/diff/140001/drmemory/optionsx.h#newc...
drmemory/optionsx.h:663: "Enable basic block coverage guided fuzzing.")
On 2015/12/18 18:08:39, bruening wrote:
> One would expect something longer that elaborates for the html docs -- though
> the coverage is not really implemented as the feedback routine does nothing
yet

add Not yet implemented.

[zhaoqin, 2015-12-18 21:10:14]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add feedback based fuzzing framework

- add option -fuzz_bbcov to enable basic block coverage guided fuzzing
- add drfuzz_mutator_feedback to drfuzz to allow client provide feedback
  to drfuzz mutator
- add bb_event in drfuzz to track the number of basic blocks seen
- add drfuzz_get_target_num_bbs to return number of basic blocks
- provide feedback to drfuzz in post_fuzz

TODO: add code in the mutator to act on the feedback

Review-URL: https://codereview.appspot.com/281880043
---------------