Fix problems with sandboxing and the ARM integrated assembler.

src/IceAssemblerARM32.cpp

Index: src/IceAssemblerARM32.cpp
diff --git a/src/IceAssemblerARM32.cpp b/src/IceAssemblerARM32.cpp
index d06ee7b6ddb4d41458101a6bea5e4c0f3d9363a6..421a3dee051cd264ea4193e7670043ed8233d5da 100644
--- a/src/IceAssemblerARM32.cpp
+++ b/src/IceAssemblerARM32.cpp
@@ -460,6 +460,14 @@ size_t BlRelocatableFixup::emit(GlobalContext *Ctx,
   return InstARM32::InstSize;
 }
 
+void AssemblerARM32::padWithNop(intptr_t Padding) {
+  constexpr intptr_t InstWidth = sizeof(IValueT);
+  assert(Padding % InstWidth == 0 &&
+         "Padding not mulitple of instruction size");

[Jim Stichnoth, 2015/12/08 19:54:58]

multiple

[Karl, 2015/12/08 20:49:36]

Done.

+  for (intptr_t i = 0; i < Padding; i += InstWidth)
+    nop();
+}
+
 BlRelocatableFixup *
 AssemblerARM32::createBlFixup(const ConstantRelocatable *BlTarget) {
   BlRelocatableFixup *F =
@@ -651,7 +659,7 @@ void AssemblerARM32::emitBranch(Label *L, CondARM32::Cond Cond, bool Link) {
   const IOffsetT Position = Buffer.size();
   // Use the offset field of the branch instruction for linking the sites.
   emitType05(Cond, L->getEncodedPosition(), Link, BranchName);
-  if (!needsTextFixup())
+  if (!needsTextFixup() && !getPreliminary())

[Jim Stichnoth, 2015/12/08 20:52:27]

This is clearly a problem that affects all assemblers, and so it would be great
to factor it up to the base Assembler class.

Can you try modifying Label::linkTo() to also take a const Assembler * argument,
and then do an early return if Assembler::getPreliminary() is true?  Then the
getPreliminary() checks are unneeded for the target-specific assemblers.

The same change should be applied to the x86 nearLinkTo() method.

I think this is simple enough that it could be part of this CL, or it could be a
separate CL if you like.

[Karl, 2015/12/08 21:38:11]

Moved check inside linkTo() and nearLinkTo(). Code now reads like there is a
single pass, even if there isn't.

     L->linkTo(Position);
 }
 
@@ -1272,6 +1280,17 @@ void AssemblerARM32::mvn(const Operand *OpRd, const Operand *OpSrc,
              MvnName);
 }
 
+void AssemblerARM32::nop(CondARM32::Cond Cond) {
+  // NOP - Section A8.8.119, encoding A1:
+  //  nop<c>
+  //
+  // cccc0011001000001111000000000000 where cccc=Cond.
+  AssemblerBuffer::EnsureCapacity ensured(&Buffer);
+  const IValueT Encoding = (encodeCondition(Cond) << kConditionShift) |
+                           (B25 | B24 | B21 | B15 | B14 | B13 | B12);
+  emitInst(Encoding);
+}
+
 void AssemblerARM32::sbc(const Operand *OpRd, const Operand *OpRn,
                          const Operand *OpSrc1, bool SetFlags,
                          CondARM32::Cond Cond) {
@@ -1365,7 +1384,6 @@ void AssemblerARM32::str(const Operand *OpRt, const Operand *OpAddress,
     // iiiiiiiiiiii=imm12, u=1 if +.
     constexpr bool IsByte = false;
     return emitMemOp(Cond, IsLoad, IsByte, Rt, OpAddress, TInfo, StrName);

[John, 2015/12/08 19:45:04]

Optional: I know the language allows to return a void expression expression, but
would you mind doing:

  emitMemOp(...);
  return;

instead?

[Jim Stichnoth, 2015/12/08 19:54:58]

This pattern of returning a void expression was suggested (by me) and first
added in:

https://chromiumcodereview.appspot.com/1418523002/#msg4

After reacquainting myself with the issue, I agree that this pattern should
probably only be used with setNeedsTextFixup(), and would ultimately disappear
when full iasm support is there.

[Karl, 2015/12/08 19:58:35]

I mainly started doing this because it was much easier to write:

  if (...)
    return setNeedsTextFixup();

rather than:

  if (...) {
    setNeedsTextFixup();
    return;
  }

To be consistent, I started doing it whenever the last element was a call. I can
change it none the less.

[Karl, 2015/12/08 20:49:36]

Removing returns of this form.

-    return setNeedsTextFixup();
   }
   }
 }