<div dir="ltr">Shouldn't a diagnostic be given if the path returning the incorrect calculation would return a result that disagrees from the correct calculation?</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 13, 2015 at 1:45 AM, Ristow, Warren <span dir="ltr"><<a href="mailto:warren_ristow@playstation.sony.com" target="_blank">warren_ristow@playstation.sony.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-family:"Courier New"">Hi,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">We at Sony have a particularly strong interest in not breaking object file compatibility from release to release. So when a bug exists in release X, which gets fixed in release Y, where that bugfix
is an ABI-breaking change, we generally have to weigh the pluses of fixing the bug against the minuses of breaking our object compatibility goal. The impact of object file incompatibility is quite bad for us, and so we likely are more concerned than most
toolchain providers about this issue. But that said, I would imagine there are others out there that have some concerns along these lines. I'm curious to hear about preferred methods of dealing with this.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">A straightforward solution is for us simply to guard an ABI-breaking bugfix with our PS4-triple. Conceptually:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (TargetIsPS4) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> // old way, with known bug, to retain object compatibility<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> } else {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> // new way, that fixes bug, but breaks object compatibility<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">But if others are interested in retaining the compatibility, then instead, something like the following may be preferred:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (TargetUsesBuggyApproachX) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> // old way, with known bug, to retain object compatibility<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> } else {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> // new way, that fixes bug, but breaks object compatibility<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">where the concept of "Target Uses Buggy Approach X" is implemented by, for example, adding another single-bit bitfield to the TargetInfo class, and initializing it appropriately for different targets.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">To be more specific, a current example of an ABI-breaking issue for us is PR22279:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D22279&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6GUNn7_wosSM&m=9_owTgWqMU8YQqgUVNKzb3EpPiJYmVx8UnQU6ouPH-8&s=94chD-pzcdYvsLLAWwRdcblPn3Eg8JvOmGYAtG4Efrc&e=" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=22279</a><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">As described there, a bugfix in the handling of alignment attributes (and alignas()) for enums causes a change in object layout. For our customers, and hence for us, the fix in proper alignment of
enum types in these cases isn't as important as retaining object compatibility. So we need to suppress that fix. It's easy enough for us to suppress the change by checking whether the target is PS4, but adding a bit to TargetInfo like:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> unsigned IgnoreEnumTypeAlignment : 1;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">seems better. I'm appending a patch on clang below, that shows this approach concretely.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">Or possibly there is some other preferred approach? I'm curious to hear people's opinions.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">Thanks,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">-Warren<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">--<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">Warren Ristow<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">SN Systems - Sony Computer Entertainment Group<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">diff --git include/clang/Basic/TargetInfo.h include/clang/Basic/TargetInfo.h<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">index fed69a8..7aca509 100644<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">--- include/clang/Basic/TargetInfo.h<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+++ include/clang/Basic/TargetInfo.h<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -84,6 +84,7 @@ protected:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> mutable VersionTuple PlatformMinVersion;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> unsigned HasAlignMac68kSupport : 1;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ unsigned IgnoreEnumTypeAlignment : 1;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> unsigned RealTypeUsesObjCFPRet : 3;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> unsigned ComplexLongDoubleUsesFP2Ret : 1;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -463,6 +464,11 @@ public:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> return HasAlignMac68kSupport;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ /// \brief Ignore alignment specifier on definitions of enum types<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ bool ignoreEnumTypeAlignment() const {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ return IgnoreEnumTypeAlignment;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> /// \brief Return the user string for the specified integer type enum.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> ///<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> /// For example, SignedShort -> "short".<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">diff --git lib/AST/ASTContext.cpp lib/AST/ASTContext.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">index fb96301..6a76ef3 100644<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">--- lib/AST/ASTContext.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+++ lib/AST/ASTContext.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -1705,6 +1705,8 @@ TypeInfo ASTContext::getTypeInfoImpl(const Type *T) const {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (const EnumType *ET = dyn_cast<EnumType>(TT)) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> const EnumDecl *ED = ET->getDecl();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ if (getTargetInfo().ignoreEnumTypeAlignment())<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ return getTypeInfo(ED->getIntegerType());<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> TypeInfo Info =<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> getTypeInfo(ED->getIntegerType()->getUnqualifiedDesugaredType());<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (unsigned AttrAlign = ED->getMaxAlignment()) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -1846,7 +1848,8 @@ unsigned ASTContext::getPreferredTypeAlign(const Type *T) const {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (const ComplexType *CT = T->getAs<ComplexType>())<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> T = CT->getElementType().getTypePtr();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (const EnumType *ET = T->getAs<EnumType>())<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">- T = ET->getDecl()->getIntegerType().getTypePtr();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ if (!getTargetInfo().ignoreEnumTypeAlignment())<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ T = ET->getDecl()->getIntegerType().getTypePtr();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (T->isSpecificBuiltinType(BuiltinType::Double) ||<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> T->isSpecificBuiltinType(BuiltinType::LongLong) ||<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> T->isSpecificBuiltinType(BuiltinType::ULongLong))<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">diff --git lib/Basic/TargetInfo.cpp lib/Basic/TargetInfo.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">index 856ad50..99ed923 100644<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">--- lib/Basic/TargetInfo.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+++ lib/Basic/TargetInfo.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -76,6 +76,7 @@ TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> RegParmMax = 0;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> SSERegParmMax = 0;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> HasAlignMac68kSupport = false;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ IgnoreEnumTypeAlignment = false;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> // Default to no types using fpret.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> RealTypeUsesObjCFPRet = 0;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">diff --git lib/Basic/Targets.cpp lib/Basic/Targets.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">index c5bf90f..b93343b 100644<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">--- lib/Basic/Targets.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+++ lib/Basic/Targets.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -580,6 +580,14 @@ public:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> PS4OSTargetInfo(const llvm::Triple &Triple) : OSTargetInfo<Target>(Triple) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> this->WCharType = this->UnsignedShort;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ // On PS4, for object compatibility with compilers prior to the fix of<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ // PR22279, we ignore alignment specifiers on definitions of enum types.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ // For example, in the following fragment, the alignment of 'y' will<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ // be 4, rather than 16:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ // enum Y { val1, val2, val3 } __attribute__((aligned(16)));<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ // Y y;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ IgnoreEnumTypeAlignment = true;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> this->UserLabelPrefix = "";<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> switch (Triple.getArch()) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">diff --git lib/Sema/SemaDeclAttr.cpp lib/Sema/SemaDeclAttr.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">index b8d0830..3843fcb 100644<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">--- lib/Sema/SemaDeclAttr.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+++ lib/Sema/SemaDeclAttr.cpp<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">@@ -3024,8 +3024,9 @@ void Sema::CheckAlignasUnderalignment(Decl *D) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> UnderlyingTy = DiagTy = VD->getType();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> } else {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> UnderlyingTy = DiagTy = Context.getTagDeclType(cast<TagDecl>(D));<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">- if (EnumDecl *ED = dyn_cast<EnumDecl>(D))<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">- UnderlyingTy = ED->getIntegerType();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ if (!Context.getTargetInfo().ignoreEnumTypeAlignment())<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ if (EnumDecl *ED = dyn_cast<EnumDecl>(D))<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">+ UnderlyingTy = ED->getIntegerType();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> if (DiagTy->isDependentType() || DiagTy->isIncompleteType())<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New""> return;<u></u><u></u></span></p>
</div>
</div>
<br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>