Add UBSAN build option and fix undefined behaviour errors.

Makefile.standalone

@@ -96,6 +96,18 @@ else
   OBJDIR := $(OBJDIR)+Asserts
 endif
 
+ifdef UBSAN
+  OBJDIR := $(OBJDIR)+UBSan
+  CXX_EXTRA += -fsanitize=undefined -fno-sanitize=vptr -fno-sanitize=nonnull-attribute
+  LD_EXTRA += -fsanitize=undefined
+endif
+
+ifdef UBSAN_TRAP
+  OBJDIR := $(OBJDIR)+UBSan_Trap
+  CXX_EXTRA += -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error -fno-sanitize=vptr -fno-sanitize=nonnull-attribute
+  LD_EXTRA += -fsanitize=undefined-trap
+endif
+
 ifdef TSAN
   OBJDIR := $(OBJDIR)+TSan
   CXX_EXTRA += -fsanitize=thread
@@ -108,6 +120,12 @@ ifdef ASAN
   LD_EXTRA += -fsanitize=address
 endif
 
+ifdef MSAN
+  OBJDIR := $(OBJDIR)+MSan
+  CXX_EXTRA += -fsanitize=memory
+  LD_EXTRA += -fsanitize=memory
+endif
+
 SB_OBJDIR := $(OBJDIR)+Sandboxed
 
 $(info -----------------------------------------------)

src/IceAssembler.h

@@ -97,24 +97,31 @@ public:
   AssemblerBuffer(Assembler &);
   ~AssemblerBuffer();
 
-  /// Basic support for emitting, loading, and storing.
+  /// \name Basic support for emitting, loading, and storing.
+  /// @{
+  // These use memcpy instead of assignment to avoid undefined behaviour of
+  // assigning to unaligned addresses. Since the size of the copy is known the
+  // compiler can inline the memcpy with simple moves.
   template <typename T> void emit(T Value) {
     assert(hasEnsuredCapacity());
-    *reinterpret_cast<T *>(Cursor) = Value;
+    memcpy(reinterpret_cast<void *>(Cursor), &Value, sizeof(T));
     Cursor += sizeof(T);
   }
 
   template <typename T> T load(intptr_t Position) const {
     assert(Position >= 0 &&
            Position <= (size() - static_cast<intptr_t>(sizeof(T))));
-    return *reinterpret_cast<T *>(Contents + Position);
+    T Value;
+    memcpy(&Value, reinterpret_cast<void *>(Contents + Position), sizeof(T));
+    return Value;
   }
 
   template <typename T> void store(intptr_t Position, T Value) {
     assert(Position >= 0 &&
            Position <= (size() - static_cast<intptr_t>(sizeof(T))));
-    *reinterpret_cast<T *>(Contents + Position) = Value;
+    memcpy(reinterpret_cast<void *>(Contents + Position), &Value, sizeof(T));
   }
+  /// @{
 
   /// Emit a fixup at the current location.
   void emitFixup(AssemblerFixup *Fixup) { Fixup->set_position(size()); }

src/IceConverter.cpp

@@ -745,7 +745,7 @@ LLVM2ICEGlobalsConverter::convertGlobalsToIce(Module *Mod) {
       addGlobalInitializer(*VarDecl, Initializer);
     }
   }
-  return std::move(VariableDeclarations);
+  return VariableDeclarations;
 }
 
 void LLVM2ICEGlobalsConverter::addGlobalInitializer(

src/IceInst.h

@@ -24,18 +24,16 @@
 #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,13 +66,14 @@ 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.
+    // 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,
+    Target_Max = std::numeric_limits<uint8_t>::max(),
   };
+  static_assert(Target <= Target_Max, "Must not be above max.");
   InstKind getKind() const { return Kind; }
 
   InstNumberT getNumber() const { return Number; }
@@ -107,13 +106,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 +130,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++
@@ -960,6 +958,7 @@ protected:
   InstTarget(Cfg *Func, InstKind Kind, SizeT MaxSrcs, Variable *Dest)
       : Inst(Func, Kind, MaxSrcs, Dest) {
     assert(Kind >= Target);
+    assert(Kind <= Target_Max);
   }
 };
 

src/IceOperand.h

@@ -8,11 +8,10 @@
 //===----------------------------------------------------------------------===//
 ///
 /// \file
-/// This file declares the Operand class and its target-independent
-/// subclasses.  The main classes are Variable, which represents an
-/// LLVM variable that is either register- or stack-allocated, and the
-/// Constant hierarchy, which represents integer, floating-point,
-/// and/or symbolic constants.
+/// This file declares the Operand class and its target-independent subclasses.
+/// The main classes are Variable, which represents an LLVM variable that is
+/// either register- or stack-allocated, and the Constant hierarchy, which
+/// represents integer, floating-point, and/or symbolic constants.
 ///
 //===----------------------------------------------------------------------===//
 
@@ -32,7 +31,7 @@ class Operand {
   Operand &operator=(const Operand &) = delete;
 
 public:
-  static const size_t MaxTargetKinds = 10;
+  static constexpr size_t MaxTargetKinds = 10;
   enum OperandKind {
     kConst_Base,
     kConstInteger32,
@@ -42,24 +41,25 @@ public:
     kConstRelocatable,
     kConstUndef,
     kConst_Target, // leave space for target-specific constant kinds
-    kConst_Num = kConst_Target + MaxTargetKinds,
+    kConst_Max = kConst_Target + MaxTargetKinds,
     kVariable,
     kVariable_Target, // leave space for target-specific variable kinds
-    kVariable_Num = kVariable_Target + MaxTargetKinds,
+    kVariable_Max = kVariable_Target + MaxTargetKinds,
     // Target-specific operand classes use kTarget as the starting
     // point for their Kind enum space. 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.
-    kTarget
+    kTarget,
+    kTarget_Max = std::numeric_limits<uint8_t>::max(),
   };
+  static_assert(kTarget <= kTarget_Max, "Must not be above max.");
   OperandKind getKind() const { return Kind; }
   Type getType() const { return Ty; }
 
-  /// Every Operand keeps an array of the Variables referenced in
-  /// the operand.  This is so that the liveness operations can get
-  /// quick access to the variables of interest, without having to dig
-  /// so far into the operand.
+  /// Every Operand keeps an array of the Variables referenced in the operand.
+  /// This is so that the liveness operations can get quick access to the
+  /// variables of interest, without having to dig so far into the operand.
   SizeT getNumVars() const { return NumVars; }
   Variable *getVar(SizeT I) const {
     assert(I < getNumVars());
@@ -86,7 +86,10 @@ public:
   /// @}
 
 protected:
-  Operand(OperandKind Kind, Type Ty) : Ty(Ty), Kind(Kind) {}
+  Operand(OperandKind Kind, Type Ty) : Ty(Ty), Kind(Kind) {
+    // It is undefined behavior to have a larger value in the enum
+    assert(Kind <= kTarget_Max);
+  }
   virtual ~Operand() = default;
 
   const Type Ty;
@@ -118,7 +121,7 @@ public:
 
   static bool classof(const Operand *Operand) {
     OperandKind Kind = Operand->getKind();
-    return Kind >= kConst_Base && Kind <= kConst_Num;
+    return Kind >= kConst_Base && Kind <= kConst_Max;
   }
 
   /// Judge if this given immediate should be randomized or pooled
@@ -508,7 +511,7 @@ public:
 
   static bool classof(const Operand *Operand) {
     OperandKind Kind = Operand->getKind();
-    return Kind >= kVariable && Kind <= kVariable_Num;
+    return Kind >= kVariable && Kind <= kVariable_Max;
   }
 
 protected:

src/IceRNG.cpp

@@ -14,7 +14,7 @@
 
 #include "IceRNG.h"
 
-#include <time.h>
+#include <ctime>
 
 namespace Ice {