[ARM/AArch64] Enforce alignment for bitfielded structs

[bsmith]

Summary

When creating a global variable with a type of a struct with bitfields, we must forcibly set the alignment of the global from the RecordDecl. We must do this so that the proper bitfield alignment makes its way down to LLVM, since clang will mangle the bitfields into one large type.

This patch feels like a bit of a hack to me, but honestly I'm not sure how else to go about making this change. This patch is a first attempt to address the problem described in the thread http://lists.cs.uiuc.edu/pipermail/cfe-dev/2015-April/042521.html.

(There is no testcase attached to this just yet I shall add one tomorrow, I just wanted to get this out for review early to see whether I'm approaching this the right way).

[rengolin]

Doesn't Clang have some king of target description infrastructure? I'd guess that adding a new field / method to get the bitfield struct alignment would be the the best option.

[bsmith]

Added testcase and removed triple check in favour of a TargetInfo query.

[rengolin]

Removing the redundant line, LGTM. Thanks!

lib/CodeGen/CodeGenModule.cpp

1806

redundant line?

[bsmith]

Thanks, committed as r235976 (with redundant line removed).

include/clang/Basic/TargetInfo.h

protected:
   /// If non-zero, specifies a fixed alignment value for bitfields that follow
   /// zero length bitfield, regardless of the zero length bitfield type.
   unsigned ZeroLengthBitfieldBoundary;
 
   /// \brief Specify if mangling based on address space map should be used or
   /// not for language specific address spaces
   bool UseAddrSpaceMapMangling;
 
+  /// \brief Specify if globals of a struct type containing bitfields should
+  /// have their alignment explicitly specified so as to ensure that LLVM uses
+  /// the correct alignment
+  bool EnforceBitfieldContainerAlignment;
+
 public:
   IntType getSizeType() const { return SizeType; }
   IntType getIntMaxType() const { return IntMaxType; }
   IntType getUIntMaxType() const {
     return getCorrespondingUnsignedType(IntMaxType);
   }
   IntType getPtrDiffType(unsigned AddrSpace) const {
     return AddrSpace == 0 ? PtrDiffType : getPtrDiffTypeV(AddrSpace);
unsigned getZeroLengthBitfieldBoundary() const {
     return ZeroLengthBitfieldBoundary;
   }
 
   /// \brief Check whether this target support '\#pragma options align=mac68k'.
   bool hasAlignMac68kSupport() const {
     return HasAlignMac68kSupport;
   }
 
+  /// \brief Check whether the alignment of bitfield struct should be enforced
+  bool enforceBitfieldContainerAlignment() const {
+    return EnforceBitfieldContainerAlignment;
+  }
+
   /// \brief Return the user string for the specified integer type enum.
   ///
   /// For example, SignedShort -> "short".
   static const char *getTypeName(IntType T);
 
   /// \brief Return the constant suffix for the specified integer type enum.
   ///
   /// For example, SignedLong -> "L".

lib/Basic/TargetInfo.cpp

TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {
   DoubleFormat = &llvm::APFloat::IEEEdouble;
   LongDoubleFormat = &llvm::APFloat::IEEEdouble;
   DescriptionString = nullptr;
   UserLabelPrefix = "_";
   MCountName = "mcount";
   RegParmMax = 0;
   SSERegParmMax = 0;
   HasAlignMac68kSupport = false;
+  EnforceBitfieldContainerAlignment = false;
 
   // Default to no types using fpret.
   RealTypeUsesObjCFPRet = 0;
 
   // Default to not using fp2ret for __Complex long double
   ComplexLongDoubleUsesFP2Ret = false;
 
   // Set the C++ ABI based on the triple.

lib/Basic/Targets.cpp

default:
       WCharType = UnsignedInt;
       break;
     }
 
     UseBitFieldTypeAlignment = true;
 
     ZeroLengthBitfieldBoundary = 0;
 
+    // Enforce the alignment of bitfield structs
+    EnforceBitfieldContainerAlignment = true;
+
     // Thumb1 add sp, #imm requires the immediate value be multiple of 4,
     // so set preferred for small types to 32.
     if (T.isOSBinFormatMachO()) {
       DescriptionString =
           BigEndian ? "E-m:o-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
                     : "e-m:o-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64";
     } else if (T.isOSWindows()) {
       assert(!BigEndian && "Windows on ARM does not support big endian");
AArch64TargetInfo(const llvm::Triple &Triple)
 
     // AAPCS gives rules for bitfields. 7.1.7 says: "The container type
     // contributes to the alignment of the containing aggregate in the same way
     // a plain (non bit-field) member of that type would, without exception for
     // zero-sized or anonymous bit-fields."
     UseBitFieldTypeAlignment = true;
     UseZeroLengthBitfieldAlignment = true;
 
+    // Enforce the alignment of bitfield structs
+    EnforceBitfieldContainerAlignment = true;
+
     // AArch64 targets default to using the ARM C++ ABI.
     TheCXXABI.set(TargetCXXABI::GenericAArch64);
   }
 
   StringRef getABI() const override { return ABI; }
   bool setABI(const std::string &Name) override {
     if (Name != "aapcs" && Name != "darwinpcs")
       return false;

lib/CodeGen/CodeGenModule.cpp

if (D) {
     }
 
     // Handle XCore specific ABI requirements.
     if (getTarget().getTriple().getArch() == llvm::Triple::xcore &&
         D->getLanguageLinkage() == CLanguageLinkage &&
         D->getType().isConstant(Context) &&
         isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
       GV->setSection(".cp.rodata");
+
+    // The ARM/AArch64 ABI expects structs with bitfields to respect the proper
+    // container alignment, hence we have to enfore this in the IR so as to
+    // work around clang combining bitfields into one large type.
+    llvm::Triple::ArchType Arch = Context.getTargetInfo().getTriple().getArch();
+    if (getContext().getTargetInfo().enforceBitfieldContainerAlignment()) {
+      if (const auto *RT = D->getType()->getAs<RecordType>()) {
+        const RecordDecl *RD = RT->getDecl();
+
+        for (auto I = RD->field_begin(), End = RD->field_end(); I != End; ++I) {
+          if ((*I)->isBitField()) {
+            const ASTRecordLayout &Info = getContext().getASTRecordLayout(RD);
+            GV->setAlignment(Info.getAlignment().getQuantity());
+            break;
+          }
+        }
+      }
+    }
   }
 
   if (AddrSpace != Ty->getAddressSpace())
     return llvm::ConstantExpr::getAddrSpaceCast(GV, Ty);
 
   return GV;
 }
 

test/CodeGen/arm-bitfield-alignment.c

+// RUN: %clang_cc1 -triple arm-none-eabi -ffreestanding -emit-llvm -o - -O3 %s | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64 -ffreestanding -emit-llvm -o - -O3 %s | FileCheck %s
+
+extern struct T {
+  int b0 : 8;
+  int b1 : 24;
+  int b2 : 1;
+} g;
+
+int func() {
+  return g.b1;
+}
+
+// CHECK: @g = external global %struct.T, align 4
+// CHECK: %{{.*}} = load i64, i64* bitcast (%struct.T* @g to i64*), align 4