Add UBSAN build option and fix undefined behaviour errors.

src/IceInst.h

 //===- subzero/src/IceInst.h - High-level instructions ----------*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file declares the Inst class and its target-independent
 /// subclasses, which represent the high-level Vanilla ICE instructions
 /// and map roughly 1:1 to LLVM instructions.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef SUBZERO_SRC_ICEINST_H
 #define SUBZERO_SRC_ICEINST_H
 
 #include "IceCfg.h"
 #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;
   Inst &operator=(const Inst &) = delete;
 
 public:
   enum InstKind {
     // Arbitrary (alphabetical) order, except put Unreachable first.
     Unreachable,
     Alloca,
@@ skipping 12 matching lines @@
     Select,
     Store,
     Switch,
     Assign,       // not part of LLVM/PNaCl bitcode
     BundleLock,   // not part of LLVM/PNaCl bitcode
     BundleUnlock, // not part of LLVM/PNaCl bitcode
     FakeDef,      // not part of LLVM/PNaCl bitcode
     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; }
 
   InstNumberT getNumber() const { return Number; }
   void renumber(Cfg *Func);
   enum {
     NumberDeleted = -1,
     NumberSentinel = 0,
     NumberInitial = 2,
     NumberExtended = NumberInitial - 1
@@ skipping 13 matching lines @@
 
   SizeT getSrcSize() const { return NumSrcs; }
   Operand *getSrc(SizeT I) const {
     assert(I < getSrcSize());
     return Srcs[I];
   }
 
   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();
   }
   virtual bool isUnconditionalBranch() const { return false; }
   /// If the instruction is a branch-type instruction with OldNode as a
   /// target, repoint it to NewNode and return true, otherwise return
   /// false. Repoint all instances of OldNode as a target.
   virtual bool repointEdges(CfgNode *OldNode, CfgNode *NewNode) {
     (void)OldNode;
     (void)NewNode;
     return false;
   }
 
   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++
   // because the resize(size_t, Ice::Inst) method is incorrectly
   // declared and thus doesn't allow the abstract class Ice::Inst.
   // The method should be declared resize(size_t, const Ice::Inst &).
   // virtual void emit(const Cfg *Func) const = 0;
   // virtual void emitIAS(const Cfg *Func) const = 0;
   virtual void emit(const Cfg *) const {
     llvm_unreachable("emit on abstract class");
@@ skipping 790 matching lines @@
 /// The Target instruction is the base class for all target-specific
 /// instructions.
 class InstTarget : public Inst {
   InstTarget() = delete;
   InstTarget(const InstTarget &) = delete;
   InstTarget &operator=(const InstTarget &) = delete;
 
 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);
   }
 };
 
 bool checkForRedundantAssign(const Variable *Dest, const Operand *Source);
 
 } // end of namespace Ice
 
 namespace llvm {
 
 /// Override the default ilist traits so that Inst's private ctor and
 /// deleted dtor aren't invoked.
 template <>
 struct ilist_traits<Ice::Inst> : public ilist_default_traits<Ice::Inst> {
   Ice::Inst *createSentinel() const {
     return static_cast<Ice::Inst *>(&Sentinel);
   }
   static void destroySentinel(Ice::Inst *) {}
   Ice::Inst *provideInitialHead() const { return createSentinel(); }
   Ice::Inst *ensureHead(Ice::Inst *) const { return createSentinel(); }
   static void noteHead(Ice::Inst *, Ice::Inst *) {}
   void deleteNode(Ice::Inst *) {}
 
 private:
   mutable ilist_half_node<Ice::Inst> Sentinel;
 };
 
 } // end of namespace llvm
 
 #endif // SUBZERO_SRC_ICEINST_H