fixes fuzz_buffer.uninitialized failure on bots

Commit log for first patchset:
---------------
fixes fuzz_buffer.uninitialized failure on bots

- update repeatme to avoid possible multiple uninit errors on the buffer
  value check code
---------------

[bruening, 2016-01-06 05:18:36]

I don't understand: the diff doesn't seem to match the commit description, and I
don't follow the diff -- was there some mistake on upload?

[zhaoqin, 2016-01-06 16:31:44]

On 2016/01/06 05:18:36, bruening wrote:
> I don't understand: the diff doesn't seem to match the commit description, and
I
> don't follow the diff -- was there some mistake on upload?

I looks right to me.
The cause of the test failure on bot is the instruction 'test eax, ecx' for 'if
((buffer[i] & (buffer[i] - 1)) != 0)', where both eax and ecx are uninit value
orig from buffer[i]. So I change the code to only check buffer[i]'s value
instead of both buffer[i] and buffer[i]-1.

[bruening, 2016-01-06 16:55:18]

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
"have at most one bit set" and "should not be -1" are not the same thing, which
is why I do not understand this change.

The commit message says "update repeatme to avoid possible multiple uninit
errors on the buffer value check code": what is "repeatme"?  I see changes to 
buffer[i] checks -- ok UPDATE: "repeatme" seems to be the name of this function.

[zhaoqin, 2016-01-06 16:57:12]

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
On 2016/01/06 16:55:18, bruening wrote:
> "have at most one bit set" and "should not be -1" are not the same thing,
which
> is why I do not understand this change.

I am not sure if there is a better way, to check the mutate value, so just relax
the check.

> 
> The commit message says "update repeatme to avoid possible multiple uninit
> errors on the buffer value check code": what is "repeatme"?  I see changes to 
> buffer[i] checks -- ok UPDATE: "repeatme" seems to be the name of this
function.

Yes, it is the function name.

[bruening, 2016-01-06 17:10:54]

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
On 2016/01/06 16:57:12, zhaoqin wrote:
> On 2016/01/06 16:55:18, bruening wrote:
> > "have at most one bit set" and "should not be -1" are not the same thing,
> which
> > is why I do not understand this change.
> 
> I am not sure if there is a better way, to check the mutate value, so just
relax
> the check.

But your CL here implies that this is a correct semantics-preserving change!  If
we really want to check for at most one bit set, but it's too flaky to do so and
we instead check something less strict, the comment needs to say that.

[bruening, 2016-01-06 17:16:53]

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
On 2016/01/06 17:10:54, bruening wrote:
> On 2016/01/06 16:57:12, zhaoqin wrote:
> > On 2016/01/06 16:55:18, bruening wrote:
> > > "have at most one bit set" and "should not be -1" are not the same thing,
> > which
> > > is why I do not understand this change.
> > 
> > I am not sure if there is a better way, to check the mutate value, so just
> relax
> > the check.
> 
> But your CL here implies that this is a correct semantics-preserving change! 
If
> we really want to check for at most one bit set, but it's too flaky to do so
and
> we instead check something less strict, the comment needs to say that.

If we use a local var "uint val = buffer[i] & (buffer[i] - 1)" (xref
IS_POWER_OF_2 from utils.h -- maybe we can't include that header in a test?) and
then have "if (val != 0)" will that make it compiler-variation-proof?

[zhaoqin, 2016-01-07 05:15:12]

Commit log for latest patchset:
---------------
fixes fuzz_buffer.uninitialized failure on bots

- update repeatme to avoid possible multiple uninit errors on the buffer
  value check code

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

[zhaoqin, 2016-01-07 05:15:34]

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
On 2016/01/06 17:16:52, bruening wrote:
> On 2016/01/06 17:10:54, bruening wrote:
> > On 2016/01/06 16:57:12, zhaoqin wrote:
> > > On 2016/01/06 16:55:18, bruening wrote:
> > > > "have at most one bit set" and "should not be -1" are not the same
thing,
> > > which
> > > > is why I do not understand this change.
> > > 
> > > I am not sure if there is a better way, to check the mutate value, so just
> > relax
> > > the check.
> > 
> > But your CL here implies that this is a correct semantics-preserving change!

> If
> > we really want to check for at most one bit set, but it's too flaky to do so
> and
> > we instead check something less strict, the comment needs to say that.
> 
> If we use a local var "uint val = buffer[i] & (buffer[i] - 1)" (xref
> IS_POWER_OF_2 from utils.h -- maybe we can't include that header in a test?)
and
> then have "if (val != 0)" will that make it compiler-variation-proof?

Not really, the end result is:
00e61041 8b4508           mov     eax,[ebp+0x8]
00e61044 8b0cb0           mov     ecx,[eax+esi*4]
00e61047 8d41ff           lea     eax,[ecx-0x1]
00e6104a 23c1             and     eax,ecx
00e6104c 740b             jz      fuzz_buffer!repeatme+0x39 (00e61059)

Still both src opnds are uninit.

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
On 2016/01/06 17:10:54, bruening wrote:
> On 2016/01/06 16:57:12, zhaoqin wrote:
> > On 2016/01/06 16:55:18, bruening wrote:
> > > "have at most one bit set" and "should not be -1" are not the same thing,
> > which
> > > is why I do not understand this change.
> > 
> > I am not sure if there is a better way, to check the mutate value, so just
> relax
> > the check.
> 
> But your CL here implies that this is a correct semantics-preserving change! 
If
> we really want to check for at most one bit set, but it's too flaky to do so
and
> we instead check something less strict, the comment needs to say that.

Done.

[bruening, 2016-01-07 05:30:37]

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/1/tests/fuzz/fuzz_buffer.c#newc...
tests/fuzz/fuzz_buffer.c:74: /* mutated buffer value should not be 0xffffffff */
On 2016/01/07 05:15:34, zhaoqin wrote:
> On 2016/01/06 17:16:52, bruening wrote:
> > On 2016/01/06 17:10:54, bruening wrote:
> > > On 2016/01/06 16:57:12, zhaoqin wrote:
> > > > On 2016/01/06 16:55:18, bruening wrote:
> > > > > "have at most one bit set" and "should not be -1" are not the same
> thing,
> > > > which
> > > > > is why I do not understand this change.
> > > > 
> > > > I am not sure if there is a better way, to check the mutate value, so
just
> > > relax
> > > > the check.
> > > 
> > > But your CL here implies that this is a correct semantics-preserving
change!
> 
> > If
> > > we really want to check for at most one bit set, but it's too flaky to do
so
> > and
> > > we instead check something less strict, the comment needs to say that.
> > 
> > If we use a local var "uint val = buffer[i] & (buffer[i] - 1)" (xref
> > IS_POWER_OF_2 from utils.h -- maybe we can't include that header in a test?)
> and
> > then have "if (val != 0)" will that make it compiler-variation-proof?
> 
> Not really, the end result is:
> 00e61041 8b4508           mov     eax,[ebp+0x8]
> 00e61044 8b0cb0           mov     ecx,[eax+esi*4]
> 00e61047 8d41ff           lea     eax,[ecx-0x1]
> 00e6104a 23c1             and     eax,ecx
> 00e6104c 740b             jz      fuzz_buffer!repeatme+0x39 (00e61059)
> 
> Still both src opnds are uninit.

This looks like it works, because the OP_and will not raise an error: only the
OP_jz on the uninit flags.

[zhaoqin, 2016-01-07 15:56:19]

Commit log for latest patchset:
---------------
fixes fuzz_buffer.uninitialized failure on bots

- update repeatme to avoid possible multiple uninit errors on the buffer
  value check code

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

[bruening, 2016-01-07 16:11:41]

The conditions are incorrect.

s/repeatme/repeatme()/ in commit msg

Shouldn't need another look after the fixes though so LGTM

https://codereview.appspot.com/285760043/diff/40001/tests/fuzz/fuzz_buffer.c
File tests/fuzz/fuzz_buffer.c (right):

https://codereview.appspot.com/285760043/diff/40001/tests/fuzz/fuzz_buffer.c#...
tests/fuzz/fuzz_buffer.c:76: val = buffer[i] & (buffer[i] - 1);
This worked in the compiler-generated code you showed.  It's possible some
compiler would still generate OP_test so maybe a comment that this could still
be fragile.

https://codereview.appspot.com/285760043/diff/40001/tests/fuzz/fuzz_buffer.c#...
tests/fuzz/fuzz_buffer.c:77: if (val == (uint)-1)
bug: this should be "!= 0"

https://codereview.appspot.com/285760043/diff/40001/tests/fuzz/fuzz_buffer.cpp
File tests/fuzz/fuzz_buffer.cpp (right):

https://codereview.appspot.com/285760043/diff/40001/tests/fuzz/fuzz_buffer.cp...
tests/fuzz/fuzz_buffer.cpp:104: if (val == (uint)-1)
bug: this should be "!= 0"

[bruening, 2016-01-08 22:46:09]

It looks like this was committed -- yet there's no update here?  Was this
committed manually, bypassing the review script?

On 2016/01/07 16:11:41, bruening wrote:
> s/repeatme/repeatme()/ in commit msg

This was ignored...