Inserted local redzones.

Inserted local redzones.

BUG=chromium:https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=227c9f39303e6357adcfeb4b94a905681f3616f0

[tlively, 2016-06-20 17:55:34]

tlively@google.com changed reviewers:
+ kschimpf@google.com, mcgrathr@chromium.org, stichnot@chromium.org

[tlively, 2016-06-20 17:58:51]

Description was changed from

==========
Inserted local redzones.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Inserted local redzones.

BUG=chromium:4374
==========

[tlively, 2016-06-20 17:59:34]

Description was changed from

==========
Inserted local redzones.

BUG=chromium:4374
==========

to

==========
Inserted local redzones.

BUG=chromium:https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

[Jim Stichnoth, 2016-06-20 19:44:41]

https://codereview.chromium.org/2086593002/diff/1/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2086593002/diff/1/runtime/szrt_asan.c#newcode38
runtime/szrt_asan.c:38: if (!behind_malloc)
if (behind_malloc == 0)

It's clearer when implicit compares against 0 are just used for booleans.

https://codereview.chromium.org/2086593002/diff/1/runtime/szrt_asan.c#newcode40
runtime/szrt_asan.c:40: behind_malloc++;
Generally prefer pre-increment and pre-decrement over the post- versions.

It doesn't make a difference here, for scalars, but I think it's just better to
get in the habit for when increment/decrement is applied to more complex
iterators and the like.

https://codereview.chromium.org/2086593002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2086593002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:147: SizeT Padding = (VarSize % RzSize) ? RzSize
- VarSize % RzSize : 0;
Can you use Utils::OffsetToAlignment() here?

https://codereview.chromium.org/2086593002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:158: // TODO(tlively): Make this unnecessary
I'm not sure what the exact problem is here.  Is it that the new alloca
instruction gets dead-code eliminated because of no uses of Rz?

If so, you might be able to create a FakeUse instruction after the alloca
instruction.

[tlively, 2016-06-20 22:19:34]

https://codereview.chromium.org/2086593002/diff/1/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2086593002/diff/1/runtime/szrt_asan.c#newcode38
runtime/szrt_asan.c:38: if (!behind_malloc)
On 2016/06/20 19:44:41, stichnot wrote:
> if (behind_malloc == 0)
> 
> It's clearer when implicit compares against 0 are just used for booleans.

Done.

https://codereview.chromium.org/2086593002/diff/1/runtime/szrt_asan.c#newcode40
runtime/szrt_asan.c:40: behind_malloc++;
On 2016/06/20 19:44:40, stichnot wrote:
> Generally prefer pre-increment and pre-decrement over the post- versions.
> 
> It doesn't make a difference here, for scalars, but I think it's just better
to
> get in the habit for when increment/decrement is applied to more complex
> iterators and the like.

Done.

https://codereview.chromium.org/2086593002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2086593002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:147: SizeT Padding = (VarSize % RzSize) ? RzSize
- VarSize % RzSize : 0;
On 2016/06/20 19:44:41, stichnot wrote:
> Can you use Utils::OffsetToAlignment() here?

Done.

https://codereview.chromium.org/2086593002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:158: // TODO(tlively): Make this unnecessary
On 2016/06/20 19:44:41, stichnot wrote:
> I'm not sure what the exact problem is here.  Is it that the new alloca
> instruction gets dead-code eliminated because of no uses of Rz?
> 
> If so, you might be able to create a FakeUse instruction after the alloca
> instruction.

I'll just get rid of this now. It will be unnecessary once the redzones are
used.

[Jim Stichnoth, 2016-06-20 22:26:59]

https://codereview.chromium.org/2086593002/diff/20001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2086593002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:147: SizeT Padding =
Utils::OffsetToAlignment(VarSize, 32);
Should the 32 be RzSize?

[tlively, 2016-06-21 01:11:06]

https://codereview.chromium.org/2086593002/diff/20001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2086593002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:147: SizeT Padding =
Utils::OffsetToAlignment(VarSize, 32);
On 2016/06/20 22:26:59, stichnot wrote:
> Should the 32 be RzSize?

Done.

[Karl, 2016-06-21 17:20:53]

lgtm

https://codereview.chromium.org/2086593002/diff/40001/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2086593002/diff/40001/runtime/szrt_asan.c#new...
runtime/szrt_asan.c:23: static __thread int behind_malloc = 0;
Why not use keyword thread_local?

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:99: // TODO(tlively): Make this thread safe
Fix this by making RzNum atomic.

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.h (right):

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:53: uint32_t RzNum = 0;
Why not make this std::atomic<uint32_t>, so that it is thread safe.

https://codereview.chromium.org/2086593002/diff/40001/tests_lit/asan_tests/in...
File tests_lit/asan_tests/instrumentload.ll (right):

https://codereview.chromium.org/2086593002/diff/40001/tests_lit/asan_tests/in...
tests_lit/asan_tests/instrumentload.ll:73: ; DUMP-NEXT: %dest1 = load i8, i8*
@srcConst8, align 1
Since the align argument is not used, by not remove from pattern?

(fix all cases).

[Jim Stichnoth, 2016-06-21 18:24:45]

lgtm

https://codereview.chromium.org/2086593002/diff/40001/tests_lit/asan_tests/in...
File tests_lit/asan_tests/instrumentload.ll (right):

https://codereview.chromium.org/2086593002/diff/40001/tests_lit/asan_tests/in...
tests_lit/asan_tests/instrumentload.ll:73: ; DUMP-NEXT: %dest1 = load i8, i8*
@srcConst8, align 1
On 2016/06/21 17:20:53, Karl wrote:
> Since the align argument is not used, by not remove from pattern?
> 
> (fix all cases).

I think this is just matching on the Subzero dump output.

Subzero dump doesn't bother to go to the trouble of suppressing default
alignment parameters, so I think it's safe to say this is stable output.  As
such, I would keep the CHECK lines as is.

[tlively, 2016-06-21 18:42:59]

https://codereview.chromium.org/2086593002/diff/40001/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2086593002/diff/40001/runtime/szrt_asan.c#new...
runtime/szrt_asan.c:23: static __thread int behind_malloc = 0;
On 2016/06/21 17:20:53, Karl wrote:
> Why not use keyword thread_local?

It doesn't seem to be supported. Perhaps there is an argument I could add to
make this work, but it didn't seem worth looking.

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:99: // TODO(tlively): Make this thread safe
On 2016/06/21 17:20:53, Karl wrote:
> Fix this by making RzNum atomic.

Done.

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.h (right):

https://codereview.chromium.org/2086593002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:53: uint32_t RzNum = 0;
On 2016/06/21 17:20:53, Karl wrote:
> Why not make this std::atomic<uint32_t>, so that it is thread safe.

Done.

https://codereview.chromium.org/2086593002/diff/40001/tests_lit/asan_tests/in...
File tests_lit/asan_tests/instrumentload.ll (right):

https://codereview.chromium.org/2086593002/diff/40001/tests_lit/asan_tests/in...
tests_lit/asan_tests/instrumentload.ll:73: ; DUMP-NEXT: %dest1 = load i8, i8*
@srcConst8, align 1
On 2016/06/21 18:24:45, stichnot wrote:
> On 2016/06/21 17:20:53, Karl wrote:
> > Since the align argument is not used, by not remove from pattern?
> > 
> > (fix all cases).
> 
> I think this is just matching on the Subzero dump output.
> 
> Subzero dump doesn't bother to go to the trouble of suppressing default
> alignment parameters, so I think it's safe to say this is stable output.  As
> such, I would keep the CHECK lines as is.

Acknowledged.

[tlively, 2016-06-21 18:43:10]

Description was changed from

==========
Inserted local redzones.

BUG=chromium:https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Inserted local redzones.

BUG=chromium:https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[tlively, 2016-06-21 18:43:11]

Committed patchset #4 (id:60001) manually as
227c9f39303e6357adcfeb4b94a905681f3616f0 (presubmit successful).