Implemented global redzones.

Implemented global redzones.

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

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

[tlively, 2016-06-09 20:01:29]

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

[tlively, 2016-06-09 20:01:30]

Sorry about deleting the last CL. I know not to do that now!

[Karl, 2016-06-09 21:20:57]

You need to add some lit tests that show what happens here, before this code can
be submitted.

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

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:24: const std::string
ASanInstrumentation::RzPrefix = "rz";
Why not prefix with "$" so that it won't conflict with user names?

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:43: auto NextRzName = [&] {
Why not make this a private method of the class, or even a function in an
anonymous namespace?

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:49: auto CreateRz = [&](VariableDeclaration
*Global) {
Why not make this a private method of the class?

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:53: Rz->addInitializer(
Question? If all initializers are zero initialializers, won't they go in .bss
section?

Would it be safer to create a 1 byte data initializer?

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:87: for (int i = 0; i < 4; i++) {
Use sizeof(uint32) instead of 4. This will give you a safer solution for

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:88: SizeContents.push_back(RzArraySize % (1 <<
8));
Use CHAR_BIT for the number of bits in a character (i.e. 8).

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:89: RzArraySize /= (1 << 8);
Since RzArraySize is unsigned (or it should be if it isn't), can't you simply do

   RzArraySize >>= CHAR_BIT;

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:91: assert(RzArraySize == 0 && "There were too
many globals for ASan to instrument");
This line is too long.

In general, you should "reformat" code before submitting a CL.

This can be done by running:

   make -f Makefile.standalone format

We use this make command so that committed code always follows the same
(formatted) coding style.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:95: // Replace old list of globals
An more efficient solution for the next two statements is:

    VariableDeclarationList Empty;
    std::swap(NewGlobals, Empty);

The swap function only exchanges internal pointers and therefore, is much more
efficient.

[Jim Stichnoth, 2016-06-09 22:27:39]

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

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:24: const std::string
ASanInstrumentation::RzPrefix = "rz";
On 2016/06/09 21:20:56, Karl wrote:
> Why not prefix with "$" so that it won't conflict with user names?

Or, __rz or __$rz because "__" prefix is reserved for the compiler, and Subzero
is the compiler.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:31: if (didInsertRedZones)
I'm not sure how this code ends up getting invoked, but in a MINIMAL build I'd
like to make sure this doesn't add to the translator code size.  This is
typically done by putting this at the beginning of the function:

  if (BuildDefs::minimal())
    return;

and then all the rest of the code in the function gets eliminated.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:73: NewGlobals.push_back(RzArray);
I believe emplace_back is the newer improveder way of doing push_back, so see if
it's practical to do that here?

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:85: // update the contents of the RzArraySize
global (as a 4-byte little endian int)
80-col here and elsewhere

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:102: Ctx->getStrDump() << "========= Instrumented
Globals =========\n";
You should "lock" the dump stream so that this output isn't interleaved with
other dump output from other methods being translated in parallel.  See
GlobalContext::lockStr().

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation.h
File src/IceASanInstrumentation.h (right):

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.h:41: bool didInsertRedZones;
capitalize field name per LLVM convention

https://codereview.chromium.org/2054943002/diff/1/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/2054943002/diff/1/src/IceCompileServer.cpp#ne...
src/IceCompileServer.cpp:20: #include "IceASanInstrumentation.h"
Sort includes (if possible).

https://codereview.chromium.org/2054943002/diff/1/src/IceInstrumentation.h
File src/IceInstrumentation.h (right):

https://codereview.chromium.org/2054943002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:44: virtual void
instrumentGlobals(VariableDeclarationList &Globals) {
If you do this:

  virtual void instrumentGlobals(VariableDeclarationList &) {

you won't need to cast Globals to void.

Or, if you prefer the implicit documentation of the argument name:

  virtual void instrumentGlobals(VariableDeclarationList &/*Globals*/) {

[tlively, 2016-06-09 23:30:10]

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

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:24: const std::string
ASanInstrumentation::RzPrefix = "rz";
On 2016/06/09 22:27:38, stichnot wrote:
> On 2016/06/09 21:20:56, Karl wrote:
> > Why not prefix with "$" so that it won't conflict with user names?
> 
> Or, __rz or __$rz because "__" prefix is reserved for the compiler, and
Subzero
> is the compiler.

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:31: if (didInsertRedZones)
On 2016/06/09 22:27:39, stichnot wrote:
> I'm not sure how this code ends up getting invoked, but in a MINIMAL build I'd
> like to make sure this doesn't add to the translator code size.  This is
> typically done by putting this at the beginning of the function:
> 
>   if (BuildDefs::minimal())
>     return;
> 
> and then all the rest of the code in the function gets eliminated.

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:43: auto NextRzName = [&] {
On 2016/06/09 21:20:56, Karl wrote:
> Why not make this a private method of the class, or even a function in an
> anonymous namespace?

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:49: auto CreateRz = [&](VariableDeclaration
*Global) {
On 2016/06/09 21:20:56, Karl wrote:
> Why not make this a private method of the class?

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:53: Rz->addInitializer(
On 2016/06/09 21:20:56, Karl wrote:
> Question? If all initializers are zero initialializers, won't they go in .bss
> section?
> 
> Would it be safer to create a 1 byte data initializer?

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:73: NewGlobals.push_back(RzArray);
On 2016/06/09 22:27:38, stichnot wrote:
> I believe emplace_back is the newer improveder way of doing push_back, so see
if
> it's practical to do that here?

VariableDeclarationList is a wrapper that only exposes push_back().

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:85: // update the contents of the RzArraySize
global (as a 4-byte little endian int)
On 2016/06/09 22:27:39, stichnot wrote:
> 80-col here and elsewhere

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:87: for (int i = 0; i < 4; i++) {
On 2016/06/09 21:20:56, Karl wrote:
> Use sizeof(uint32) instead of 4. This will give you a safer solution for 

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:88: SizeContents.push_back(RzArraySize % (1 <<
8));
On 2016/06/09 21:20:56, Karl wrote:
> Use CHAR_BIT for the number of bits in a character (i.e. 8).

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:89: RzArraySize /= (1 << 8);
On 2016/06/09 21:20:56, Karl wrote:
> Since RzArraySize is unsigned (or it should be if it isn't), can't you simply
do
> 
>    RzArraySize >>= CHAR_BIT;

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:91: assert(RzArraySize == 0 && "There were too
many globals for ASan to instrument");
On 2016/06/09 21:20:56, Karl wrote:
> This line is too long.
> 
> In general, you should "reformat" code before submitting a CL.
> 
> This can be done by running:
> 
>    make -f Makefile.standalone format
> 
> We use this make command so that committed code always follows the same
> (formatted) coding style.

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:95: // Replace old list of globals
On 2016/06/09 21:20:56, Karl wrote:
> An more efficient solution for the next two statements is:
> 
>     VariableDeclarationList Empty;
>     std::swap(NewGlobals, Empty);
> 
> The swap function only exchanges internal pointers and therefore, is much more
> efficient.

Not possible due to VariableDeclarationList being a wrapper with an arena
allocator.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:102: Ctx->getStrDump() << "========= Instrumented
Globals =========\n";
On 2016/06/09 22:27:38, stichnot wrote:
> You should "lock" the dump stream so that this output isn't interleaved with
> other dump output from other methods being translated in parallel.  See
> GlobalContext::lockStr().

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation.h
File src/IceASanInstrumentation.h (right):

https://codereview.chromium.org/2054943002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.h:41: bool didInsertRedZones;
On 2016/06/09 22:27:39, stichnot wrote:
> capitalize field name per LLVM convention

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/2054943002/diff/1/src/IceCompileServer.cpp#ne...
src/IceCompileServer.cpp:20: #include "IceASanInstrumentation.h"
On 2016/06/09 22:27:39, stichnot wrote:
> Sort includes (if possible).

Done.

https://codereview.chromium.org/2054943002/diff/1/src/IceInstrumentation.h
File src/IceInstrumentation.h (right):

https://codereview.chromium.org/2054943002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:44: virtual void
instrumentGlobals(VariableDeclarationList &Globals) {
On 2016/06/09 22:27:39, stichnot wrote:
> If you do this:
> 
>   virtual void instrumentGlobals(VariableDeclarationList &) {
> 
> you won't need to cast Globals to void.
> 
> Or, if you prefer the implicit documentation of the argument name:
> 
>   virtual void instrumentGlobals(VariableDeclarationList &/*Globals*/) {

Done.

[Karl, 2016-06-10 15:20:27]

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

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:40: RzArray =
VariableDeclaration::create(&NewGlobals);
Why not combine this with line 35 as the initial value for RzArray?

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:41: RzArraySizeVar =
VariableDeclaration::create(&NewGlobals);
Same here.

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:73: if (BuildDefs::dump()) {
The preferred way of locking the Str lock is with an instance of OstreamLocker.
This class automatically locks on creation, and unlocks when destructed.

Hence, replace Ctx->lockStr() with:

   OstreamLocker _(Ctx);

And remove Ctx->unlockStr();

Note: We typically use identifier "_" to represent variables that we only want
for its initialization/destruction (i.e. no uses/references to it in the code).

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:96: llvm::NaClBitcodeRecord::RecordVector
Contents(RzSize, 'R');
Minor nit. Do you really want the contents (other than the first byte) be
uninitialized?

[Karl, 2016-06-10 15:30:49]

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

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:96: llvm::NaClBitcodeRecord::RecordVector
Contents(RzSize, 'R');
This record "Contents" is just used to provide the initializer for Rz. It is the
same for all poison variables. Why not make this a class-level field and
initialize in the constructor?

[tlively, 2016-06-10 17:30:22]

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

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:40: RzArray =
VariableDeclaration::create(&NewGlobals);
On 2016/06/10 15:20:27, Karl wrote:
> Why not combine this with line 35 as the initial value for RzArray?

Done.

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:41: RzArraySizeVar =
VariableDeclaration::create(&NewGlobals);
On 2016/06/10 15:20:27, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:73: if (BuildDefs::dump()) {
On 2016/06/10 15:20:26, Karl wrote:
> The preferred way of locking the Str lock is with an instance of
OstreamLocker.
> This class automatically locks on creation, and unlocks when destructed.
> 
> Hence, replace Ctx->lockStr() with:
> 
>    OstreamLocker _(Ctx);
> 
> And remove Ctx->unlockStr();
> 
> Note: We typically use identifier "_" to represent variables that we only want
> for its initialization/destruction (i.e. no uses/references to it in the
code).

Done.

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:96: llvm::NaClBitcodeRecord::RecordVector
Contents(RzSize, 'R');
On 2016/06/10 15:20:27, Karl wrote:
> Minor nit. Do you really want the contents (other than the first byte) be
> uninitialized?

This constructor fills the entire vector with 'R'.

https://codereview.chromium.org/2054943002/diff/20001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:96: llvm::NaClBitcodeRecord::RecordVector
Contents(RzSize, 'R');
On 2016/06/10 15:30:49, Karl wrote:
> This record "Contents" is just used to provide the initializer for Rz. It is
the
> same for all poison variables. Why not make this a class-level field and
> initialize in the constructor? 

I went a step further and made it a static member of the class.

[Karl, 2016-06-10 18:04:51]

Close to ready. You still need to add some tests.

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

https://codereview.chromium.org/2054943002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:26: llvm::NaClBitcodeRecord::RecordVector(RzSize,
'R');
Minor nit. Since these globals are not needed in the header file, why not limit
the scope by moving these constants out of the class and define in an anonymous
namespace. That will limit them to this source file in terms of access.

[tlively, 2016-06-10 19:17:22]

Tests coming in the next patch set.

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

https://codereview.chromium.org/2054943002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:26: llvm::NaClBitcodeRecord::RecordVector(RzSize,
'R');
On 2016/06/10 18:04:50, Karl wrote:
> Minor nit. Since these globals are not needed in the header file, why not
limit
> the scope by moving these constants out of the class and define in an
anonymous
> namespace. That will limit them to this source file in terms of access.

Done.

[Jim Stichnoth, 2016-06-10 22:38:33]

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

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:25: const SizeT RzSize = 32;
constexpr

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:40: VariableDeclaration *RzArray =
VariableDeclaration::create(&NewGlobals);
I would use "auto" here, and other places with the pattern
  Foo *Ptr = Foo::create();

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:44: static SizeT RzArraySize = 0;
I don't think this should be static?

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:86: std::stringstream Name;
if (BuildDefs::minimal())
  return "";

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:95: VariableDeclaration *Rz =
VariableDeclaration::create(List);
if (BuildDefs::minimal())
  return nullptr;

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

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:44: bool DidInsertRedZones;
You should be able to do this:
  bool DidInsertRedZones = false;
  uint32_t RzNum = 0;
and then you can remove those fields from the ctor initializer list above.

[tlively, 2016-06-10 23:43:32]

Added a test!

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

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:25: const SizeT RzSize = 32;
On 2016/06/10 22:38:33, stichnot wrote:
> constexpr

Done.

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:40: VariableDeclaration *RzArray =
VariableDeclaration::create(&NewGlobals);
On 2016/06/10 22:38:33, stichnot wrote:
> I would use "auto" here, and other places with the pattern
>   Foo *Ptr = Foo::create();

Done.

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:44: static SizeT RzArraySize = 0;
On 2016/06/10 22:38:33, stichnot wrote:
> I don't think this should be static?

Done.

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:86: std::stringstream Name;
On 2016/06/10 22:38:33, stichnot wrote:
> if (BuildDefs::minimal())
>   return "";

Done.

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:95: VariableDeclaration *Rz =
VariableDeclaration::create(List);
On 2016/06/10 22:38:33, stichnot wrote:
> if (BuildDefs::minimal())
>   return nullptr;

Done.

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

https://codereview.chromium.org/2054943002/diff/60001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:44: bool DidInsertRedZones;
On 2016/06/10 22:38:33, stichnot wrote:
> You should be able to do this:
>   bool DidInsertRedZones = false;
>   uint32_t RzNum = 0;
> and then you can remove those fields from the ctor initializer list above.

Done.

[Jim Stichnoth, 2016-06-11 14:26:41]

https://codereview.chromium.org/2054943002/diff/120001/src/IceASanInstrumenta...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2054943002/diff/120001/src/IceASanInstrumenta...
src/IceASanInstrumentation.cpp:53: auto *RzLeft = createRz(&NewGlobals, RzArray,
RzArraySize, Global);
I personally would not use "auto" here, because it's not clear to the casual
reader that createRz() returns VariableDeclaration* .

This is in contrast to a "Foo::create()" method, which is likely to be a factory
method returning Foo* or unique_ptr<Foo> or something similar.

https://codereview.chromium.org/2054943002/diff/120001/tests_lit/asan_tests/g...
File tests_lit/asan_tests/globalredzones.ll (right):

https://codereview.chromium.org/2054943002/diff/120001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:7: ; RUN: %lc2i -i %s --args
-verbose=global_init,inst -threads=0 -fsanitize-address \
Fix this RUN line for 80-col.

It's OK if the bitcode lines or the CHECK lines exceed 80-col, as they do below.

https://codereview.chromium.org/2054943002/diff/120001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:80: ; CHECK: func:
Should this be
  CHECK-LABEL: func
?

[tlively, 2016-06-13 17:35:54]

https://codereview.chromium.org/2054943002/diff/120001/src/IceASanInstrumenta...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2054943002/diff/120001/src/IceASanInstrumenta...
src/IceASanInstrumentation.cpp:53: auto *RzLeft = createRz(&NewGlobals, RzArray,
RzArraySize, Global);
On 2016/06/11 14:26:41, stichnot wrote:
> I personally would not use "auto" here, because it's not clear to the casual
> reader that createRz() returns VariableDeclaration* .
> 
> This is in contrast to a "Foo::create()" method, which is likely to be a
factory
> method returning Foo* or unique_ptr<Foo> or something similar.

Done.

https://codereview.chromium.org/2054943002/diff/120001/tests_lit/asan_tests/g...
File tests_lit/asan_tests/globalredzones.ll (right):

https://codereview.chromium.org/2054943002/diff/120001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:7: ; RUN: %lc2i -i %s --args
-verbose=global_init,inst -threads=0 -fsanitize-address \
On 2016/06/11 14:26:41, stichnot wrote:
> Fix this RUN line for 80-col.
> 
> It's OK if the bitcode lines or the CHECK lines exceed 80-col, as they do
below.

Done.

https://codereview.chromium.org/2054943002/diff/120001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:80: ; CHECK: func:
On 2016/06/11 14:26:41, stichnot wrote:
> Should this be
>   CHECK-LABEL: func
> ?

Done.

[Karl, 2016-06-13 17:55:50]

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
File tests_lit/asan_tests/globalredzones.ll (right):

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:5: ; RUN: %lc2i -i %s --args -threads=0
-fsanitize-address \
nit: %p2i (rather than %lc2i)is a better choice. lc2i is less efficient, and not
as robust.

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:7: ; RUN: %lc2i -i %s --args
-verbose=global_init,inst -threads=0 \
Same here.

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:42: ; DUMP-NEXT: @__$rz2 = internal
global [32 x i8] zeroinitializer
Nit: I think that DUMP-NEXT's should appear first. You may also want to add
spaces between each check.

Also, add a ; DUMP-LABEL: prefix as well (if you need to align for one, why not
the other).

[tlively, 2016-06-13 18:13:22]

Reformatted test.

[tlively, 2016-06-13 18:17:26]

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
File tests_lit/asan_tests/globalredzones.ll (right):

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:5: ; RUN: %lc2i -i %s --args -threads=0
-fsanitize-address \
On 2016/06/13 17:55:50, Karl wrote:
> nit: %p2i (rather than %lc2i)is a better choice. lc2i is less efficient, and
not
> as robust.

Done.

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:7: ; RUN: %lc2i -i %s --args
-verbose=global_init,inst -threads=0 \
On 2016/06/13 17:55:49, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:42: ; DUMP-NEXT: @__$rz2 = internal
global [32 x i8] zeroinitializer
On 2016/06/13 17:55:50, Karl wrote:
> Nit: I think that DUMP-NEXT's should appear first. You may also want to add
> spaces between each check.
> 
> Also, add a ; DUMP-LABEL: prefix as well (if you need to align for one, why
not
> the other).

Done.

[Karl, 2016-06-13 18:22:04]

lgtm

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
File tests_lit/asan_tests/globalredzones.ll (right):

https://codereview.chromium.org/2054943002/diff/140001/tests_lit/asan_tests/g...
tests_lit/asan_tests/globalredzones.ll:47: @constInitGlobal = internal constant
[32 x i8] c"ABCDEFGHIJKLMNOPQRSTUVWXYZ012345"
Note: Considering moving this to the next line (indented).

[tlively, 2016-06-13 18:23:32]

Description was changed from

==========
Implemented global redzones.

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

to

==========
Implemented global redzones.

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

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

[tlively, 2016-06-13 18:23:33]

Committed patchset #10 (id:180001) manually as
3f5cb6f3ea08dae995108130ffe8af8ec1bcbcd2 (presubmit successful).