[llvm] r239428 - Make MCSymbol::Name be a union of uint64_t and a pointer.

[David Blaikie]

On Tue, Jun 9, 2015 at 12:56 PM, Pete Cooper <peter_cooper at apple.com> wrote:

> Author: pete
> Date: Tue Jun  9 14:56:05 2015
> New Revision: 239428
>
> URL: http://llvm.org/viewvc/llvm-project?rev=239428&view=rev
> Log:
> Make MCSymbol::Name be a union of uint64_t and a pointer.
>
> This should hopefully fix the 32-bit bots which were allocating space for
> a pointer
> but needed to be aligned to 64-bits.
>
> Now we allocate enough space for a uint64_t and a pointer and cast to the
> appropriate storage
>

This kind fo complicates the rest of the MCSymbol code. A few options:

have getNameEntry return the NameEntry reference as before (do the
".NameEntry" inside getNameEntry rather than in both the callers).

Possibly just do the interesting uint64_t stuff inside operator new -
though I suppose you'd still need some magic in getNameEntry that would
look similar.

& the alignment assertion could probably be a static_assert? (might need to
check if alignOf is constexpr or if we have a trait/constexpr form of it)


Oh, side note (should've caught this in review): you could have
getNameEntry non-const call getNameEntry const and just cast away the
constness on the resulting reference - to reduce some of that code
duplication.


>
> Modified:
>     llvm/trunk/include/llvm/MC/MCSymbol.h
>     llvm/trunk/lib/MC/MCSymbol.cpp
>
> Modified: llvm/trunk/include/llvm/MC/MCSymbol.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=239428&r1=239427&r2=239428&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/MC/MCSymbol.h (original)
> +++ llvm/trunk/include/llvm/MC/MCSymbol.h Tue Jun  9 14:56:05 2015
> @@ -120,20 +120,29 @@ protected: // MCContext creates and uniq
>    friend class MCExpr;
>    friend class MCContext;
>
> -  typedef const StringMapEntry<bool> NameEntryTy;
> -  MCSymbol(SymbolKind Kind, NameEntryTy *Name, bool isTemporary)
> +  /// \brief The name for a symbol.
> +  /// MCSymbol contains a uint64_t so is probably aligned to 8.  On a
> 32-bit
> +  /// system, the name is a pointer so isn't going to satisfy the 8 byte
> +  /// alignment of uint64_t.  Account for that here.
> +  typedef union {
> +    const StringMapEntry<bool> *NameEntry;
> +    uint64_t AlignmentPadding;
> +  } NameEntryStorageTy;
> +
> +  MCSymbol(SymbolKind Kind, const StringMapEntry<bool> *Name, bool
> isTemporary)
>        : Value(nullptr), IsTemporary(isTemporary),
>          IsRedefinable(false), IsUsed(false), IsRegistered(false),
>          IsExternal(false), IsPrivateExtern(false), HasName(!!Name),
>          Kind(Kind) {
>      Offset = 0;
>      if (Name)
> -      getNameEntryPtr() = Name;
> +      getNameEntryPtr().NameEntry = Name;
>    }
>
>    // Provide custom new/delete as we will only allocate space for a name
>    // if we need one.
> -  void *operator new(size_t s, NameEntryTy *Name, MCContext &Ctx);
> +  void *operator new(size_t s, const StringMapEntry<bool> *Name,
> +                     MCContext &Ctx);
>
>  private:
>
> @@ -160,14 +169,14 @@ private:
>    }
>
>    /// \brief Get a reference to the name field.  Requires that we have a
> name
> -  NameEntryTy *&getNameEntryPtr() {
> +  NameEntryStorageTy &getNameEntryPtr() {
>      assert(HasName && "Name is required");
> -    NameEntryTy **Name = reinterpret_cast<NameEntryTy **>(this);
> +    NameEntryStorageTy *Name = reinterpret_cast<NameEntryStorageTy
> *>(this);
>      return *(Name - 1);
>    }
> -  NameEntryTy *const &getNameEntryPtr() const {
> +  const NameEntryStorageTy &getNameEntryPtr() const {
>      assert(HasName && "Name is required");
> -    NameEntryTy *const *Name = reinterpret_cast<NameEntryTy *const
> *>(this);
> +    const auto *Name = reinterpret_cast<const NameEntryStorageTy *>(this);
>      return *(Name - 1);
>    }
>
> @@ -177,7 +186,7 @@ public:
>      if (!HasName)
>        return StringRef();
>
> -    return getNameEntryPtr()->first();
> +    return getNameEntryPtr().NameEntry->first();
>    }
>
>    bool isRegistered() const { return IsRegistered; }
>
> Modified: llvm/trunk/lib/MC/MCSymbol.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=239428&r1=239427&r2=239428&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/MC/MCSymbol.cpp (original)
> +++ llvm/trunk/lib/MC/MCSymbol.cpp Tue Jun  9 14:56:05 2015
> @@ -19,17 +19,20 @@ using namespace llvm;
>  // Sentinel value for the absolute pseudo section.
>  MCSection *MCSymbol::AbsolutePseudoSection = reinterpret_cast<MCSection
> *>(1);
>
> -void *MCSymbol::operator new(size_t s, NameEntryTy *Name, MCContext &Ctx)
> {
> -  size_t Size = s + (Name ? sizeof(Name) : 0);
> +void *MCSymbol::operator new(size_t s, const StringMapEntry<bool> *Name,
> +                             MCContext &Ctx) {
> +  // We may need more space for a Name to account for alignment.  So
> allocate
> +  // space for the storage type and not the name pointer.
> +  size_t Size = s + (Name ? sizeof(NameEntryStorageTy) : 0);
>
>    // For safety, ensure that the alignment of a pointer is enough for an
>    // MCSymbol.  This also ensures we don't need padding between the name
> and
>    // symbol.
> -  assert(alignOf<MCSymbol>() <= alignOf<NameEntryTy *>() &&
> +  assert(alignOf<MCSymbol>() <= alignOf<NameEntryStorageTy>() &&
>           "Bad alignment of MCSymbol");
> -  void *Storage = Ctx.allocate(Size, alignOf<NameEntryTy *>());
> -  NameEntryTy **Start = static_cast<NameEntryTy**>(Storage);
> -  NameEntryTy **End = Start + (Name ? 1 : 0);
> +  void *Storage = Ctx.allocate(Size, alignOf<NameEntryStorageTy>());
> +  NameEntryStorageTy *Start = static_cast<NameEntryStorageTy*>(Storage);
> +  NameEntryStorageTy *End = Start + (Name ? 1 : 0);
>    return End;
>  }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150609/7fdad36a/attachment.html>