[cfe-dev] Managing bugfixes that cause struct-layout changes
David Majnemer
david.majnemer at gmail.com
Tue Jul 14 10:57:30 PDT 2015
On Tue, Jul 14, 2015 at 10:46 AM, Ristow, Warren <
warren_ristow at playstation.sony.com> wrote:
> Yes, I agree a diagnostic is a good idea.
>
>
>
> That said, I wasn't so much proposing the specifics of that example-patch
> as the precise fix/change to make, as I was using it to concretely
> demonstrate to concept of how I was thinking of adding a flag to the
> TargetInfo class, and then making use of it. I'm wondering do others care
> about an object-compatibility issue like this (in which case guarding it in
> this more generic way seems sensible), or would it be better to just guard
> it directly with the PS4-triple?
>
I'm not so sure we need to burn a bit for this sort of thing. We already
have stuff like honorsRevision0_98() which is purely triple based.
Us on the clang-cl side care deeply about compatibility between clang and
MSVC to the point where we will mimic their semantic bugs. However, we are
not interested in mimicing bugs in older versions of clang.
TBH, I'm very curious how far you want to take this. Would every fix to
ItaniumMangle.cpp be wrapped in if () logic?
>
> Thanks,
>
> -Warren
>
>
>
> --
>
> Warren Ristow
>
> SN Systems - Sony Computer Entertainment Group
>
>
>
> *From:* David Majnemer [mailto:david.majnemer at gmail.com]
> *Sent:* Monday, July 13, 2015 4:56 PM
> *To:* Ristow, Warren
> *Cc:* cfe-dev at cs.uiuc.edu
> *Subject:* Re: [cfe-dev] Managing bugfixes that cause struct-layout
> changes
>
>
>
> Shouldn't a diagnostic be given if the path returning the incorrect
> calculation would return a result that disagrees from the correct
> calculation?
>
>
>
> On Mon, Jul 13, 2015 at 1:45 AM, Ristow, Warren <
> warren_ristow at playstation.sony.com> wrote:
>
> Hi,
>
>
>
> 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.
>
>
>
> A straightforward solution is for us simply to guard an ABI-breaking
> bugfix with our PS4-triple. Conceptually:
>
>
>
> if (TargetIsPS4) {
>
> // old way, with known bug, to retain object compatibility
>
> } else {
>
> // new way, that fixes bug, but breaks object compatibility
>
> }
>
>
>
> But if others are interested in retaining the compatibility, then instead,
> something like the following may be preferred:
>
>
>
> if (TargetUsesBuggyApproachX) {
>
> // old way, with known bug, to retain object compatibility
>
> } else {
>
> // new way, that fixes bug, but breaks object compatibility
>
> }
>
>
>
> 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.
>
>
>
> To be more specific, a current example of an ABI-breaking issue for us is
> PR22279:
>
>
>
> https://llvm.org/bugs/show_bug.cgi?id=22279
>
>
>
> 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:
>
>
>
> unsigned IgnoreEnumTypeAlignment : 1;
>
>
>
> seems better. I'm appending a patch on clang below, that shows this
> approach concretely.
>
>
>
> Or possibly there is some other preferred approach? I'm curious to hear
> people's opinions.
>
>
>
> Thanks,
>
> -Warren
>
>
>
> --
>
> Warren Ristow
>
> SN Systems - Sony Computer Entertainment Group
>
>
>
>
>
> diff --git include/clang/Basic/TargetInfo.h
> include/clang/Basic/TargetInfo.h
>
> index fed69a8..7aca509 100644
>
> --- include/clang/Basic/TargetInfo.h
>
> +++ include/clang/Basic/TargetInfo.h
>
> @@ -84,6 +84,7 @@ protected:
>
> mutable VersionTuple PlatformMinVersion;
>
>
>
> unsigned HasAlignMac68kSupport : 1;
>
> + unsigned IgnoreEnumTypeAlignment : 1;
>
> unsigned RealTypeUsesObjCFPRet : 3;
>
> unsigned ComplexLongDoubleUsesFP2Ret : 1;
>
>
>
> @@ -463,6 +464,11 @@ public:
>
> return HasAlignMac68kSupport;
>
> }
>
>
>
> + /// \brief Ignore alignment specifier on definitions of enum types
>
> + bool ignoreEnumTypeAlignment() const {
>
> + return IgnoreEnumTypeAlignment;
>
> + }
>
> +
>
> /// \brief Return the user string for the specified integer type enum.
>
> ///
>
> /// For example, SignedShort -> "short".
>
> diff --git lib/AST/ASTContext.cpp lib/AST/ASTContext.cpp
>
> index fb96301..6a76ef3 100644
>
> --- lib/AST/ASTContext.cpp
>
> +++ lib/AST/ASTContext.cpp
>
> @@ -1705,6 +1705,8 @@ TypeInfo ASTContext::getTypeInfoImpl(const Type *T)
> const {
>
>
>
> if (const EnumType *ET = dyn_cast<EnumType>(TT)) {
>
> const EnumDecl *ED = ET->getDecl();
>
> + if (getTargetInfo().ignoreEnumTypeAlignment())
>
> + return getTypeInfo(ED->getIntegerType());
>
> TypeInfo Info =
>
>
> getTypeInfo(ED->getIntegerType()->getUnqualifiedDesugaredType());
>
> if (unsigned AttrAlign = ED->getMaxAlignment()) {
>
> @@ -1846,7 +1848,8 @@ unsigned ASTContext::getPreferredTypeAlign(const
> Type *T) const {
>
> if (const ComplexType *CT = T->getAs<ComplexType>())
>
> T = CT->getElementType().getTypePtr();
>
> if (const EnumType *ET = T->getAs<EnumType>())
>
> - T = ET->getDecl()->getIntegerType().getTypePtr();
>
> + if (!getTargetInfo().ignoreEnumTypeAlignment())
>
> + T = ET->getDecl()->getIntegerType().getTypePtr();
>
> if (T->isSpecificBuiltinType(BuiltinType::Double) ||
>
> T->isSpecificBuiltinType(BuiltinType::LongLong) ||
>
> T->isSpecificBuiltinType(BuiltinType::ULongLong))
>
> diff --git lib/Basic/TargetInfo.cpp lib/Basic/TargetInfo.cpp
>
> index 856ad50..99ed923 100644
>
> --- lib/Basic/TargetInfo.cpp
>
> +++ lib/Basic/TargetInfo.cpp
>
> @@ -76,6 +76,7 @@ TargetInfo::TargetInfo(const llvm::Triple &T) :
> TargetOpts(), Triple(T) {
>
> RegParmMax = 0;
>
> SSERegParmMax = 0;
>
> HasAlignMac68kSupport = false;
>
> + IgnoreEnumTypeAlignment = false;
>
>
>
> // Default to no types using fpret.
>
> RealTypeUsesObjCFPRet = 0;
>
> diff --git lib/Basic/Targets.cpp lib/Basic/Targets.cpp
>
> index c5bf90f..b93343b 100644
>
> --- lib/Basic/Targets.cpp
>
> +++ lib/Basic/Targets.cpp
>
> @@ -580,6 +580,14 @@ public:
>
> PS4OSTargetInfo(const llvm::Triple &Triple) :
> OSTargetInfo<Target>(Triple) {
>
> this->WCharType = this->UnsignedShort;
>
>
>
> + // On PS4, for object compatibility with compilers prior to the fix of
>
> + // PR22279, we ignore alignment specifiers on definitions of enum
> types.
>
> + // For example, in the following fragment, the alignment of 'y' will
>
> + // be 4, rather than 16:
>
> + // enum Y { val1, val2, val3 } __attribute__((aligned(16)));
>
> + // Y y;
>
> + IgnoreEnumTypeAlignment = true;
>
> +
>
> this->UserLabelPrefix = "";
>
>
>
> switch (Triple.getArch()) {
>
> diff --git lib/Sema/SemaDeclAttr.cpp lib/Sema/SemaDeclAttr.cpp
>
> index b8d0830..3843fcb 100644
>
> --- lib/Sema/SemaDeclAttr.cpp
>
> +++ lib/Sema/SemaDeclAttr.cpp
>
> @@ -3024,8 +3024,9 @@ void Sema::CheckAlignasUnderalignment(Decl *D) {
>
> UnderlyingTy = DiagTy = VD->getType();
>
> } else {
>
> UnderlyingTy = DiagTy = Context.getTagDeclType(cast<TagDecl>(D));
>
> - if (EnumDecl *ED = dyn_cast<EnumDecl>(D))
>
> - UnderlyingTy = ED->getIntegerType();
>
> + if (!Context.getTargetInfo().ignoreEnumTypeAlignment())
>
> + if (EnumDecl *ED = dyn_cast<EnumDecl>(D))
>
> + UnderlyingTy = ED->getIntegerType();
>
> }
>
> if (DiagTy->isDependentType() || DiagTy->isIncompleteType())
>
> return;
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150714/d7844137/attachment.html>
More information about the cfe-dev
mailing list