Fix problems with sandboxing and the ARM integrated assembler.

src/IceAssembler.h

Index: src/IceAssembler.h
diff --git a/src/IceAssembler.h b/src/IceAssembler.h
index d80e3d26e9b4f4f1b8d2a09e06e4539efb7993b7..0febc624b52384cc4234a5021754a1b6bbd8ebda 100644
--- a/src/IceAssembler.h
+++ b/src/IceAssembler.h
@@ -32,6 +32,8 @@
 
 namespace Ice {
 
+class Assembler;
+
 /// A Label can be in one of three states:
 ///  - Unused.
 ///  - Linked, unplaced and tracking the position of branches to the label.
@@ -82,11 +84,7 @@ public:
     assert(isBound());
   }
 
-  void linkTo(intptr_t position) {
-    assert(!isBound());
-    Position = position + kWordSize;
-    assert(isLinked());
-  }
+  inline void linkTo(const Assembler &Asm, intptr_t position);

[Jim Stichnoth, 2015/12/08 22:20:00]

Does the "inline" really help here?

Also, we may be a bit inconsistent about whether to use Assembler & versus
Assembler *.  This version is probably reasonable, but just wondering if you
considered it.

[Karl, 2015/12/08 22:39:32]

The "inline" is needed if we don't want the function to be compiled into every
source, but maintain the property that it is still inlined. However, since you
suggest moving the implementation to the cpp file, this is mute. Removing the
inline.

As far as why I used "const Assembler &Asm" rather than "const Assembler *Asm",
there are two (correlated) reasons:

1) The const has different meanings. The first is that the Assembler instance is
const. The second is that the pointer is const (not the instance).

2) The compiler insists on a cast on derived classes for the second form, but
not the first.

Hence, I used "const Assembler &Asm".

[Jim Stichnoth, 2015/12/08 23:06:44]

No - "const Assembler *Asm" means that the pointed-to object is constant - "Asm
= Asm2;" is fine but "Asm->some_field++;" is not.  What you are describing is
"Assembler *const Asm".
Not sure I understand this.  Upcasts shouldn't require an explicit case.  For
example,
  Variable *Var = ...;
  Operand *Opnd = Var;
is fine, and similarly a Variable* can be passed as an Operand* parameter.
No big deal, because I think a stylistic argument could be made for your choice.

 
 protected:
   intptr_t Position = 0;
@@ -354,6 +352,14 @@ protected:
   AssemblerBuffer Buffer;
 };
 
+void Label::linkTo(const Assembler &Asm, intptr_t position) {

[Jim Stichnoth, 2015/12/08 22:20:00]

I think this should be moved into the .cpp file.

Also, "position" should be capitalized (and probably also renamed to avoid
shadowing).

[Karl, 2015/12/08 22:39:32]

Done.

+  if (Asm.getPreliminary() || Asm.needsTextFixup())

[Jim Stichnoth, 2015/12/08 22:20:00]

I think it's important to leave some documentation here.  Suggestion:

// We must not set the link until the Position is absolutely known.  This means
not during the preliminary (sandboxing) pass, and not when the instruction needs
a text fixup (hybrid iasm mode).

[Karl, 2015/12/08 22:39:32]

Done.

+    return;
+  assert(!isBound());
+  Position = position + kWordSize;
+  assert(isLinked());
+}
+
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICEASSEMBLER_H_