<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 9, 2015, at 1:55 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, Jun 9, 2015 at 1:52 PM, Pete Cooper<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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;" class="">Thanks for the feedback. Comments below, based on r239429 which i just pushed.<br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jun 9, 2015, at 1:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Tue, Jun 9, 2015 at 12:56 PM, Pete Cooper<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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 class="">Date: Tue Jun 9 14:56:05 2015<br class="">New Revision: 239428<br class=""><br class="">URL:<span class="Apple-converted-space"> </span><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=agNgruyxGHpanfdOqYZ-_-yMXjmiYDrc1Pwn2EUCPBw&s=V3XUT4-6s_c0T21m3DhhlaB7HDU9Nrb85lB2lafD7Gg&e=" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=239428&view=rev</a><br class="">Log:<br class="">Make MCSymbol::Name be a union of uint64_t and a pointer.<br class=""><br class="">This should hopefully fix the 32-bit bots which were allocating space for a pointer<br class="">but needed to be aligned to 64-bits.<br class=""><br class="">Now we allocate enough space for a uint64_t and a pointer and cast to the appropriate storage<br class=""></blockquote><div class=""><br class="">This kind fo complicates the rest of the MCSymbol code. A few options:<br class=""><br class="">have getNameEntry return the NameEntry reference as before (do the ".NameEntry" inside getNameEntry rather than in both the callers).<br class=""></div></div></div></div></div></blockquote></span>This is done.<span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">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 class=""></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 class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">& 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 class=""></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 class=""><br class="">Seems we have at least one example of a static assertion for alignment:<br class=""><br class="">include/llvm/Support/ArrayRecycler.h: static_assert(Align >= AlignOf<FreeList>::Alignment, "Object underaligned");<br class=""><br class="">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 class=""></div></div></div></blockquote>Yeah, that worked. Thanks! r239431.</div><div><br class=""></div><div>(will keep an eye on the bots just in case MSVC complains, but I assume it’ll be ok given the precedence you found)<br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""> </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;" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">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 class=""></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 class=""><br class=""></div><div class="">Thanks for the help!</div><div class=""><span class=""><font color="#888888" class="">Pete</font></span><div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </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 class="">Modified:<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/include/llvm/MC/MCSymbol.h<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/lib/MC/MCSymbol.cpp<br class=""><br class="">Modified: llvm/trunk/include/llvm/MC/MCSymbol.h<br class="">URL:<span class="Apple-converted-space"> </span><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=agNgruyxGHpanfdOqYZ-_-yMXjmiYDrc1Pwn2EUCPBw&s=47yDXtHkByi1NROPGAhukefqqGfGcHYG6Vmne7pbgWI&e=" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=239428&r1=239427&r2=239428&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/include/llvm/MC/MCSymbol.h (original)<br class="">+++ llvm/trunk/include/llvm/MC/MCSymbol.h Tue Jun 9 14:56:05 2015<br class="">@@ -120,20 +120,29 @@ protected: // MCContext creates and uniq<br class=""> friend class MCExpr;<br class=""> friend class MCContext;<br class=""><br class="">- typedef const StringMapEntry<bool> NameEntryTy;<br class="">- MCSymbol(SymbolKind Kind, NameEntryTy *Name, bool isTemporary)<br class="">+ /// \brief The name for a symbol.<br class="">+ /// MCSymbol contains a uint64_t so is probably aligned to 8. On a 32-bit<br class="">+ /// system, the name is a pointer so isn't going to satisfy the 8 byte<br class="">+ /// alignment of uint64_t. Account for that here.<br class="">+ typedef union {<br class="">+ const StringMapEntry<bool> *NameEntry;<br class="">+ uint64_t AlignmentPadding;<br class="">+ } NameEntryStorageTy;<br class="">+<br class="">+ MCSymbol(SymbolKind Kind, const StringMapEntry<bool> *Name, bool isTemporary)<br class=""> : Value(nullptr), IsTemporary(isTemporary),<br class=""> IsRedefinable(false), IsUsed(false), IsRegistered(false),<br class=""> IsExternal(false), IsPrivateExtern(false), HasName(!!Name),<br class=""> Kind(Kind) {<br class=""> Offset = 0;<br class=""> if (Name)<br class="">- getNameEntryPtr() = Name;<br class="">+ getNameEntryPtr().NameEntry = Name;<br class=""> }<br class=""><br class=""> // Provide custom new/delete as we will only allocate space for a name<br class=""> // if we need one.<br class="">- void *operator new(size_t s, NameEntryTy *Name, MCContext &Ctx);<br class="">+ void *operator new(size_t s, const StringMapEntry<bool> *Name,<br class="">+ MCContext &Ctx);<br class=""><br class=""> private:<br class=""><br class="">@@ -160,14 +169,14 @@ private:<br class=""> }<br class=""><br class=""> /// \brief Get a reference to the name field. Requires that we have a name<br class="">- NameEntryTy *&getNameEntryPtr() {<br class="">+ NameEntryStorageTy &getNameEntryPtr() {<br class=""> assert(HasName && "Name is required");<br class="">- NameEntryTy **Name = reinterpret_cast<NameEntryTy **>(this);<br class="">+ NameEntryStorageTy *Name = reinterpret_cast<NameEntryStorageTy *>(this);<br class=""> return *(Name - 1);<br class=""> }<br class="">- NameEntryTy *const &getNameEntryPtr() const {<br class="">+ const NameEntryStorageTy &getNameEntryPtr() const {<br class=""> assert(HasName && "Name is required");<br class="">- NameEntryTy *const *Name = reinterpret_cast<NameEntryTy *const *>(this);<br class="">+ const auto *Name = reinterpret_cast<const NameEntryStorageTy *>(this);<br class=""> return *(Name - 1);<br class=""> }<br class=""><br class="">@@ -177,7 +186,7 @@ public:<br class=""> if (!HasName)<br class=""> return StringRef();<br class=""><br class="">- return getNameEntryPtr()->first();<br class="">+ return getNameEntryPtr().NameEntry->first();<br class=""> }<br class=""><br class=""> bool isRegistered() const { return IsRegistered; }<br class=""><br class="">Modified: llvm/trunk/lib/MC/MCSymbol.cpp<br class="">URL:<span class="Apple-converted-space"> </span><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=agNgruyxGHpanfdOqYZ-_-yMXjmiYDrc1Pwn2EUCPBw&s=4BvT1OpMirOnCTr3HH-A59Md2CI-_507vZCLXjKBO34&e=" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=239428&r1=239427&r2=239428&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/MC/MCSymbol.cpp (original)<br class="">+++ llvm/trunk/lib/MC/MCSymbol.cpp Tue Jun 9 14:56:05 2015<br class="">@@ -19,17 +19,20 @@ using namespace llvm;<br class=""> // Sentinel value for the absolute pseudo section.<br class=""> MCSection *MCSymbol::AbsolutePseudoSection = reinterpret_cast<MCSection *>(1);<br class=""><br class="">-void *MCSymbol::operator new(size_t s, NameEntryTy *Name, MCContext &Ctx) {<br class="">- size_t Size = s + (Name ? sizeof(Name) : 0);<br class="">+void *MCSymbol::operator new(size_t s, const StringMapEntry<bool> *Name,<br class="">+ MCContext &Ctx) {<br class="">+ // We may need more space for a Name to account for alignment. So allocate<br class="">+ // space for the storage type and not the name pointer.<br class="">+ size_t Size = s + (Name ? sizeof(NameEntryStorageTy) : 0);<br class=""><br class=""> // For safety, ensure that the alignment of a pointer is enough for an<br class=""> // MCSymbol. This also ensures we don't need padding between the name and<br class=""> // symbol.<br class="">- assert(alignOf<MCSymbol>() <= alignOf<NameEntryTy *>() &&<br class="">+ assert(alignOf<MCSymbol>() <= alignOf<NameEntryStorageTy>() &&<br class=""> <span class="Apple-converted-space"> </span>"Bad alignment of MCSymbol");<br class="">- void *Storage = Ctx.allocate(Size, alignOf<NameEntryTy *>());<br class="">- NameEntryTy **Start = static_cast<NameEntryTy**>(Storage);<br class="">- NameEntryTy **End = Start + (Name ? 1 : 0);<br class="">+ void *Storage = Ctx.allocate(Size, alignOf<NameEntryStorageTy>());<br class="">+ NameEntryStorageTy *Start = static_cast<NameEntryStorageTy*>(Storage);<br class="">+ NameEntryStorageTy *End = Start + (Name ? 1 : 0);<br class=""> return End;<br class=""> }<br class=""><br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></div></div></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div><br class=""></body></html>