<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 9, 2015 at 1:52 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Thanks for the feedback. Comments below, based on r239429 which i just pushed.<br><div><span class=""><blockquote type="cite"><div>On Jun 9, 2015, at 1:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 9, 2015 at 12:56 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: pete<br>
Date: Tue Jun 9 14:56:05 2015<br>
New Revision: 239428<br>
<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D239428-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v9JIVEbbFzgNuDrqHfL-mSlXo9hU8grzQ0PrVTTsb0k&s=IhaJkyOTk4jIxaMVRAxF_A2L1i0wbrztHPJfTzRoEss&e=" target="_blank">http://llvm.org/viewvc/llvm-project?rev=239428&view=rev</a><br>
Log:<br>
Make MCSymbol::Name be a union of uint64_t and a pointer.<br>
<br>
This should hopefully fix the 32-bit bots which were allocating space for a pointer<br>
but needed to be aligned to 64-bits.<br>
<br>
Now we allocate enough space for a uint64_t and a pointer and cast to the appropriate storage<br></blockquote><div><br>This kind fo complicates the rest of the MCSymbol code. A few options:<br><br>have getNameEntry return the NameEntry reference as before (do the ".NameEntry" inside getNameEntry rather than in both the callers).<br></div></div></div></div></div></blockquote></span>This is done.<span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>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.<br></div></div></div></div></div></blockquote></span>I thought about this, but because it was handled in getNameEntry() too, I went for the union solution so that at least everything is based on the union size and storage.<span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>& 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)<br></div></div></div></div></div></blockquote></span>Turns out constexpr is still not used much in LLVM. We have a macro for it. I guess MSVC doesn’t support it fully on our version. I’ve added a FIXME so that we can handle this later.</div></div></blockquote><div><br>Seems we have at least one example of a static assertion for alignment:<br><br>include/llvm/Support/ArrayRecycler.h: static_assert(Align >= AlignOf<FreeList>::Alignment, "Object underaligned");<br><br>So if you use the llvm::AlignOf trait instead of llvm::alignOf function, you should be able to make it a static instead of a dynamic assertion. Maybe?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.<br></div></div></div></div></div></blockquote></span>I’ve done this. I had to cast away the const-ness and make the const method call the non-const one. This was due to the const-ness of the result. Basically, this solution is a single const_cast, but the other way would have been 2.</div><div><br></div><div>Thanks for the help!</div><div><span class=""><font color="#888888">Pete</font></span><div><div class="h5"><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Modified:<br>
llvm/trunk/include/llvm/MC/MCSymbol.h<br>
llvm/trunk/lib/MC/MCSymbol.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/MC/MCSymbol.h<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_include_llvm_MC_MCSymbol.h-3Frev-3D239428-26r1-3D239427-26r2-3D239428-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v9JIVEbbFzgNuDrqHfL-mSlXo9hU8grzQ0PrVTTsb0k&s=lpLvFUj6NyD3k7OKeo_LGH3C5tkK8XcQVARNB6fvJY0&e=" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=239428&r1=239427&r2=239428&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/MC/MCSymbol.h (original)<br>
+++ llvm/trunk/include/llvm/MC/MCSymbol.h Tue Jun 9 14:56:05 2015<br>
@@ -120,20 +120,29 @@ protected: // MCContext creates and uniq<br>
friend class MCExpr;<br>
friend class MCContext;<br>
<br>
- typedef const StringMapEntry<bool> NameEntryTy;<br>
- MCSymbol(SymbolKind Kind, NameEntryTy *Name, bool isTemporary)<br>
+ /// \brief The name for a symbol.<br>
+ /// MCSymbol contains a uint64_t so is probably aligned to 8. On a 32-bit<br>
+ /// system, the name is a pointer so isn't going to satisfy the 8 byte<br>
+ /// alignment of uint64_t. Account for that here.<br>
+ typedef union {<br>
+ const StringMapEntry<bool> *NameEntry;<br>
+ uint64_t AlignmentPadding;<br>
+ } NameEntryStorageTy;<br>
+<br>
+ MCSymbol(SymbolKind Kind, const StringMapEntry<bool> *Name, bool isTemporary)<br>
: Value(nullptr), IsTemporary(isTemporary),<br>
IsRedefinable(false), IsUsed(false), IsRegistered(false),<br>
IsExternal(false), IsPrivateExtern(false), HasName(!!Name),<br>
Kind(Kind) {<br>
Offset = 0;<br>
if (Name)<br>
- getNameEntryPtr() = Name;<br>
+ getNameEntryPtr().NameEntry = Name;<br>
}<br>
<br>
// Provide custom new/delete as we will only allocate space for a name<br>
// if we need one.<br>
- void *operator new(size_t s, NameEntryTy *Name, MCContext &Ctx);<br>
+ void *operator new(size_t s, const StringMapEntry<bool> *Name,<br>
+ MCContext &Ctx);<br>
<br>
private:<br>
<br>
@@ -160,14 +169,14 @@ private:<br>
}<br>
<br>
/// \brief Get a reference to the name field. Requires that we have a name<br>
- NameEntryTy *&getNameEntryPtr() {<br>
+ NameEntryStorageTy &getNameEntryPtr() {<br>
assert(HasName && "Name is required");<br>
- NameEntryTy **Name = reinterpret_cast<NameEntryTy **>(this);<br>
+ NameEntryStorageTy *Name = reinterpret_cast<NameEntryStorageTy *>(this);<br>
return *(Name - 1);<br>
}<br>
- NameEntryTy *const &getNameEntryPtr() const {<br>
+ const NameEntryStorageTy &getNameEntryPtr() const {<br>
assert(HasName && "Name is required");<br>
- NameEntryTy *const *Name = reinterpret_cast<NameEntryTy *const *>(this);<br>
+ const auto *Name = reinterpret_cast<const NameEntryStorageTy *>(this);<br>
return *(Name - 1);<br>
}<br>
<br>
@@ -177,7 +186,7 @@ public:<br>
if (!HasName)<br>
return StringRef();<br>
<br>
- return getNameEntryPtr()->first();<br>
+ return getNameEntryPtr().NameEntry->first();<br>
}<br>
<br>
bool isRegistered() const { return IsRegistered; }<br>
<br>
Modified: llvm/trunk/lib/MC/MCSymbol.cpp<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_MC_MCSymbol.cpp-3Frev-3D239428-26r1-3D239427-26r2-3D239428-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v9JIVEbbFzgNuDrqHfL-mSlXo9hU8grzQ0PrVTTsb0k&s=H6IaNgXAxm0S11HZUmIj7mSCi8-6nuVF1BGZCZ8Sc2Y&e=" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=239428&r1=239427&r2=239428&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/MC/MCSymbol.cpp (original)<br>
+++ llvm/trunk/lib/MC/MCSymbol.cpp Tue Jun 9 14:56:05 2015<br>
@@ -19,17 +19,20 @@ using namespace llvm;<br>
// Sentinel value for the absolute pseudo section.<br>
MCSection *MCSymbol::AbsolutePseudoSection = reinterpret_cast<MCSection *>(1);<br>
<br>
-void *MCSymbol::operator new(size_t s, NameEntryTy *Name, MCContext &Ctx) {<br>
- size_t Size = s + (Name ? sizeof(Name) : 0);<br>
+void *MCSymbol::operator new(size_t s, const StringMapEntry<bool> *Name,<br>
+ MCContext &Ctx) {<br>
+ // We may need more space for a Name to account for alignment. So allocate<br>
+ // space for the storage type and not the name pointer.<br>
+ size_t Size = s + (Name ? sizeof(NameEntryStorageTy) : 0);<br>
<br>
// For safety, ensure that the alignment of a pointer is enough for an<br>
// MCSymbol. This also ensures we don't need padding between the name and<br>
// symbol.<br>
- assert(alignOf<MCSymbol>() <= alignOf<NameEntryTy *>() &&<br>
+ assert(alignOf<MCSymbol>() <= alignOf<NameEntryStorageTy>() &&<br>
"Bad alignment of MCSymbol");<br>
- void *Storage = Ctx.allocate(Size, alignOf<NameEntryTy *>());<br>
- NameEntryTy **Start = static_cast<NameEntryTy**>(Storage);<br>
- NameEntryTy **End = Start + (Name ? 1 : 0);<br>
+ void *Storage = Ctx.allocate(Size, alignOf<NameEntryStorageTy>());<br>
+ NameEntryStorageTy *Start = static_cast<NameEntryStorageTy*>(Storage);<br>
+ NameEntryStorageTy *End = Start + (Name ? 1 : 0);<br>
return End;<br>
}<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>