Combine traits for IPC mutation and generation fuzzing plus other refactoring.

Combine traits for IPC mutation and generation fuzzing plus other refactoring.

R=inferno@chromium.org,tsepez@chromium.org
BUG=450268
Committed: https://crrev.com/870c84a3e72ac9b3bdcd9a2512a063b366f1d55b
Cr-Commit-Position: refs/heads/master@{#321369}

[inferno, 2015-03-16 18:56:35]

lgtm with nits.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/fuzzer.cc (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:25: // First include of message files to
provide basic type.
s/of/all
s/type/types

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:27: #include "ipc/ipc_message_null_macros.h"
newline before this, with comment
https://code.google.com/p/chromium/codesearch#chromium/src/tools/ipc_fuzzer/m...

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:50: bool Fuzzer::ShouldGenerate() {
Why is this defined here ? shouldn't this only be in generate.cc and mutate.cc

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:1668: // TODO(mbarbella): Support mutation.
If we can't mutate [for todos], should be not generate ? maybe we generate with
a frequency.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:1856: // TODO(mbarbella): Find a way to handle
the size mutation properly.
TODO is fine, but we shouldn't just bailout, it can continue and subsitute with
a new one.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/fuzzer.h (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.h:8: #include <set>
this looks unused ?

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.h:71: typedef std::vector<FuzzerFunction>
FuzzerFunctionVector;
maybe club 71,72 with 74,75. also add comment explaining the purpose of these.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.h:77: extern FuzzerFunctionVector
g_function_vector;
comment on use of this.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/mutator.cc (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.cc:222: int frequency = 23;
this 23 should a global. why 23 ? 20 as a percent looks good.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.cc:250: if (!type_set.empty() && type_set.end()
== std::find(
Add a comment.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.cc:256: delete message_vector[i];
Wont this shift the vector, you might wanna assign to a local then subsitute and
then delete local.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/mutator.h (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.h:16: explicit Mutator(int frequency) :
frequency_(frequency) {}
match the return type of frequency.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.h:38: unsigned int frequency_;
don't need this large frequency, just have frequency as a percent. maybe keep it
unsigned int, but leave comment on range. 0-100.

[Martin Barbella, 2015-03-16 19:19:56]

Thanks. I'll take a look a closer look at these and upload another patch set
soon. It looks like the mutator is broken on Windows at the moment as well, so
I'm looking into that first.

https://codereview.chromium.org/1000373004/diff/20001/tools/ipc_fuzzer/dump/d...
File tools/ipc_fuzzer/dump/dump.gyp (right):

https://codereview.chromium.org/1000373004/diff/20001/tools/ipc_fuzzer/dump/d...
tools/ipc_fuzzer/dump/dump.gyp:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
Rename to message_dump.

https://codereview.chromium.org/1000373004/diff/20001/tools/ipc_fuzzer/dump/d...
tools/ipc_fuzzer/dump/dump.gyp:24: 'target_name': 'ipc_message_util',
Move to message_util dir, new CL.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/fuzzer.cc (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:50: bool Fuzzer::ShouldGenerate() {
On 2015/03/16 18:56:34, inferno wrote:
> Why is this defined here ? shouldn't this only be in generate.cc and mutate.cc

It shouldn't only be in those places, but maybe it shouldn't be in Fuzzer. This
is intended to return false by default unless we don't override it (needed for
NoOpFuzzer at the moment).

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:1668: // TODO(mbarbella): Support mutation.
On 2015/03/16 18:56:34, inferno wrote:
> If we can't mutate [for todos], should be not generate ? maybe we generate
with
> a frequency.

I'm planning to handle this by updating the mutator to sometimes return true in
this case. I still think it would be best to do nothing by default in cases that
we don't know how to handle.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:1856: // TODO(mbarbella): Find a way to handle
the size mutation properly.
On 2015/03/16 18:56:34, inferno wrote:
> TODO is fine, but we shouldn't just bailout, it can continue and subsitute
with
> a new one.

The complication is that we still need to make sure that the size is very small
after mutation. It blows up pretty quickly. Worse case I guess we could just
have a hard cap on each dimension, but since that didn't quite seem right that's
why I added the TODO.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/mutator.cc (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.cc:222: int frequency = 23;
On 2015/03/16 18:56:35, inferno wrote:
> this 23 should a global. why 23 ? 20 as a percent looks good.

This is almost an exact copy of the old MutateMain, so not sure why the number
was chosen. Same for other comments. I think we need some cleanup here, but I'd
rather handle this kind of change (along with other tweaks to how we handle
frequencies) in another CL.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.cc:250: if (!type_set.empty() && type_set.end()
== std::find(
On 2015/03/16 18:56:35, inferno wrote:
> Add a comment.

Since I changed the macros, that's a good idea for this CL.

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/mutator.cc:256: delete message_vector[i];
On 2015/03/16 18:56:35, inferno wrote:
> Wont this shift the vector, you might wanna assign to a local then subsitute
and
> then delete local.

Good point. This is a copy, but I think it's worth addressing it now.

[Martin Barbella, 2015-03-18 17:59:06]

Addressed most of the comments but replied to a few others. Could you take
another quick look to see if anything that wasn't addressed should be included
in this CL?

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
File tools/ipc_fuzzer/mutate/fuzzer.cc (right):

https://codereview.chromium.org/1000373004/diff/60001/tools/ipc_fuzzer/mutate...
tools/ipc_fuzzer/mutate/fuzzer.cc:27: #include "ipc/ipc_message_null_macros.h"
On 2015/03/16 18:56:34, inferno wrote:
> newline before this, with comment
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/ipc_fuzzer/m...

That's actually much later (line 2037 on this patch set). We aren't redefining
them yet for the generation of the vector/map. At this point we just want to
have the type information.

[Martin Barbella, 2015-03-18 18:02:14]

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

[Martin Barbella, 2015-03-18 18:04:12]

Tom, could you please take a look at this as well?

[Tom Sepez, 2015-03-18 18:34:12]

On 2015/03/18 18:04:12, mbarbella wrote:
> Tom, could you please take a look at this as well?
While you're at it, I've always disliked having all these subdirectories since
there are only about 8 files total, maybe you want to flatten the directory
structure so long as things are getting renamed. I'm taking a look at the
specifics now.

[Tom Sepez, 2015-03-18 18:35:16]

On 2015/03/18 18:34:12, Tom Sepez wrote:
> On 2015/03/18 18:04:12, mbarbella wrote:
> > Tom, could you please take a look at this as well?
> While you're at it, I've always disliked having all these subdirectories since
> there are only about 8 files total, maybe you want to flatten the directory
> structure so long as things are getting renamed. I'm taking a look at the
> specifics now.
Or rename "mutate" to fuzzer.  Or just leave it for another day.

[Martin Barbella, 2015-03-18 18:38:00]

On 2015/03/18 18:35:16, Tom Sepez wrote:
> On 2015/03/18 18:34:12, Tom Sepez wrote:
> > On 2015/03/18 18:04:12, mbarbella wrote:
> > > Tom, could you please take a look at this as well?
> > While you're at it, I've always disliked having all these subdirectories
since
> > there are only about 8 files total, maybe you want to flatten the directory
> > structure so long as things are getting renamed. I'm taking a look at the
> > specifics now.
> Or rename "mutate" to fuzzer.  Or just leave it for another day.

Yeah, something like that sounds good. I'll probably think about it a bit more
and handle it in another CL (initially had it here, but it made things a bit
more confusing to review). If you have any preferences for how things should be
moved around, let me know.

[Tom Sepez, 2015-03-18 18:49:19]

https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/ipc_f...
File tools/ipc_fuzzer/ipc_fuzzer.gyp (right):

https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/ipc_f...
tools/ipc_fuzzer/ipc_fuzzer.gyp:16: 'mutate/mutate.gyp:ipc_fuzzer_generate',
It seems a pity to have two utilities instead of just having a cmd line flag to
pick the fuzzer to use, ignoring the input file cmd line argument in the
generative case.  Then you wouldn't need a library and it all links together. 
Or am I missing the point of having a separate library?

LGTM otherwise.

https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/mutat...
File tools/ipc_fuzzer/mutate/fuzzer.h (right):

https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/mutat...
tools/ipc_fuzzer/mutate/fuzzer.h:14: #include
"tools/ipc_fuzzer/message_lib/message_file.h"
Nit: does message_file.h need to be in this header, or can it move to a .cc
file?

[Martin Barbella, 2015-03-18 19:00:20]

On 2015/03/18 18:49:19, Tom Sepez wrote:
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/ipc_f...
> File tools/ipc_fuzzer/ipc_fuzzer.gyp (right):
> 
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/ipc_f...
> tools/ipc_fuzzer/ipc_fuzzer.gyp:16: 'mutate/mutate.gyp:ipc_fuzzer_generate',
> It seems a pity to have two utilities instead of just having a cmd line flag
to
> pick the fuzzer to use, ignoring the input file cmd line argument in the
> generative case.  Then you wouldn't need a library and it all links together. 
> Or am I missing the point of having a separate library?

That seems like a good idea. It was easier to split things up this way, but it
would be better to try to find a way to combine them.

> 
> LGTM otherwise.
> 
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/mutat...
> File tools/ipc_fuzzer/mutate/fuzzer.h (right):
> 
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/mutat...
> tools/ipc_fuzzer/mutate/fuzzer.h:14: #include
> "tools/ipc_fuzzer/message_lib/message_file.h"
> Nit: does message_file.h need to be in this header, or can it move to a .cc
> file?

Done.

[Martin Barbella, 2015-03-18 19:04:51]

On 2015/03/18 18:49:19, Tom Sepez wrote:
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/ipc_f...
> File tools/ipc_fuzzer/ipc_fuzzer.gyp (right):
> 
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/ipc_f...
> tools/ipc_fuzzer/ipc_fuzzer.gyp:16: 'mutate/mutate.gyp:ipc_fuzzer_generate',
> It seems a pity to have two utilities instead of just having a cmd line flag
to
> pick the fuzzer to use, ignoring the input file cmd line argument in the
> generative case.  Then you wouldn't need a library and it all links together. 
> Or am I missing the point of having a separate library?

That seems like a good idea. It was easier to split things up this way, but it
would be better to try to find a way to combine them.

> 
> LGTM otherwise.
> 
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/mutat...
> File tools/ipc_fuzzer/mutate/fuzzer.h (right):
> 
>
https://codereview.chromium.org/1000373004/diff/120001/tools/ipc_fuzzer/mutat...
> tools/ipc_fuzzer/mutate/fuzzer.h:14: #include
> "tools/ipc_fuzzer/message_lib/message_file.h"
> Nit: does message_file.h need to be in this header, or can it move to a .cc
> file?

Done.

[inferno, 2015-03-19 16:00:13]

lgtm

https://codereview.chromium.org/1000373004/diff/180001/tools/ipc_fuzzer/mutat...
File tools/ipc_fuzzer/mutate/fuzzer_main.cc (right):

https://codereview.chromium.org/1000373004/diff/180001/tools/ipc_fuzzer/mutat...
tools/ipc_fuzzer/mutate/fuzzer_main.cc:219: int frequency = 23;
Please add a FIXME for this hardcoded value. Move to global.

https://codereview.chromium.org/1000373004/diff/180001/tools/ipc_fuzzer/mutat...
File tools/ipc_fuzzer/mutate/ipc_fuzzer_gen.py (right):

https://codereview.chromium.org/1000373004/diff/180001/tools/ipc_fuzzer/mutat...
tools/ipc_fuzzer/mutate/ipc_fuzzer_gen.py:44: FUZZER_NAME_OPTION,
should be the first argument, probably on line 43.

[Martin Barbella, 2015-03-19 16:04:48]

The CQ bit was checked by mbarbella@chromium.org

[Martin Barbella, 2015-03-19 16:04:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/1000373004/#ps200001
(title: "Define frequency as a constant and reorder arguments in
ipc_fuzzer_gen.py")

[commit-bot: I haz the power, 2015-03-19 16:05:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1000373004/200001

[commit-bot: I haz the power, 2015-03-19 16:27:00]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-19 16:27:47]

Patchset 11 (id:??) landed as
https://crrev.com/870c84a3e72ac9b3bdcd9a2512a063b366f1d55b
Cr-Commit-Position: refs/heads/master@{#321369}