i#1734 drfuzz: refact pre_fuzz, post_fuzz

Commit log for first patchset:
---------------
i#1734 drfuzz: refact pre_fuzz, post_fuzz

- extract shadow state save/restore code from pre/post_fuzz into
  shadow_state_* routines
- extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines
- move shadow_state_restore from post_fuzz to pre_fuzz
---------------

[bruening, 2015-11-20 21:51:40]

On 2015/11/20 21:43:48, zhaoqin wrote:
> i#1734 drfuzz: refact pre_fuzz, post_fuzz

"refact"?

[zhaoqin, 2015-11-20 23:09:10]

Commit log for latest patchset:
---------------
i#1734 drfuzz: refactor pre_fuzz, post_fuzz

- extract shadow state save/restore code from pre/post_fuzz into
  shadow_state_* routines
- extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines
- move shadow_state_restore from post_fuzz to pre_fuzz

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

[bruening, 2015-11-23 15:31:13]

On 2015/11/20 23:09:10, zhaoqin wrote:
> Commit log for latest patchset:
> ---------------
> i#1734 drfuzz: refactor pre_fuzz, post_fuzz
> 
> - extract shadow state save/restore code from pre/post_fuzz into
>   shadow_state_* routines
> - extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines
> - move shadow_state_restore from post_fuzz to pre_fuzz

This says what, but I would like to know why: what is gained by these code
movements?  Is this preparation for some split into separate files?  Why is
shadow_state_restore better in pre than in  post -- won't it need an extra check
in pre?  Intuition says that post is a more natural place for it.

[zhaoqin, 2015-11-23 15:58:32]

On 2015/11/23 15:31:13, bruening wrote:
> On 2015/11/20 23:09:10, zhaoqin wrote:
> > Commit log for latest patchset:
> > ---------------
> > i#1734 drfuzz: refactor pre_fuzz, post_fuzz
> > 
> > - extract shadow state save/restore code from pre/post_fuzz into
> >   shadow_state_* routines
> > - extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines
> > - move shadow_state_restore from post_fuzz to pre_fuzz
> 
> This says what, but I would like to know why: what is gained by these code
> movements?  Is this preparation for some split into separate files?  Why is
> shadow_state_restore better in pre than in  post -- won't it need an extra
check
> in pre?  Intuition says that post is a more natural place for it.

currently, the sahdow state restore is split, the args reg/stack is in post, and
input buffer restore is in pre. I think it makes more sense to put them together
(i.e., put into shadow_state_restore) and call shadow_state_restore either in
pre or post. I prefer pre as it makes more sense to setup the state right before
execution and at the same place where the state save happens.
We can do it at post, the downside is the stack pointer is at different place in
post-func from stack pointer in pre-func, so need more adjustment.

I am not sure what extra check in pre you mean.

[bruening, 2015-11-23 19:49:30]

On 2015/11/23 15:58:32, zhaoqin wrote:
> On 2015/11/23 15:31:13, bruening wrote:
> > On 2015/11/20 23:09:10, zhaoqin wrote:
> > > Commit log for latest patchset:
> > > ---------------
> > > i#1734 drfuzz: refactor pre_fuzz, post_fuzz
> > > 
> > > - extract shadow state save/restore code from pre/post_fuzz into
> > >   shadow_state_* routines
> > > - extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines
> > > - move shadow_state_restore from post_fuzz to pre_fuzz
> > 
> > This says what, but I would like to know why: what is gained by these code
> > movements?  Is this preparation for some split into separate files?  Why is
> > shadow_state_restore better in pre than in  post -- won't it need an extra
> check
> > in pre?  Intuition says that post is a more natural place for it.
> 
> currently, the sahdow state restore is split, the args reg/stack is in post,
and
> input buffer restore is in pre. I think it makes more sense to put them
together
> (i.e., put into shadow_state_restore) and call shadow_state_restore either in
> pre or post. I prefer pre as it makes more sense to setup the state right
before
> execution and at the same place where the state save happens.
> We can do it at post, the downside is the stack pointer is at different place
in
> post-func from stack pointer in pre-func, so need more adjustment.

Could a short version of this be in the commit msg?

> I am not sure what extra check in pre you mean.

You do not want to restore on the first iteration, so you have to check for
that, whereas in post there is no check, making post seem like a more natural
place in that sense.

[bruening, 2015-11-23 20:29:36]

LGTM w/ comments

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c
File drmemory/fuzzer.c (left):

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#oldcode990
drmemory/fuzzer.c:990: shadow_state_restore_stack_frame(&mc, shadow);
I don't quite get how this worked before: mc is only used for the retaddr
restore.  However, this assumes that the retaddr slot is just beyond TOS.  That
won't be true for stdcall, right?  So wasn't it restoring the wrong slot before?
 So why didn't you get a false pos on the OP_ret for each subsequent iter?

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode649
drmemory/fuzzer.c:649: stack_ret_size + stack_arg_size);
This isn't listed in the commit msg, which is not ideal, as normally a CL
labeled "refactor" does not change semantics, while here the behavior is
different for Win64.

The change looks good, but not for the reason the comments say: saving+restoring
the 4 Win64 stack slots is important, and before they were not preserved at all,
right?

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode661
drmemory/fuzzer.c:661: shadow->reg_args[i]);
style violation: need {} for multi-line body

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode743
drmemory/fuzzer.c:743: /* we only need save shadow state for uninit check */
"need to"

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode756
drmemory/fuzzer.c:756: /* We only need save shadow state for arguments and input
buffers */
nit: why "We only" here vs "we only" above?  inconsistent.

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode756
drmemory/fuzzer.c:756: /* We only need save shadow state for arguments and input
buffers */
"need to"

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode768
drmemory/fuzzer.c:768: /* we only need save shadow state for uninit check */
ditto: "need to", and "we" vs "We" below

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode784
drmemory/fuzzer.c:784: /* We do not reset redzone because the shadow state would
not be changed. */
nit: "We... ." vs no cap and no period above for very similar kind of comment

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode784
drmemory/fuzzer.c:784: /* We do not reset redzone because the shadow state would
not be changed. */
"the redzone"

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode784
drmemory/fuzzer.c:784: /* We do not reset redzone because the shadow state would
not be changed. */
s/would/should/

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode793
drmemory/fuzzer.c:793: /* we only need save shadow state for uninit check */
ditto, ditto

https://codereview.appspot.com/272590043/diff/20001/drmemory/fuzzer.c#newcode976
drmemory/fuzzer.c:976: shadow_state_restore(dcontext, fuzzcxt, mc);
Wasteful to restore on the 1st iteration, esp if the buffer is large!  This is
one reason why this makes more sense in the post-fuzz callback.

[zhaoqin, 2015-11-23 20:41:31]

On 2015/11/23 19:49:30, bruening wrote:
> On 2015/11/23 15:58:32, zhaoqin wrote:
> > On 2015/11/23 15:31:13, bruening wrote:
> > > On 2015/11/20 23:09:10, zhaoqin wrote:
> > > > Commit log for latest patchset:
> > > > ---------------
> > > > i#1734 drfuzz: refactor pre_fuzz, post_fuzz
> > > > 
> > > > - extract shadow state save/restore code from pre/post_fuzz into
> > > >   shadow_state_* routines
> > > > - extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines
> > > > - move shadow_state_restore from post_fuzz to pre_fuzz
> > > 
> > > This says what, but I would like to know why: what is gained by these code
> > > movements?  Is this preparation for some split into separate files?  Why
is
> > > shadow_state_restore better in pre than in  post -- won't it need an extra
> > check
> > > in pre?  Intuition says that post is a more natural place for it.
> > 
> > currently, the sahdow state restore is split, the args reg/stack is in post,
> and
> > input buffer restore is in pre. I think it makes more sense to put them
> together
> > (i.e., put into shadow_state_restore) and call shadow_state_restore either
in
> > pre or post. I prefer pre as it makes more sense to setup the state right
> before
> > execution and at the same place where the state save happens.
> > We can do it at post, the downside is the stack pointer is at different
place
> in
> > post-func from stack pointer in pre-func, so need more adjustment.
> 
> Could a short version of this be in the commit msg?
> 
> > I am not sure what extra check in pre you mean.
> 
> You do not want to restore on the first iteration, so you have to check for
> that, whereas in post there is no check, making post seem like a more natural
> place in that sense.

There will always be an check if this is the first iteration.
For the first iteration, I can simply return after the state save. Currently I
did not return there in order to match old behavior, i.e., mutate on the first
iteration.
I am thinking about changing that to execute the default input first then start
mutation, seems makes more sense to me.

Also extra restore right after save without check seems ok, just waste sometime
on the first iteration, but simplify the code.

[zhaoqin, 2015-11-24 22:19:08]

Committed as
https://github.com/DynamoRIO/drmemory/commit/63581ad7bbf88be75045d40e4e6dfe0b...

Final commit log: 
---------------
i#1734 drfuzz: refactor pre_fuzz, post_fuzz

- remove shadow_config_t
- update shadow_state_save_stack_frame to save both arg and return slots
- update shadow_state_restore_stack_frame to restore both arg and return slots
- extract shadow state save code from pre_fuzz into shadow_state_init
- extract/merge shadow state restore code from pre_fuzz and post_fuzz
  into shadow_state_restore
- call shadow_state_restore from pre_fuzz only
- extract shadow state cleanup code from post_fuzz into shadow_state_exit
- extract mutator code from pre/post_fuzz into fuzzer_mutator_* routines

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