Fix problems with sandboxing and the ARM integrated assembler.

src/IceAssembler.h

 //===- subzero/src/IceAssembler.h - Integrated assembler --------*- C++ -*-===//
 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 //
 // Modified by the Subzero authors.
 //
 //===----------------------------------------------------------------------===//
 //
 //                        The Subzero Code Generator
@@ skipping 14 matching lines @@
 
 #ifndef SUBZERO_SRC_ICEASSEMBLER_H
 #define SUBZERO_SRC_ICEASSEMBLER_H
 
 #include "IceDefs.h"
 #include "IceFixups.h"
 #include "IceUtils.h"
 
 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.
 ///  - Bound, placed and tracking its position.
 class Label {
   Label(const Label &) = delete;
   Label &operator=(const Label &) = delete;
 
 public:
   Label() = default;
@@ skipping 30 matching lines @@
   bool isLinked() const { return Position > 0; }
 
   virtual bool isUnused() const { return Position == 0; }
 
   void bindTo(intptr_t position) {
     assert(!isBound());
     Position = -position - kWordSize;
     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;
 
   // TODO(jvoung): why are labels offset by this?
   static constexpr uint32_t kWordSize = sizeof(uint32_t);
 };
 
 /// Assembler buffers are used to emit binary code. They grow on demand.
 class AssemblerBuffer {
@@ skipping 247 matching lines @@
   /// Installs a created fixup, after it has been allocated.
   void installFixup(AssemblerFixup *F) { Buffer.installFixup(F); }
 
 protected:
   // Buffer's constructor uses the Allocator, so it needs to appear after it.
   // TODO(jpp): dependencies on construction order are a nice way of shooting
   // yourself in the foot. Fix this.
   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_