Fix alignment issues in LLVM.

[jyknight]

Summary

Adds static_asserts to ensure alignment of concatenated objects is
correct, and fixes them where they are not.

Also changes the definition of AlignOf to use constexpr, except on
MSVC, to avoid enum comparison warnings from GCC.

(There's not too much of this in llvm itself, most of the fun is in
clang).

This seems to make LLVM actually work without Bus Error on 32bit
sparc.

[majnemer]

include/llvm/Support/AlignOf.h

56–57 ↗

(On Diff #27178)

Why not just do enum : unsigned { Alignment = ... ; ?

[jyknight]

include/llvm/Support/AlignOf.h

56–57 ↗

(On Diff #27178)

I hadn't tried until you suggested, but the answer is: because GCC still warns about it.

[jyknight]

Ping? If there's no objection, I'd like to submit this.

[rnk]

BTW, Duncan recently rewrote this so I'll add him.

include/llvm/Support/AlignOf.h

56–57 ↗

(On Diff #27178)

A static constexpr int still sometimes requires an out of class definition if you accidentally take the address of the global by passing it to a reference-taking function like std::max, which is a pretty common operation for alignments.

I'd rather add casts in the assertions or disable gcc's warning. Seem reasonable?

lib/IR/Metadata.cpp

393–396 ↗

(On Diff #27178)

I feel like a more intuitive way to phrase this computation is to overallocate like so:

size_t OpSize = NumOps * sizeof(MDOperand);
OpSize = RoundUpToAlignment(OpSize, llvm::alignOf<uint64_t>());

The placement new operations can use the same negative indexing approach as operator delete, and we never have to consider AlignOffset as a separate quantity.

411–412 ↗

(On Diff #27178)

What was wrong with the old code here? A strict aliasing warning?

[jyknight]

include/llvm/Support/AlignOf.h

56–57 ↗

(On Diff #27178)

Adding casts everywhere would be pretty annoying. Adding -Wno-enum-compare to the compile flags seems okay, but somewhat unfortunate, as it can usually expose real bugs.

How about just adding a couple lines to the header:

template <typename T>
constexpr unsigned AlignOf<T>::Alignment;

?

lib/IR/Metadata.cpp

393–396 ↗

(On Diff #27178)

SGTM, will change it.

411–412 ↗

(On Diff #27178)

It's fine back the way it was before; I think I must've added some arithmetic in there in a prior revision.

[jyknight]

Updated per review comments from Duncan and Reid.

[rnk]

lgtm

llvm/trunk/include/llvm/IR/DerivedTypes.h

public:
   ///
   unsigned getNumParams() const { return NumContainedTys - 1; }
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast.
   static inline bool classof(const Type *T) {
     return T->getTypeID() == FunctionTyID;
   }
 };
-
+static_assert(AlignOf<FunctionType>::Alignment >= AlignOf<Type *>::Alignment,
+              "Alignment sufficient for objects appended to FunctionType");
 
 /// CompositeType - Common super class of ArrayType, StructType, PointerType
 /// and VectorType.
 class CompositeType : public Type {
 protected:
   explicit CompositeType(LLVMContext &C, TypeID tid) : Type(C, tid) { }
 public:
 

llvm/trunk/include/llvm/IR/User.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_USER_H
 #define LLVM_IR_USER_H
 
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/IR/Value.h"
+#include "llvm/Support/AlignOf.h"
 #include "llvm/Support/ErrorHandling.h"
 
 namespace llvm {
 
 /// \brief Compile-time customization of User operands.
 ///
 /// Customizes operand-related allocators and accessors.
 template <class>
public:
   /// "To" definition.
   void replaceUsesOfWith(Value *From, Value *To);
 
   // Methods for support type inquiry through isa, cast, and dyn_cast:
   static inline bool classof(const Value *V) {
     return isa<Instruction>(V) || isa<Constant>(V);
   }
 };
+// Either Use objects, or a Use pointer can be prepended to User.
+static_assert(AlignOf<Use>::Alignment >= AlignOf<User>::Alignment,
+              "Alignment sufficient after objects prepended to User");
+static_assert(AlignOf<Use *>::Alignment >= AlignOf<User>::Alignment,
+              "Alignment sufficient after objects prepended to User");
 
 template<> struct simplify_type<User::op_iterator> {
   typedef Value* SimpleType;
   static SimpleType getSimplifiedValue(User::op_iterator &Val) {
     return Val->get();
   }
 };
 template<> struct simplify_type<User::const_op_iterator> {

llvm/trunk/include/llvm/Support/AlignOf.h

 ///  the alignment of the template argument.  For example,
 ///  AlignOf<int>::Alignment represents the alignment of type "int".  The
 ///  alignment calculated is the minimum alignment, and not necessarily
 ///  the "desired" alignment returned by GCC's __alignof__ (for example).  Note
 ///  that because the alignment is an enum value, it can be used as a
 ///  compile-time constant (e.g., for template instantiation).
 template <typename T>
 struct AlignOf {
+#ifndef _MSC_VER
+  // Avoid warnings from GCC like:
+  //   comparison between 'enum llvm::AlignOf<X>::<anonymous>' and 'enum
+  //   llvm::AlignOf<Y>::<anonymous>' [-Wenum-compare]
+  // by using constexpr instead of enum.
+  // (except on MSVC, since it doesn't support constexpr yet).
+  static constexpr unsigned Alignment =
+      static_cast<unsigned int>(sizeof(AlignmentCalcImpl<T>) - sizeof(T));
+#else
   enum { Alignment =
          static_cast<unsigned int>(sizeof(AlignmentCalcImpl<T>) - sizeof(T)) };
-
+#endif
   enum { Alignment_GreaterEqual_2Bytes = Alignment >= 2 ? 1 : 0 };
   enum { Alignment_GreaterEqual_4Bytes = Alignment >= 4 ? 1 : 0 };
   enum { Alignment_GreaterEqual_8Bytes = Alignment >= 8 ? 1 : 0 };
   enum { Alignment_GreaterEqual_16Bytes = Alignment >= 16 ? 1 : 0 };
 
   enum { Alignment_LessEqual_2Bytes = Alignment <= 2 ? 1 : 0 };
   enum { Alignment_LessEqual_4Bytes = Alignment <= 4 ? 1 : 0 };
   enum { Alignment_LessEqual_8Bytes = Alignment <= 8 ? 1 : 0 };
   enum { Alignment_LessEqual_16Bytes = Alignment <= 16 ? 1 : 0 };
 };
 
+#ifndef _MSC_VER
+template <typename T> constexpr unsigned AlignOf<T>::Alignment;
+#endif
+
 /// alignOf - A templated function that returns the minimum alignment of
 ///  of a type.  This provides no extra functionality beyond the AlignOf
 ///  class besides some cosmetic cleanliness.  Example usage:
 ///  alignOf<int>() returns the alignment of an int.
 template <typename T>
 inline unsigned alignOf() { return AlignOf<T>::Alignment; }
 
 /// \struct AlignedCharArray

llvm/trunk/lib/IR/AttributeImpl.h

public:
   void Profile(FoldingSetNodeID &ID) const {
     Profile(ID, makeArrayRef(begin(), end()));
   }
   static void Profile(FoldingSetNodeID &ID, ArrayRef<Attribute> AttrList) {
     for (unsigned I = 0, E = AttrList.size(); I != E; ++I)
       AttrList[I].Profile(ID);
   }
 };
+static_assert(AlignOf<AttributeSetNode>::Alignment >=
+                  AlignOf<Attribute>::Alignment,
+              "Alignment sufficient for objects appended to AttributeSetNode");
 
 //===----------------------------------------------------------------------===//
 /// \class
 /// \brief This class represents a set of attributes that apply to the function,
 /// return type, and parameters.
 class AttributeSetImpl : public FoldingSetNode {
   friend class AttributeSet;
 
-  LLVMContext &Context;
-
+public:
   typedef std::pair<unsigned, AttributeSetNode*> IndexAttrPair;
+
+private:
+  LLVMContext &Context;
   unsigned NumAttrs; ///< Number of entries in this set.
 
   /// \brief Return a pointer to the IndexAttrPair for the specified slot.
   const IndexAttrPair *getNode(unsigned Slot) const {
     return reinterpret_cast<const IndexAttrPair *>(this + 1) + Slot;
   }
 
   // AttributesSet is uniqued, these should not be publicly available.
   void operator=(const AttributeSetImpl &) = delete;
   AttributeSetImpl(const AttributeSetImpl &) = delete;
 public:
   AttributeSetImpl(LLVMContext &C,
                    ArrayRef<std::pair<unsigned, AttributeSetNode *> > Attrs)
       : Context(C), NumAttrs(Attrs.size()) {
+
 #ifndef NDEBUG
     if (Attrs.size() >= 2) {
       for (const std::pair<unsigned, AttributeSetNode *> *i = Attrs.begin() + 1,
                                                          *e = Attrs.end();
            i != e; ++i) {
         assert((i-1)->first <= i->first && "Attribute set not ordered!");
       }
     }
static void Profile(FoldingSetNodeID &ID,
     }
   }
 
   // FIXME: This atrocity is temporary.
   uint64_t Raw(unsigned Index) const;
 
   void dump() const;
 };
+static_assert(AlignOf<AttributeSetImpl>::Alignment >=
+                  AlignOf<AttributeSetImpl::IndexAttrPair>::Alignment,
+              "Alignment sufficient for objects appended to AttributeSetImpl");
 
 } // end llvm namespace
 
 #endif

llvm/trunk/lib/IR/Metadata.cpp

StringRef MDString::getString() const {
   assert(Entry && "Expected to find string map entry");
   return Entry->first();
 }
 
 //===----------------------------------------------------------------------===//
 // MDNode implementation.
 //
 
+// Assert that the MDNode types will not be unaligned by the objects
+// prepended to them.
+#define HANDLE_MDNODE_LEAF(CLASS)                                              \
+  static_assert(llvm::AlignOf<uint64_t>::Alignment >=                          \
+                    llvm::AlignOf<CLASS>::Alignment,                           \
+                "Alignment sufficient after objects prepended to " #CLASS);
+#include "llvm/IR/Metadata.def"
+
 void *MDNode::operator new(size_t Size, unsigned NumOps) {
-  void *Ptr = ::operator new(Size + NumOps * sizeof(MDOperand));
+  size_t OpSize = NumOps * sizeof(MDOperand);
+  // uint64_t is the most aligned type we need support (ensured by static_assert
+  // above)
+  OpSize = RoundUpToAlignment(OpSize, llvm::alignOf<uint64_t>());
+  void *Ptr = reinterpret_cast<char *>(::operator new(OpSize + Size)) + OpSize;
   MDOperand *O = static_cast<MDOperand *>(Ptr);
-  for (MDOperand *E = O + NumOps; O != E; ++O)
-    (void)new (O) MDOperand;
-  return O;
+  for (MDOperand *E = O - NumOps; O != E; --O)
+    (void)new (O - 1) MDOperand;
+  return Ptr;
 }
 
 void MDNode::operator delete(void *Mem) {
   MDNode *N = static_cast<MDNode *>(Mem);
+  size_t OpSize = N->NumOperands * sizeof(MDOperand);
+  OpSize = RoundUpToAlignment(OpSize, llvm::alignOf<uint64_t>());
+
   MDOperand *O = static_cast<MDOperand *>(Mem);
   for (MDOperand *E = O - N->NumOperands; O != E; --O)
     (O - 1)->~MDOperand();
-  ::operator delete(O);
+  ::operator delete(reinterpret_cast<char *>(Mem) - OpSize);
 }
 
 MDNode::MDNode(LLVMContext &Context, unsigned ID, StorageType Storage,
                ArrayRef<Metadata *> Ops1, ArrayRef<Metadata *> Ops2)
     : Metadata(ID, Storage), NumOperands(Ops1.size() + Ops2.size()),
       NumUnresolved(0), Context(Context) {
   unsigned Op = 0;
   for (Metadata *MD : Ops1)

llvm/trunk/lib/IR/User.cpp

 }
 
 //===----------------------------------------------------------------------===//
 //                         User allocHungoffUses Implementation
 //===----------------------------------------------------------------------===//
 
 void User::allocHungoffUses(unsigned N, bool IsPhi) {
   assert(HasHungOffUses && "alloc must have hung off uses");
+
+  static_assert(AlignOf<Use>::Alignment >= AlignOf<Use::UserRef>::Alignment,
+                "Alignment sufficient for hung-off-uses pieces");
+  static_assert(AlignOf<Use::UserRef>::Alignment >= AlignOf<BasicBlock *>::Alignment,
+                "Alignment sufficient for hung-off-uses pieces");
+
   // Allocate the array of Uses, followed by a pointer (with bottom bit set) to
   // the User.
   size_t size = N * sizeof(Use) + sizeof(Use::UserRef);
   if (IsPhi)
     size += N * sizeof(BasicBlock *);
   Use *Begin = static_cast<Use*>(::operator new(size));
   Use *End = Begin + N;
   (void) new(End) Use::UserRef(const_cast<User*>(this), 1);