Update base/tools_sanity_unittest.cc to cover MSan.

Update base/tools_sanity_unittest.cc to cover MSan.

BUG=178409
R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245142

[earthdok, 2014-01-15 17:35:27]

ptal

[Nico, 2014-01-15 17:46:17]

Does it make sense to run these tests under msan at all?

https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.cc
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.c...
base/tools_sanity_unittest.cc:40: VLOG(1) << "Uninit condition is true";
why this change?

[eugenis, 2014-01-15 17:49:52]

On Jan 15, 2014 9:46 PM, <thakis@chromium.org> wrote:
>
> Does it make sense to run these tests under msan at all?

Yes. It's a sanity test, the same way as it's for Valgrind. Msan doesn't
detect some of these bugs, but it detected uninitialized reads.
>
>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.cc
> File base/tools_sanity_unittest.cc (right):
>
>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.c...
> base/tools_sanity_unittest.cc:40: VLOG(1) << "Uninit condition is true";
> why this change?
>
> https://codereview.chromium.org/139723002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[earthdok, 2014-01-15 18:04:07]

On 2014/01/15 17:46:17, Nico wrote:
> Does it make sense to run these tests under msan at all?
It does, although I agree that the code probably could be structured better. The
HARMFUL_ACCESS macro is a no-op under MSan, so essentially we're only checking
that uninitialized reads from different memory locations fail as expected.

> https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.cc
> File base/tools_sanity_unittest.cc (right):
> 
>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.c...
> base/tools_sanity_unittest.cc:40: VLOG(1) << "Uninit condition is true";
> why this change?

MSan does not consider (*ptr)++ to be a sufficiently significant side effect,
which is why it did not report an error before this change.

[Nico, 2014-01-15 18:05:55]

lgtm

On 2014/01/15 18:04:07, earthdok wrote:
> On 2014/01/15 17:46:17, Nico wrote:
> > Does it make sense to run these tests under msan at all?
> It does, although I agree that the code probably could be structured better.
The
> HARMFUL_ACCESS macro is a no-op under MSan, so essentially we're only checking
> that uninitialized reads from different memory locations fail as expected.
> 
> >
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.cc
> > File base/tools_sanity_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.c...
> > base/tools_sanity_unittest.cc:40: VLOG(1) << "Uninit condition is true";
> > why this change?
> 
> MSan does not consider (*ptr)++ to be a sufficiently significant side effect,
> which is why it did not report an error before this change.

Huh, it looks kinda sufficient to me – do you consider this an msan bug?

[eugenis, 2014-01-15 18:09:22]

I think *ptr++ is here to stop clang from optimizing the branch away.
MSan is expected to report use-of-unitialized-value on the branch.



On Wed, Jan 15, 2014 at 10:04 PM,  <earthdok@chromium.org> wrote:
> On 2014/01/15 17:46:17, Nico wrote:
>>
>> Does it make sense to run these tests under msan at all?
>
> It does, although I agree that the code probably could be structured better.
> The
> HARMFUL_ACCESS macro is a no-op under MSan, so essentially we're only
> checking
> that uninitialized reads from different memory locations fail as expected.
>
>
>>
>>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.cc
>> File base/tools_sanity_unittest.cc (right):
>
>
>
>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.c...
>>
>> base/tools_sanity_unittest.cc:40: VLOG(1) << "Uninit condition is true";
>> why this change?
>
>
> MSan does not consider (*ptr)++ to be a sufficiently significant side
> effect,
> which is why it did not report an error before this change.
>
> https://codereview.chromium.org/139723002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eugenis, 2014-01-15 18:11:47]

This needs a closer look, but I see an obvious way of flattening this
code that would hide the report.
Something like *(ptr) += ((*ptr == 64) * 2 - 1).
Try changing the whole if() to
if (*ptr)
  printf("something");  // any call to external function here

On Wed, Jan 15, 2014 at 10:09 PM, Evgeniy Stepanov <eugenis@chromium.org>
wrote:
> I think *ptr++ is here to stop clang from optimizing the branch away.
> MSan is expected to report use-of-unitialized-value on the branch.
>
>
>
> On Wed, Jan 15, 2014 at 10:04 PM,  <earthdok@chromium.org> wrote:
>> On 2014/01/15 17:46:17, Nico wrote:
>>>
>>> Does it make sense to run these tests under msan at all?
>>
>> It does, although I agree that the code probably could be structured better.
>> The
>> HARMFUL_ACCESS macro is a no-op under MSan, so essentially we're only
>> checking
>> that uninitialized reads from different memory locations fail as expected.
>>
>>
>>>
>>>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.cc
>>> File base/tools_sanity_unittest.cc (right):
>>
>>
>>
>>
https://codereview.chromium.org/139723002/diff/1/base/tools_sanity_unittest.c...
>>>
>>> base/tools_sanity_unittest.cc:40: VLOG(1) << "Uninit condition is true";
>>> why this change?
>>
>>
>> MSan does not consider (*ptr)++ to be a sufficiently significant side
>> effect,
>> which is why it did not report an error before this change.
>>
>> https://codereview.chromium.org/139723002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[earthdok, 2014-01-15 18:24:27]

Yeah, I spoke too soon. The branch most likely gets optimized away. I'll look
into it.

On 2014/01/15 18:11:47, eugenis wrote:
> This needs a closer look, but I see an obvious way of flattening this
> code that would hide the report.
> Something like *(ptr) += ((*ptr == 64) * 2 - 1).
> Try changing the whole if() to
> if (*ptr)
>   printf("something");  // any call to external function here

Isn't this exactly what I did?

[commit-bot: I haz the power, 2014-01-15 18:51:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/139723002/1

[Nico, 2014-01-15 19:03:05]

On 2014/01/15 18:51:04, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/earthdok%40chromium.org/139723002/1

"Yeah, I spoke too soon. The branch most likely gets optimized away. I'll look
into it." <- so the result of that investigation was "nothing to do"?

[earthdok, 2014-01-15 19:27:49]

On 2014/01/15 19:03:05, Nico wrote:
> On 2014/01/15 18:51:04, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> > https://chromium-status.appspot.com/cq/earthdok%2540chromium.org/139723002/1
> 
> "Yeah, I spoke too soon. The branch most likely gets optimized away. I'll look
> into it." <- so the result of that investigation was "nothing to do"?

I'm still looking. If you think this should block the commit then I'll cancel.

[Nico, 2014-01-15 19:30:46]

On Wed, Jan 15, 2014 at 11:27 AM, <earthdok@chromium.org> wrote:

> On 2014/01/15 19:03:05, Nico wrote:
>
>> On 2014/01/15 18:51:04, I haz the power (commit-bot) wrote:
>> > CQ is trying da patch. Follow status at
>> > https://chromium-status.appspot.com/cq/earthdok%
>> 2540chromium.org/139723002/1
>>
>
>  "Yeah, I spoke too soon. The branch most likely gets optimized away. I'll
>> look
>> into it." <- so the result of that investigation was "nothing to do"?
>>
>
> I'm still looking. If you think this should block the commit then I'll
> cancel.
>

The cq doesn't run asan/valgrind, so if you think this will break
asan/valgrind it'd make sense to check that before landing.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[earthdok, 2014-01-15 22:19:04]

On 2014/01/15 19:30:46, Nico wrote:
> On Wed, Jan 15, 2014 at 11:27 AM, <mailto:earthdok@chromium.org> wrote:
> 
> > On 2014/01/15 19:03:05, Nico wrote:
> >
> >> On 2014/01/15 18:51:04, I haz the power (commit-bot) wrote:
> >> > CQ is trying da patch. Follow status at
> >> > https://chromium-status.appspot.com/cq/earthdok%25
> >> 2540chromium.org/139723002/1
> >>
> >
> >  "Yeah, I spoke too soon. The branch most likely gets optimized away. I'll
> >> look
> >> into it." <- so the result of that investigation was "nothing to do"?
> >>
> >
> > I'm still looking. If you think this should block the commit then I'll
> > cancel.
> >
> 
> The cq doesn't run asan/valgrind, so if you think this will break
> asan/valgrind it'd make sense to check that before landing.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ran this through linux_valgrind. ASan should not be affected.

[commit-bot: I haz the power, 2014-01-15 22:26:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/139723002/1

[commit-bot: I haz the power, 2014-01-16 05:31:45]

Change committed as 245142

[eugenis, 2014-01-16 13:11:33]

LLVM IR for this function uses "select %cmp, 1, -1" and then adds this value to
(*ptr). MSan propagates shadow for "select" instruction, keeping (*ptr)
uninitialized but not printing any report just yet.
X86 assembly for this function contains a branch, so Valgrind is unable to
propagate and has to report an error.

[Nico, 2014-01-16 19:38:01]

On Thu, Jan 16, 2014 at 5:11 AM, <eugenis@chromium.org> wrote:

> LLVM IR for this function uses "select %cmp, 1, -1" and then adds this
> value to
> (*ptr). MSan propagates shadow for "select" instruction, keeping (*ptr)
> uninitialized but not printing any report just yet.
> X86 assembly for this function contains a branch, so Valgrind is unable to
> propagate and has to report an error.
>

I think valgrind maybe also doesn't report unitialized values in the flag
register / cmov – at least I think I saw that once or twice (?)


>
>
> https://codereview.chromium.org/139723002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.