Add UBSAN build option and fix undefined behaviour errors.

src/IceInst.h

Index: src/IceInst.h
diff --git a/src/IceInst.h b/src/IceInst.h
index 360135a133b144c05cfe4013e8ff110cef550192..b147a5dc8e3448dfa3f692b34faffe10d25e7df0 100644
--- a/src/IceInst.h
+++ b/src/IceInst.h
@@ -21,21 +21,20 @@
 #include "IceDefs.h"
 #include "IceInst.def"
 #include "IceIntrinsics.h"
+#include "IceTargetKinds.def"
 #include "IceTypes.h"
 
 // TODO: The Cfg structure, and instructions in particular, need to be
-// validated for things like valid operand types, valid branch
-// targets, proper ordering of Phi and non-Phi instructions, etc.
-// Most of the validity checking will be done in the bitcode reader.
-// We need a list of everything that should be validated, and tests
-// for each.
+// validated for things like valid operand types, valid branch targets, proper
+// ordering of Phi and non-Phi instructions, etc. Most of the validity
+// checking will be done in the bitcode reader. We need a list of everything
+// that should be validated, and tests for each.
 
 namespace Ice {
 
-/// Base instruction class for ICE.  Inst has two subclasses:
-/// InstHighLevel and InstTarget.  High-level ICE instructions inherit
-/// from InstHighLevel, and low-level (target-specific) ICE
-/// instructions inherit from InstTarget.
+/// Base instruction class for ICE. Inst has two subclasses: InstHighLevel and
+/// InstTarget. High-level ICE instructions inherit from InstHighLevel, and
+/// low-level (target-specific) ICE instructions inherit from InstTarget.
 class Inst : public llvm::ilist_node<Inst> {
   Inst() = delete;
   Inst(const Inst &) = delete;
@@ -68,12 +67,13 @@ public:
     FakeUse,      // not part of LLVM/PNaCl bitcode
     FakeKill,     // not part of LLVM/PNaCl bitcode
     JumpTable,    // not part of LLVM/PNaCl bitcode
-    Target        // target-specific low-level ICE
-                  // Anything >= Target is an InstTarget subclass.
-                  // Note that the value-spaces are shared across targets.
-                  // To avoid confusion over the definition of shared values,
-                  // an object specific to one target should never be passed
-                  // to a different target.
+    // target-specific low-level ICE Anything >= Target is an InstTarget

[Jim Stichnoth, 2015/09/04 21:32:48]

probably delete the "target-specific low-level ICE"

[ascull, 2015/09/05 00:00:48]

Done.

+    // subclass. Note that the value-spaces are shared across targets. To
+    // avoid confusion over the definition of shared values, an object specific
+    // to one target should never be passed to a different target.
+#define X(n) Target##n,
+    TARGETKINDS_INSTS_TABLE

[Jim Stichnoth, 2015/09/04 21:32:49]

IIRC, you are solving the problem by defining Target0, Target1, ..., Target100,
but you only actually use Target0, leaving the rest just as placeholders.

Could you instead do something like:
  JumpTable,
  Target,
  Target_Max = Target + 100
and then after the target instruction enum definition, static_assert that the
target enum max value is no greater than Target_Max?

[ascull, 2015/09/05 00:00:48]

I'm not sure I see what you mean here. You need to explicitly define the
placeholders to avoid UB. Or are you just suggesting to add Target_Max as a
value?

[Jim Stichnoth, 2015/09/05 01:03:22]

I believe the UB comes from trying to represent an enum value outside the enum's
min/max range, not about "holes" in the specified enum values.  For example,
consider this program:

#include <cstdio>

enum TestEnum {
  v10 = 10,
  v20 = 20,
  v30 = 30
};

void consume(TestEnum te) {
  printf("%d\n", te);
}

int main(int argc, char **argv) {
  TestEnum te;
  te = (TestEnum)5;
  consume(te);
  te = (TestEnum)10;
  consume(te);
  te = (TestEnum)15;
  consume(te);
  te = (TestEnum)25;
  consume(te);
  te = (TestEnum)30;
  consume(te);
  te = (TestEnum)35;
  consume(te);
  return 0;
}

UBSan complains about the 35, but not about 15 or 25, or, curiously, 5.

Also a stackoverflow question:
http://stackoverflow.com/questions/7676817/forcing-improper-enum-values

While we're at it, I think 100 may be too small for the number of
target-specific instructions.  Maybe just set the max enum value to 255?  (or (1
<< CHAR_BIT) - 1 if we're avoiding magic constants)?

[ascull, 2015/09/08 17:29:26]

Done.

+#undef X
   };
   InstKind getKind() const { return Kind; }
 
@@ -107,13 +107,13 @@ public:
   bool isLastUse(const Operand *Src) const;
   void spliceLivenessInfo(Inst *OrigInst, Inst *SpliceAssn);
 
-  /// Returns a list of out-edges corresponding to a terminator
-  /// instruction, which is the last instruction of the block.
-  /// The list must not contain duplicates.
+  /// Returns a list of out-edges corresponding to a terminator instruction,
+  /// which is the last instruction of the block. The list must not contain
+  /// duplicates.
   virtual NodeList getTerminatorEdges() const {
-    // All valid terminator instructions override this method.  For
-    // the default implementation, we assert in case some CfgNode
-    // is constructed without a terminator instruction at the end.
+    // All valid terminator instructions override this method. For the default
+    // implementation, we assert in case some CfgNode is constructed without a
+    // terminator instruction at the end.
     llvm_unreachable(
         "getTerminatorEdges() called on a non-terminator instruction");
     return NodeList();
@@ -131,19 +131,18 @@ public:
   virtual bool isSimpleAssign() const { return false; }
 
   void livenessLightweight(Cfg *Func, LivenessBV &Live);
-  /// Calculates liveness for this instruction.  Returns true if this
-  /// instruction is (tentatively) still live and should be retained,
-  /// and false if this instruction is (tentatively) dead and should be
-  /// deleted.  The decision is tentative until the liveness dataflow
-  /// algorithm has converged, and then a separate pass permanently
-  /// deletes dead instructions.
+  // Calculates liveness for this instruction.  Returns true if this
+  /// instruction is (tentatively) still live and should be retained, and false
+  /// if this instruction is (tentatively) dead and should be deleted. The
+  /// decision is tentative until the liveness dataflow algorithm has converged,
+  /// and then a separate pass permanently deletes dead instructions.
   bool liveness(InstNumberT InstNumber, LivenessBV &Live, Liveness *Liveness,
                 LiveBeginEndMap *LiveBegin, LiveBeginEndMap *LiveEnd);
 
-  /// Get the number of native instructions that this instruction
-  /// ultimately emits.  By default, high-level instructions don't
-  /// result in any native instructions, and a target-specific
-  /// instruction results in a single native instruction.
+  /// Get the number of native instructions that this instruction ultimately
+  /// emits. By default, high-level instructions don't result in any native
+  /// instructions, and a target-specific instruction results in a single native
+  /// instruction.
   virtual uint32_t getEmitInstCount() const { return 0; }
   // TODO(stichnot): Change Inst back to abstract once the g++ build
   // issue is fixed.  llvm::ilist<Ice::Inst> doesn't work under g++
@@ -954,12 +953,12 @@ class InstTarget : public Inst {
 public:
   uint32_t getEmitInstCount() const override { return 1; }
   void dump(const Cfg *Func) const override;
-  static bool classof(const Inst *Inst) { return Inst->getKind() >= Target; }
+  static bool classof(const Inst *Inst) { return Inst->getKind() >= Target0; }
 
 protected:
   InstTarget(Cfg *Func, InstKind Kind, SizeT MaxSrcs, Variable *Dest)
       : Inst(Func, Kind, MaxSrcs, Dest) {
-    assert(Kind >= Target);
+    assert(Kind >= Target0);
   }
 };