Prevent clang from optimizing away a malloc in a test.

clang/win: Prevent clang from optimizing away a malloc in a test.

Similar to e.g. https://codereview.chromium.org/8429008 and
https://codereview.chromium.org/8429008 .

BUG=448935
R=scottmg@chromium.org, wfh@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/33fd64cf5b5e6cff7c96bd74051a0494d45b879a

[Nico, 2015-01-22 19:44:34]

thakis@chromium.org changed reviewers:
+ wfh@chromium.org

[scottmg, 2015-01-22 19:46:21]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-01-22 19:46:22]

lgtm

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc#ne...
base/security_unittest.cc:45: // Without the |volatile|, clang optimizez away
the allocation.
optimizes

[Will Harris, 2015-01-22 19:46:45]

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc#ne...
base/security_unittest.cc:45: // Without the |volatile|, clang optimizez away
the allocation.
ah. interesting.  We actually have HideValueFromCompiler below whose purpose is
to solve this generically.  Perhaps HideValueFromCompiler could be changed to
understand clang, and then used here instead?

[scottmg, 2015-01-22 19:47:28]

(It's interesting that `malloc()` isn't considered to have side-effects, I
wouldn't have guess that was allowed.)

[Nico, 2015-01-22 19:48:31]

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc#ne...
base/security_unittest.cc:45: // Without the |volatile|, clang optimizez away
the allocation.
On 2015/01/22 19:46:45, Will Harris wrote:
> ah. interesting.  We actually have HideValueFromCompiler below whose purpose
is
> to solve this generically.  Perhaps HideValueFromCompiler could be changed to
> understand clang, and then used here instead?

What's wrong with volatile? That's kind of the language-supported way of doing
this (and also what we use in other files doing similar things, see links to
other cls in the cl description.)

https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc#ne...
base/security_unittest.cc:45: // Without the |volatile|, clang optimizez away
the allocation.
On 2015/01/22 19:46:21, scottmg wrote:
> optimizes

Done.

[Nico, 2015-01-22 19:49:54]

On 2015/01/22 19:48:31, Nico wrote:
> https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc
> File base/security_unittest.cc (right):
> 
>
https://codereview.chromium.org/868743003/diff/1/base/security_unittest.cc#ne...
> base/security_unittest.cc:45: // Without the |volatile|, clang optimizez away
> the allocation.
> On 2015/01/22 19:46:45, Will Harris wrote:
> > ah. interesting.  We actually have HideValueFromCompiler below whose purpose
> is
> > to solve this generically.  Perhaps HideValueFromCompiler could be changed
to
> > understand clang, and then used here instead?
> 
> What's wrong with volatile? That's kind of the language-supported way of doing
> this (and also what we use in other files doing similar things, see links to
> other cls in the cl description.)

Looking at the uses of HideValueFromCompiler(), it's usually not used with a
decl, so volatile can't be used easily there. But here, just making the decl
volatile seems like the right thing to do to me.

[Will Harris, 2015-01-22 19:50:19]

On 2015/01/22 19:48:31, Nico wrote:
> What's wrong with volatile? That's kind of the language-supported way of doing
> this (and also what we use in other files doing similar things, see links to
> other cls in the cl description.)

okay, the HideValueFromCompiler must be there specifically for a gcc issue where
volatile isn't enough.

I can chat to jln@ who added this extra wrapper and make sure it's still needed
in the latest gccs

[Will Harris, 2015-01-22 19:51:00]

you wait a long time for an lgtm then two come along at the same time.

lgtm

[Nico, 2015-01-22 19:52:25]

> I can chat to jln@ who added this extra wrapper and make sure it's still
needed

I can believe it's needed in the below, but it probably isn't needed here.

On 2015/01/22 19:51:00, Will Harris wrote:
> you wait a long time for an lgtm then two come along at the same time.

Yeah, what's up with having to wait 5 minutes for a review! ;-)

> lgtm

Thanks!

[Nico, 2015-01-22 19:52:28]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-01-22 19:53:37]

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

[commit-bot: I haz the power, 2015-01-22 20:37:28]

Patchset 2 (id:??) landed as
https://crrev.com/33fd64cf5b5e6cff7c96bd74051a0494d45b879a
Cr-Commit-Position: refs/heads/master@{#312673}

[Nico, 2015-01-22 20:37:40]

Committed patchset #2 (id:20001) manually as
33fd64cf5b5e6cff7c96bd74051a0494d45b879a (presubmit successful).