Implemented global redzones.

src/IceASanInstrumentation.cpp

+//===- subzero/src/IceASanInstrumentation.cpp - ASan ------------*- C++ -*-===//
+//
+//                        The Subzero Code Generator
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// \brief Implements the AddressSanitizer instrumentation class.
+///
+//===----------------------------------------------------------------------===//
+
+#include "IceASanInstrumentation.h"
+
+#include "IceBuildDefs.h"
+#include "IceGlobalInits.h"
+
+#include <sstream>
+
+namespace Ice {
+
+const std::string ASanInstrumentation::RzPrefix = "rz";

[Karl, 2016/06/09 21:20:56]

Why not prefix with "$" so that it won't conflict with user names?

[Jim Stichnoth, 2016/06/09 22:27:38]

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

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

Done.

+
+// Create redzones between all global variables, ensuring that the initializer
+// types of the redzones and their associated globals match so that they are
+// laid out together in memory.
+// TODO(tlively): make everything here thread safe, including dumps
+void ASanInstrumentation::instrumentGlobals(VariableDeclarationList &Globals) {
+  if (didInsertRedZones)

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

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.

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

Done.

+    return;
+
+  VariableDeclarationList NewGlobals;
+  // Global holding pointers to all redzones
+  VariableDeclaration *RzArray;
+  // Global holding the size of RzArray
+  VariableDeclaration *RzArraySizeVar;
+  static SizeT RzArraySize;
+
+  // used to construct new redzones
+  uint32_t RzNum = 0;
+  auto NextRzName = [&] {

[Karl, 2016/06/09 21:20:56]

Why not make this a private method of the class, or even a function in an
anonymous namespace?

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

Done.

+    std::stringstream Name;
+    Name << RzPrefix << "." << RzNum++;
+    return Name.str();
+  };
+
+  auto CreateRz = [&](VariableDeclaration *Global) {

[Karl, 2016/06/09 21:20:56]

Why not make this a private method of the class?

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

Done.

+    VariableDeclaration *Rz = VariableDeclaration::create(&NewGlobals);
+    Rz->setName(Ctx, NextRzName());
+    if (Global->hasNonzeroInitializer()) {
+      Rz->addInitializer(

[Karl, 2016/06/09 21:20:56]

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?

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

Done.

+          VariableDeclaration::ZeroInitializer::create(&NewGlobals, RzSize-1));
+      Rz->addInitializer(
+          VariableDeclaration::ZeroInitializer::create(&NewGlobals, 1));
+    } else {
+      Rz->addInitializer(
+          VariableDeclaration::ZeroInitializer::create(
+              &NewGlobals, RzSize));
+    }
+    Rz->setIsConstant(Global->getIsConstant());
+    RzArray->addInitializer(VariableDeclaration::RelocInitializer::create(
+        &NewGlobals, Rz, RelocOffsetArray(0)));
+    ++RzArraySize;
+    return Rz;
+  };
+
+  RzArray = VariableDeclaration::create(&NewGlobals);
+  RzArraySizeVar = VariableDeclaration::create(&NewGlobals);
+  RzArray->setName(Ctx, NextRzName());
+  RzArraySizeVar->setName(Ctx, NextRzName());
+  NewGlobals.push_back(RzArray);

[Jim Stichnoth, 2016/06/09 22:27:38]

I believe emplace_back is the newer improveder way of doing push_back, so see if
it's practical to do that here?

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

VariableDeclarationList is a wrapper that only exposes push_back().

+  NewGlobals.push_back(RzArraySizeVar);
+
+  // TODO(tlively): Consider alignment when determining redzone layout.
+  for (VariableDeclaration *Global : Globals) {
+    VariableDeclaration *RzLeft = CreateRz(Global);
+    VariableDeclaration *RzRight = CreateRz(Global);
+    NewGlobals.push_back(RzLeft);
+    NewGlobals.push_back(Global);
+    NewGlobals.push_back(RzRight);
+  }
+
+  // update the contents of the RzArraySize global (as a 4-byte little endian int)

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

80-col here and elsewhere

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

Done.

+  llvm::NaClBitcodeRecord::RecordVector SizeContents;
+  for (int i = 0; i < 4; i++) {

[Karl, 2016/06/09 21:20:56]

Use sizeof(uint32) instead of 4. This will give you a safer solution for 

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

Done.

+    SizeContents.push_back(RzArraySize % (1 << 8));

[Karl, 2016/06/09 21:20:56]

Use CHAR_BIT for the number of bits in a character (i.e. 8).

[tlively, 2016/06/09 23:30:09]

Done.

+    RzArraySize /= (1 << 8);

[Karl, 2016/06/09 21:20:56]

Since RzArraySize is unsigned (or it should be if it isn't), can't you simply do

   RzArraySize >>= CHAR_BIT;

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

Done.

+  }
+  assert(RzArraySize == 0 && "There were too many globals for ASan to instrument");

[Karl, 2016/06/09 21:20:56]

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.

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

Done.

+  RzArraySizeVar->addInitializer(VariableDeclaration::DataInitializer::create(
+      &NewGlobals, SizeContents));
+
+  // Replace old list of globals

[Karl, 2016/06/09 21:20:56]

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.

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

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

+  Globals.clear();
+  Globals.merge(&NewGlobals);
+  didInsertRedZones = true;
+
+  // Log the new set of globals
+  if (BuildDefs::dump()) {
+    Ctx->getStrDump() << "========= Instrumented Globals =========\n";

[Jim Stichnoth, 2016/06/09 22:27:38]

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().

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

Done.

+    for (VariableDeclaration *Global : Globals) {
+      Global->dump(Ctx->getStrDump());
+    }
+  }
+}
+
+} // end of namespace Ice