Subzero: Implemented codegen for poisoning and unpoisoning stack redzones

Subzero: Implemented codegen for poisoning and unpoisoning stack redzones

BUG=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=cbd3dbc6f00a7784f523132910cc9adbdfa09bf2

[tlively, 2016-07-29 21:30:38]

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

[Karl, 2016-08-01 15:40:21]

lgtm

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

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:181: (VarSizeOp =
llvm::dyn_cast<ConstantInteger32>(Cur->getSizeInBytes()))) {
Doesn't this code assume that all Allocas are at the beginning? Is this a good
assumption?

Also, if the size of an alloc (in bytes) is zero, won't this stop the loop?

[tlively, 2016-08-01 17:08:51]

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

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:181: (VarSizeOp =
llvm::dyn_cast<ConstantInteger32>(Cur->getSizeInBytes()))) {
On 2016/08/01 15:40:21, Karl wrote:
> Doesn't this code assume that all Allocas are at the beginning? Is this a good
> assumption?
> 
> Also, if the size of an alloc (in bytes) is zero, won't this stop the loop?

I believe that allocas of known size corresponding to stack variables are always
at the beginning. I don't currently do anything with dynamic allocas (though I
should).

The loops would only stop if a zero byte alloca is represented with a nullptr as
its size operand. I'm not sure whether that's the case. Also I'm not sure that a
stack variable would ever be zero bytes. Can you confirm?

[Jim Stichnoth, 2016-08-01 20:38:01]

otherwise lgtm

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

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:186: Variable *LastRzVar =
Func->makeVariable(IceType_i32);
auto * ?

(you used it below for ShadowIndexVar)

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:220: auto *NewVar = InstAlloca::create(Func,
Dest, ByteCount, Alignment);
Could you name this something like "NewAllocaVar" or similar?

When I see "auto *NewVar = ...", I assume its type is Variable * .

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:223: SizeT Zeros = VarSize >> ShadowScaleLog2;
Declare scalar locals as const when possible/practical/reasonable.

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:243: for (SizeT Offset = 0; Iter !=
PoisonVals.end(); Offset += 4) {
Instead of magic constant "4", can you make it a static constexpr with a
meaningful name?  e.g. BytesPerWord or RedZoneWords

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:245: for (int I = 0; I < 4; ++I) {
Can you use int32_t or uint32_t instead of int here, for consistency across the
code base?

Also, for silly index variables like this, feel free to name them lower-case
"i", "j", etc.  Even the LLVM code base does this sometimes.

[Karl, 2016-08-04 14:26:34]

Otherwise LGTM.

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

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:181: (VarSizeOp =
llvm::dyn_cast<ConstantInteger32>(Cur->getSizeInBytes()))) {
On 2016/08/01 17:08:50, tlively wrote:
> On 2016/08/01 15:40:21, Karl wrote:
> > Doesn't this code assume that all Allocas are at the beginning? Is this a
good
> > assumption?
> > 
> > Also, if the size of an alloc (in bytes) is zero, won't this stop the loop?
> 
> I believe that allocas of known size corresponding to stack variables are
always
> at the beginning. I don't currently do anything with dynamic allocas (though I
> should).
> 
> The loops would only stop if a zero byte alloca is represented with a nullptr
as
> its size operand. I'm not sure whether that's the case. Also I'm not sure that
a
> stack variable would ever be zero bytes. Can you confirm?

I don't know the answer to this. I was asking because I didn't know.

[Jim Stichnoth, 2016-08-04 14:42:54]

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

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:181: (VarSizeOp =
llvm::dyn_cast<ConstantInteger32>(Cur->getSizeInBytes()))) {
On 2016/08/04 14:26:34, Karl wrote:
> On 2016/08/01 17:08:50, tlively wrote:
> > On 2016/08/01 15:40:21, Karl wrote:
> > > Doesn't this code assume that all Allocas are at the beginning? Is this a
> good
> > > assumption?
> > > 
> > > Also, if the size of an alloc (in bytes) is zero, won't this stop the
loop?
> > 
> > I believe that allocas of known size corresponding to stack variables are
> always
> > at the beginning. I don't currently do anything with dynamic allocas (though
I
> > should).
> > 
> > The loops would only stop if a zero byte alloca is represented with a
nullptr
> as
> > its size operand. I'm not sure whether that's the case. Also I'm not sure
that
> a
> > stack variable would ever be zero bytes. Can you confirm?
> 
> I don't know the answer to this. I was asking because I didn't know.

Good point Karl.  All the fixed-size alloca instructions (at least those we care
about) will appear in the entry block, but they could be interleaved with other
instructions.

See Cfg::processAllocas() which iterates through the entire entry block without
an early exit.

[tlively, 2016-08-05 18:24:53]

Blocked on https://bugs.chromium.org/p/nativeclient/issues/detail?id=4377 to
pass presubmit.

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

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:181: (VarSizeOp =
llvm::dyn_cast<ConstantInteger32>(Cur->getSizeInBytes()))) {
On 2016/08/04 14:42:54, stichnot wrote:
> On 2016/08/04 14:26:34, Karl wrote:
> > On 2016/08/01 17:08:50, tlively wrote:
> > > On 2016/08/01 15:40:21, Karl wrote:
> > > > Doesn't this code assume that all Allocas are at the beginning? Is this
a
> > good
> > > > assumption?
> > > > 
> > > > Also, if the size of an alloc (in bytes) is zero, won't this stop the
> loop?
> > > 
> > > I believe that allocas of known size corresponding to stack variables are
> > always
> > > at the beginning. I don't currently do anything with dynamic allocas
(though
> I
> > > should).
> > > 
> > > The loops would only stop if a zero byte alloca is represented with a
> nullptr
> > as
> > > its size operand. I'm not sure whether that's the case. Also I'm not sure
> that
> > a
> > > stack variable would ever be zero bytes. Can you confirm?
> > 
> > I don't know the answer to this. I was asking because I didn't know.
> 
> Good point Karl.  All the fixed-size alloca instructions (at least those we
care
> about) will appear in the entry block, but they could be interleaved with
other
> instructions.
> 
> See Cfg::processAllocas() which iterates through the entire entry block
without
> an early exit.

Done.

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:186: Variable *LastRzVar =
Func->makeVariable(IceType_i32);
On 2016/08/01 20:38:00, stichnot wrote:
> auto * ?
> 
> (you used it below for ShadowIndexVar)

Done.

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:220: auto *NewVar = InstAlloca::create(Func,
Dest, ByteCount, Alignment);
On 2016/08/01 20:38:00, stichnot wrote:
> Could you name this something like "NewAllocaVar" or similar?
> 
> When I see "auto *NewVar = ...", I assume its type is Variable * .

Done.

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:223: SizeT Zeros = VarSize >> ShadowScaleLog2;
On 2016/08/01 20:38:01, stichnot wrote:
> Declare scalar locals as const when possible/practical/reasonable.

Done.

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:243: for (SizeT Offset = 0; Iter !=
PoisonVals.end(); Offset += 4) {
On 2016/08/01 20:38:01, stichnot wrote:
> Instead of magic constant "4", can you make it a static constexpr with a
> meaningful name?  e.g. BytesPerWord or RedZoneWords

Done.

https://codereview.chromium.org/2194853003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:245: for (int I = 0; I < 4; ++I) {
On 2016/08/01 20:38:01, stichnot wrote:
> Can you use int32_t or uint32_t instead of int here, for consistency across
the
> code base?
> 
> Also, for silly index variables like this, feel free to name them lower-case
> "i", "j", etc.  Even the LLVM code base does this sometimes.

Done.

[Karl, 2016-08-05 19:06:19]

LGTM once blocker handled.

[tlively, 2016-08-09 22:02:43]

Description was changed from

==========
Subzero: Implemented codegen for poisoning and unpoisoning stack redzones

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

to

==========
Subzero: Implemented codegen for poisoning and unpoisoning stack redzones

BUG=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-08-09 22:02:44]

Committed patchset #3 (id:40001) manually as
cbd3dbc6f00a7784f523132910cc9adbdfa09bf2 (presubmit successful).