Subzero. Wasm. Implement sbrk and correctly do bounds checks.

src/WasmTranslator.cpp

Index: src/WasmTranslator.cpp
diff --git a/src/WasmTranslator.cpp b/src/WasmTranslator.cpp
index a3ca88d4d2ba0c68c39ec1780992ad9f8e334ba4..af37e729bd39bb53e36a32377e33799bc7c6badc 100644
--- a/src/WasmTranslator.cpp
+++ b/src/WasmTranslator.cpp
@@ -1065,9 +1065,7 @@ public:
     assert(Module);
     const auto &IndirectTable = Module->function_table;
 
-    // TODO(eholk): This should probably actually call abort instead.
-    auto *Abort = Func->makeNode();
-    Abort->appendInst(InstUnreachable::create(Func));
+    auto *Abort = getAbortTarget();
 
     assert(Args[0].toOperand());
 
@@ -1137,14 +1135,23 @@ public:
 
   Operand *sanitizeAddress(Operand *Base, uint32_t Offset) {
     SizeT MemSize = Module->module->min_mem_pages * WASM_PAGE_SIZE;
-    SizeT MemMask = MemSize - 1;
 
     bool ConstZeroBase = false;
 
     // first, add the index and the offset together.
     if (auto *ConstBase = llvm::dyn_cast<ConstantInteger32>(Base)) {
       uint32_t RealOffset = Offset + ConstBase->getValue();
-      RealOffset &= MemMask;
+      if (RealOffset >= MemSize) {
+        // We've proven this will always be an out of bounds access, so insert
+        // an unconditional trap.
+        Control()->appendInst(InstUnreachable::create(Func));
+        // It doesn't matter what we return here, so return something that will
+        // allow the rest of code generation to happen.
+        //
+        // We might be tempted to just abort translation here, but out of bounds
+        // memory access is a runtime trap, not a compile error.
+        return Base;

[Jim Stichnoth, 2016/04/22 20:48:54]

Another possibility is GlobalContext::getConstantZero() - your call.

[Eric Holk, 2016/04/22 22:32:48]

Returning 0 is better. It means that even this dereference manages to happen
somehow it will still trap by being a null pointer dereference.

Done.

+      }
       Base = Ctx->getConstantInt32(RealOffset);
       ConstZeroBase = (0 == RealOffset);
     } else if (0 != Offset) {
@@ -1156,12 +1163,18 @@ public:
       Base = Addr;
     }
 
+    // Do the bounds check.

[Jim Stichnoth, 2016/04/22 20:48:54]

Maybe in a separate CL, but can you add a flag to allow suppressing the bounds
check?  This would be for perf-testing its impact.

Even better, make the flag be numeric instead of boolean, so that we could mock
the effect of having to make 2 (or more!) cmp/br operations per bounds check.

[Eric Holk, 2016/04/22 22:32:48]

I added a TODO.

     if (!llvm::dyn_cast<ConstantInteger32>(Base)) {
-      auto *ClampedAddr = makeVariable(Ice::getPointerType());
+      auto *CheckPassed = Func->makeNode();
+      auto *CheckFailed = getAbortTarget();
+
+      auto *Check = makeVariable(IceType_i1);
+      Control()->appendInst(InstIcmp::create(Func, InstIcmp::Ult, Check, Base,
+                                             Ctx->getConstantInt32(MemSize)));
       Control()->appendInst(
-          InstArithmetic::create(Func, InstArithmetic::And, ClampedAddr, Base,
-                                 Ctx->getConstantInt32(MemSize - 1)));
-      Base = ClampedAddr;
+          InstBr::create(Func, Check, CheckPassed, CheckFailed));
+
+      *ControlPtr = OperandNode(CheckPassed);

[Jim Stichnoth, 2016/04/22 20:48:54]

This one is definitely for a separate CL.

I think it's going to be a big problem if every memory access effectively
creates a new basic block.

- Liveness analysis cost increases with the number of nodes.

- A number of low-cost optimizations are suppressed on variables with
multi-block live ranges.  Splitting "natural" blocks because of memory accesses
reduces the number of such opportunities.

To make this work, you might need to introduce a new flavor of InstBr.  There is
already one flavor for conditional branches, and one for unconditional.  The
third flavor would be an intra-block branch to a CfgNode, and this flavor of
branch would not be considered a terminator instruction.  (Actually, maybe that
should be a new instruction type separate from InstBr, to make it easier to roll
in.)

This assumes that the AbortTarget node unconditionally leaves the function and
doesn't have any out-edges to other nodes in this function.  Otherwise we might
have to make changes to liveness analysis.

One last point - in this approach, the AbortTarget wouldn't have any explicit
in-edges, so care would have to be taken that the node doesn't get removed for
being "unreachable".

[Eric Holk, 2016/04/22 22:32:48]

I added a TODO, with a link to this comment.

     }
 
     Ice::Operand *RealAddr = nullptr;
@@ -1261,6 +1274,8 @@ private:
   class Cfg *Func;
   GlobalContext *Ctx;
 
+  CfgNode *AbortTarget = nullptr;
+
   SizeT NextArg = 0;
 
   CfgUnorderedMap<Operand *, InstPhi *> PhiMap;
@@ -1297,6 +1312,16 @@ private:
     return Iter->second;
   }
 
+  CfgNode *getAbortTarget() {

[Karl, 2016/04/22 19:18:14]

Does this need an atomic or a mutex to avoid multiple generations.

You should be able to do this using an "std::atomic_bool createAbortTarget;",
initialize it to true, and then use:

   if (!AbortTarget) {
     if (createAbortTarget.exchange(false)) {
       ... create abort target
     }
   }

[Jim Stichnoth, 2016/04/22 20:48:54]

I don't think there's any concurrency at this level, so atomics should be
unnecessary.

[Eric Holk, 2016/04/22 21:02:10]

I think unless we have plans to parallelize Wasm decoding as finer granularity
than per function that there's no reason to do this.

The IceBuilder that contains this abort target is created and destroyed for each
function that is translated (see WasmTranslator::translateFunction around line
1343). There shouldn't be any way for this data to be shared between threads.

+    if (!AbortTarget) {
+      AbortTarget = Func->makeNode();

[Jim Stichnoth, 2016/04/22 20:48:54]

I think it's best if you can somehow force this node to be at the end of the
linearization list.  Or maybe it will just happen naturally during a
re-linearization pass?

[Eric Holk, 2016/04/22 22:32:48]

Do you know a good strategy for doing this off hand?

This node currently ends up somewhere in the middle of the function.

[Jim Stichnoth, 2016/04/23 16:23:04]

I would probably just do a post-pass that heavy-handedly moves it to the end of
the node vector, making sure to properly renumber the nodes.

Leave it as a TODO for now. :)

+      // TODO(eholk): This should probably actually call abort instead.
+      AbortTarget->appendInst(InstUnreachable::create(Func));
+    }
+
+    return AbortTarget;
+  }
+
   template <typename F = std::function<void(Ostream &)>> void log(F Fn) const {
     if (BuildDefs::dump() && (getFlags().getVerbose() & IceV_Wasm)) {
       Fn(Ctx->getStrDump());
@@ -1470,6 +1495,14 @@ void WasmTranslator::translate(
       WritePtr += Seg.source_size;
     }
 
+    // Save the size of the initialized data in a global variable so the runtime
+    // can use it to determine the initial heap break.
+    auto *GlobalDataSize = VariableDeclaration::createExternal(Globals.get());
+    GlobalDataSize->setName(Ctx->getGlobalString("WASM_DATA_SIZE"));
+    GlobalDataSize->addInitializer(VariableDeclaration::DataInitializer::create(
+        Globals.get(), reinterpret_cast<const char *>(&WritePtr),
+        sizeof(WritePtr)));
+
     // Pad the rest with zeros
     SizeT DataSize = Module->min_mem_pages * WASM_PAGE_SIZE;
     if (WritePtr < DataSize) {
@@ -1477,7 +1510,16 @@ void WasmTranslator::translate(
           Globals.get(), DataSize - WritePtr));
     }
 
+    // Save the number of pages for the runtime
+    auto *GlobalNumPages = VariableDeclaration::createExternal(Globals.get());
+    GlobalNumPages->setName(Ctx->getGlobalString("WASM_NUM_PAGES"));
+    GlobalNumPages->addInitializer(VariableDeclaration::DataInitializer::create(
+        Globals.get(), reinterpret_cast<const char *>(&Module->min_mem_pages),
+        sizeof(Module->min_mem_pages)));
+
     Globals->push_back(WasmMemory);
+    Globals->push_back(GlobalDataSize);
+    Globals->push_back(GlobalNumPages);
 
     lowerGlobals(std::move(Globals));
   }