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

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

[jyknight]

include/llvm/Support/AlignOf.h

56–57

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

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

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

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

[jyknight]

include/llvm/Support/AlignOf.h

56–57

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

SGTM, will change it.

411–412

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

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;
   }
 };
-
+// Assert objects tacked on the end of FunctionType won't be misaligned
+static_assert(AlignOf<FunctionType>::Alignment >= AlignOf<Type *>::Alignment,
+              "");
 
 /// 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:
 

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 };

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);
   }
 };
+// Assert objects tacked on the end of AttributeSetNode won't be misaligned
+static_assert(AlignOf<AttributeSetNode>::Alignment >=
+                  AlignOf<Attribute>::Alignment,
+              "");
 
 //===----------------------------------------------------------------------===//
 /// \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;
 };
+// Assert objects tacked on the end of AttributeSetImpl won't be misaligned
+static_assert(AlignOf<AttributeSetImpl>::Alignment >=
+                  AlignOf<AttributeSetImpl::IndexAttrPair>::Alignment,
+              "");
 
 } // end llvm namespace
 
 #endif

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,                           \
+                "");
+#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)
+  size_t AlignOffset = OffsetToAlignment(OpSize, llvm::alignOf<uint64_t>());
+  void *Ptr =
+      reinterpret_cast<char *>(::operator new(OpSize + AlignOffset + Size)) +
+      AlignOffset;
   MDOperand *O = static_cast<MDOperand *>(Ptr);
   for (MDOperand *E = O + NumOps; O != E; ++O)
     (void)new (O) MDOperand;
   return O;
 }
 
 void MDNode::operator delete(void *Mem) {
   MDNode *N = static_cast<MDNode *>(Mem);
-  MDOperand *O = static_cast<MDOperand *>(Mem);
+  size_t OpSize = N->NumOperands * sizeof(MDOperand);
+  size_t AlignOffset = OffsetToAlignment(OpSize, llvm::alignOf<uint64_t>());
+
+  MDOperand *O =
+      reinterpret_cast<MDOperand *>(reinterpret_cast<uintptr_t>(Mem));
   for (MDOperand *E = O - N->NumOperands; O != E; --O)
     (O - 1)->~MDOperand();
-  ::operator delete(O);
+  ::operator delete(reinterpret_cast<char *>(O) - AlignOffset);
 }
 
 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)