[cfe-dev] Managing bugfixes that cause struct-layout changes

Mon Jul 13 16:55:54 PDT 2015

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/20150713/35d17f8e/attachment.html>