I have reverted it (and r230448), for now. Reproducible with LLVM_DEFAULT_TARGET_TRIPLE=x86_64-win32.
Unfortunately, clang/test/PCH/headersearch.cpp is skipped on Windows due to shell issue. Could you take a look please? 2015-02-25 11:16 GMT+09:00 Reid Kleckner <[email protected]>: > Author: rnk > Date: Tue Feb 24 20:16:09 2015 > New Revision: 230446 > > URL: http://llvm.org/viewvc/llvm-project?rev=230446&view=rev > Log: > MS ABI: Try to respect external AST source record layouts > > Covered by existing tests in test/CodeGen/override-layout.c and > test/CodeGenCXX/override-layout.cpp. Seriously, they found real bugs in > my code. :) > > Modified: > cfe/trunk/lib/AST/RecordLayoutBuilder.cpp > cfe/trunk/test/CodeGenCXX/override-layout.cpp > > Modified: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/RecordLayoutBuilder.cpp?rev=230446&r1=230445&r2=230446&view=diff > ============================================================================== > --- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp (original) > +++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp Tue Feb 24 20:16:09 2015 > @@ -55,6 +55,52 @@ struct BaseSubobjectInfo { > const BaseSubobjectInfo *Derived; > }; > > +/// \brief Externally provided layout. Typically used when the AST source, > such > +/// as DWARF, lacks all the information that was available at compile time, > such > +/// as alignment attributes on fields and pragmas in effect. > +struct ExternalLayout { > + ExternalLayout() : Size(0), Align(0) {} > + > + /// \brief Overall record size in bits. > + uint64_t Size; > + > + /// \brief Overall record alignment in bits. > + uint64_t Align; > + > + /// \brief Record field offsets in bits. > + llvm::DenseMap<const FieldDecl *, uint64_t> FieldOffsets; > + > + /// \brief Direct, non-virtual base offsets. > + llvm::DenseMap<const CXXRecordDecl *, CharUnits> BaseOffsets; > + > + /// \brief Virtual base offsets. > + llvm::DenseMap<const CXXRecordDecl *, CharUnits> VirtualBaseOffsets; > + > + /// Get the offset of the given field. The external source must provide > + /// entries for all fields in the record. > + uint64_t getExternalFieldOffset(const FieldDecl *FD) { > + assert(FieldOffsets.count(FD) && > + "Field does not have an external offset"); > + return FieldOffsets[FD]; > + } > + > + bool getExternalNVBaseOffset(const CXXRecordDecl *RD, CharUnits > &BaseOffset) { > + auto Known = BaseOffsets.find(RD); > + if (Known == BaseOffsets.end()) > + return false; > + BaseOffset = Known->second; > + return true; > + } > + > + bool getExternalVBaseOffset(const CXXRecordDecl *RD, CharUnits > &BaseOffset) { > + auto Known = VirtualBaseOffsets.find(RD); > + if (Known == VirtualBaseOffsets.end()) > + return false; > + BaseOffset = Known->second; > + return true; > + } > +}; > + > /// EmptySubobjectMap - Keeps track of which empty subobjects exist at > different > /// offsets while laying out a C++ class. > class EmptySubobjectMap { > @@ -541,7 +587,7 @@ protected: > > /// \brief Whether the external AST source has provided a layout for this > /// record. > - unsigned ExternalLayout : 1; > + unsigned UseExternalLayout : 1; > > /// \brief Whether we need to infer alignment, even when we have an > /// externally-provided layout. > @@ -607,26 +653,14 @@ protected: > /// avoid visiting virtual bases more than once. > llvm::SmallPtrSet<const CXXRecordDecl *, 4> VisitedVirtualBases; > > - /// \brief Externally-provided size. > - uint64_t ExternalSize; > - > - /// \brief Externally-provided alignment. > - uint64_t ExternalAlign; > - > - /// \brief Externally-provided field offsets. > - llvm::DenseMap<const FieldDecl *, uint64_t> ExternalFieldOffsets; > - > - /// \brief Externally-provided direct, non-virtual base offsets. > - llvm::DenseMap<const CXXRecordDecl *, CharUnits> ExternalBaseOffsets; > - > - /// \brief Externally-provided virtual base offsets. > - llvm::DenseMap<const CXXRecordDecl *, CharUnits> > ExternalVirtualBaseOffsets; > + /// Valid if UseExternalLayout is true. > + ExternalLayout External; > > RecordLayoutBuilder(const ASTContext &Context, > EmptySubobjectMap *EmptySubobjects) > : Context(Context), EmptySubobjects(EmptySubobjects), Size(0), > Alignment(CharUnits::One()), UnpackedAlignment(CharUnits::One()), > - ExternalLayout(false), InferAlignment(false), > + UseExternalLayout(false), InferAlignment(false), > Packed(false), IsUnion(false), IsMac68kAlign(false), IsMsStruct(false), > UnfilledBitsInLastUnit(0), LastBitfieldTypeSize(0), > MaxFieldAlignment(CharUnits::Zero()), > @@ -1134,21 +1168,12 @@ CharUnits RecordLayoutBuilder::LayoutBas > > // Query the external layout to see if it provides an offset. > bool HasExternalLayout = false; > - if (ExternalLayout) { > + if (UseExternalLayout) { > llvm::DenseMap<const CXXRecordDecl *, CharUnits>::iterator Known; > - if (Base->IsVirtual) { > - Known = ExternalVirtualBaseOffsets.find(Base->Class); > - if (Known != ExternalVirtualBaseOffsets.end()) { > - Offset = Known->second; > - HasExternalLayout = true; > - } > - } else { > - Known = ExternalBaseOffsets.find(Base->Class); > - if (Known != ExternalBaseOffsets.end()) { > - Offset = Known->second; > - HasExternalLayout = true; > - } > - } > + if (Base->IsVirtual) > + HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, > Offset); > + else > + HasExternalLayout = External.getExternalVBaseOffset(Base->Class, > Offset); > } > > CharUnits UnpackedBaseAlign = Layout.getNonVirtualAlignment(); > @@ -1235,18 +1260,15 @@ void RecordLayoutBuilder::InitializeLayo > > // If there is an external AST source, ask it for the various offsets. > if (const RecordDecl *RD = dyn_cast<RecordDecl>(D)) > - if (ExternalASTSource *External = Context.getExternalSource()) { > - ExternalLayout = External->layoutRecordType(RD, > - ExternalSize, > - ExternalAlign, > - ExternalFieldOffsets, > - ExternalBaseOffsets, > - > ExternalVirtualBaseOffsets); > - > + if (ExternalASTSource *Source = Context.getExternalSource()) { > + UseExternalLayout = Source->layoutRecordType( > + RD, External.Size, External.Align, External.FieldOffsets, > + External.BaseOffsets, External.VirtualBaseOffsets); > + > // Update based on external alignment. > - if (ExternalLayout) { > - if (ExternalAlign > 0) { > - Alignment = Context.toCharUnitsFromBits(ExternalAlign); > + if (UseExternalLayout) { > + if (External.Align > 0) { > + Alignment = Context.toCharUnitsFromBits(External.Align); > } else { > // The external source didn't have alignment information; infer it. > InferAlignment = true; > @@ -1588,7 +1610,7 @@ void RecordLayoutBuilder::LayoutBitField > > // If we're using external layout, give the external layout a chance > // to override this information. > - if (ExternalLayout) > + if (UseExternalLayout) > FieldOffset = updateExternalFieldOffset(D, FieldOffset); > > // Okay, place the bitfield at the calculated offset. > @@ -1604,7 +1626,7 @@ void RecordLayoutBuilder::LayoutBitField > FieldAlign = UnpackedFieldAlign = 1; > > // Diagnose differences in layout due to padding or packing. > - if (!ExternalLayout) > + if (!UseExternalLayout) > CheckFieldPadding(FieldOffset, UnpaddedFieldOffset, UnpackedFieldOffset, > UnpackedFieldAlign, FieldPacked, D); > > @@ -1727,7 +1749,7 @@ void RecordLayoutBuilder::LayoutField(co > UnpackedFieldOffset = > UnpackedFieldOffset.RoundUpToAlignment(UnpackedFieldAlign); > > - if (ExternalLayout) { > + if (UseExternalLayout) { > FieldOffset = Context.toCharUnitsFromBits( > updateExternalFieldOffset(D, > Context.toBits(FieldOffset))); > > @@ -1750,7 +1772,7 @@ void RecordLayoutBuilder::LayoutField(co > // Place this field at the current location. > FieldOffsets.push_back(Context.toBits(FieldOffset)); > > - if (!ExternalLayout) > + if (!UseExternalLayout) > CheckFieldPadding(Context.toBits(FieldOffset), UnpaddedFieldOffset, > Context.toBits(UnpackedFieldOffset), > Context.toBits(UnpackedFieldAlign), FieldPacked, D); > @@ -1802,15 +1824,15 @@ void RecordLayoutBuilder::FinishLayout(c > uint64_t RoundedSize > = llvm::RoundUpToAlignment(getSizeInBits(), Context.toBits(Alignment)); > > - if (ExternalLayout) { > + if (UseExternalLayout) { > // If we're inferring alignment, and the external size is smaller than > // our size after we've rounded up to alignment, conservatively set the > // alignment to 1. > - if (InferAlignment && ExternalSize < RoundedSize) { > + if (InferAlignment && External.Size < RoundedSize) { > Alignment = CharUnits::One(); > InferAlignment = false; > } > - setSize(ExternalSize); > + setSize(External.Size); > return; > } > > @@ -1846,7 +1868,7 @@ void RecordLayoutBuilder::UpdateAlignmen > CharUnits UnpackedNewAlignment) { > // The alignment is not modified when using 'mac68k' alignment or when > // we have an externally-supplied layout that also provides overall > alignment. > - if (IsMac68kAlign || (ExternalLayout && !InferAlignment)) > + if (IsMac68kAlign || (UseExternalLayout && !InferAlignment)) > return; > > if (NewAlignment > Alignment) { > @@ -1865,11 +1887,8 @@ void RecordLayoutBuilder::UpdateAlignmen > uint64_t > RecordLayoutBuilder::updateExternalFieldOffset(const FieldDecl *Field, > uint64_t ComputedOffset) { > - assert(ExternalFieldOffsets.find(Field) != ExternalFieldOffsets.end() && > - "Field does not have an external offset"); > - > - uint64_t ExternalFieldOffset = ExternalFieldOffsets[Field]; > - > + uint64_t ExternalFieldOffset = External.getExternalFieldOffset(Field); > + > if (InferAlignment && ExternalFieldOffset < ComputedOffset) { > // The externally-supplied field offset is before the field offset we > // computed. Assume that the structure is packed. > @@ -2251,6 +2270,13 @@ public: > /// \brief True if this class is zero sized or first base is zero sized or > /// has this property. Only used for MS-ABI. > bool LeadsWithZeroSizedBase : 1; > + > + /// \brief True if the external AST source provided a layout for this > record. > + bool UseExternalLayout : 1; > + > + /// \brief The layout provided by the external AST source. Only active if > + /// UseExternalLayout is true. > + ExternalLayout External; > }; > } // namespace > > @@ -2370,6 +2396,13 @@ void MicrosoftRecordLayoutBuilder::initi > // Packed attribute forces max field alignment to be 1. > if (RD->hasAttr<PackedAttr>()) > MaxFieldAlignment = CharUnits::One(); > + > + // Try to respect the external layout if present. > + UseExternalLayout = false; > + if (ExternalASTSource *Source = Context.getExternalSource()) > + UseExternalLayout = Source->layoutRecordType( > + RD, External.Size, External.Align, External.FieldOffsets, > + External.BaseOffsets, External.VirtualBaseOffsets); > } > > void > @@ -2474,7 +2507,18 @@ void MicrosoftRecordLayoutBuilder::layou > BaseLayout.leadsWithZeroSizedBase()) > Size++; > ElementInfo Info = getAdjustedElementInfo(BaseLayout); > - CharUnits BaseOffset = Size.RoundUpToAlignment(Info.Alignment); > + CharUnits BaseOffset; > + > + // Respect the external AST source base offset, if present. > + bool FoundBase = false; > + if (UseExternalLayout) { > + FoundBase = External.getExternalNVBaseOffset(BaseDecl, BaseOffset); > + if (FoundBase) > + assert(BaseOffset >= Size && "base offset already allocated"); > + } > + > + if (!FoundBase) > + BaseOffset = Size.RoundUpToAlignment(Info.Alignment); > Bases.insert(std::make_pair(BaseDecl, BaseOffset)); > Size = BaseOffset + BaseLayout.getNonVirtualSize(); > PreviousBaseLayout = &BaseLayout; > @@ -2498,7 +2542,14 @@ void MicrosoftRecordLayoutBuilder::layou > placeFieldAtOffset(CharUnits::Zero()); > Size = std::max(Size, Info.Size); > } else { > - CharUnits FieldOffset = Size.RoundUpToAlignment(Info.Alignment); > + CharUnits FieldOffset; > + if (UseExternalLayout) { > + FieldOffset = > + Context.toCharUnitsFromBits(External.getExternalFieldOffset(FD)); > + assert(FieldOffset >= Size && "field offset already allocated"); > + } else { > + FieldOffset = Size.RoundUpToAlignment(Info.Alignment); > + } > placeFieldAtOffset(FieldOffset); > Size = FieldOffset + Info.Size; > } > @@ -2572,14 +2623,16 @@ void MicrosoftRecordLayoutBuilder::injec > CharUnits InjectionSite = VBPtrOffset; > // But before we do, make sure it's properly aligned. > VBPtrOffset = VBPtrOffset.RoundUpToAlignment(PointerInfo.Alignment); > + // Shift everything after the vbptr down, unless we're using an external > + // layout. > + if (UseExternalLayout) > + return; > // Determine where the first field should be laid out after the vbptr. > CharUnits FieldStart = VBPtrOffset + PointerInfo.Size; > // Make sure that the amount we push the fields back by is a multiple of > the > // alignment. > CharUnits Offset = (FieldStart - InjectionSite).RoundUpToAlignment( > std::max(RequiredAlignment, Alignment)); > - // Increase the size of the object and push back all fields by the offset > - // amount. > Size += Offset; > for (uint64_t &FieldOffset : FieldOffsets) > FieldOffset += Context.toBits(Offset); > @@ -2646,7 +2699,18 @@ void MicrosoftRecordLayoutBuilder::layou > } > // Insert the virtual base. > ElementInfo Info = getAdjustedElementInfo(BaseLayout); > - CharUnits BaseOffset = Size.RoundUpToAlignment(Info.Alignment); > + CharUnits BaseOffset; > + > + // Respect the external AST source base offset, if present. > + bool FoundBase = false; > + if (UseExternalLayout) { > + FoundBase = External.getExternalVBaseOffset(BaseDecl, BaseOffset); > + if (FoundBase) > + assert(BaseOffset >= Size && "base offset already allocated"); > + } > + if (!FoundBase) > + BaseOffset = Size.RoundUpToAlignment(Info.Alignment); > + > VBases.insert(std::make_pair(BaseDecl, > ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp))); > Size = BaseOffset + BaseLayout.getNonVirtualSize(); > @@ -2676,6 +2740,12 @@ void MicrosoftRecordLayoutBuilder::final > else > Size = MinEmptyStructSize; > } > + > + if (UseExternalLayout) { > + Size = Context.toCharUnitsFromBits(External.Size); > + if (External.Align) > + Alignment = Context.toCharUnitsFromBits(External.Align); > + } > } > > // Recursively walks the non-virtual bases of a class and determines if any > of > @@ -2814,7 +2884,7 @@ ASTContext::getASTRecordLayout(const Rec > > const ASTRecordLayout *NewEntry = nullptr; > > - if (isMsLayout(D) && !D->getASTContext().getExternalSource()) { > + if (isMsLayout(D)) { > NewEntry = BuildMicrosoftASTRecordLayout(D); > } else if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) { > EmptySubobjectMap EmptySubobjects(*this, RD); > > Modified: cfe/trunk/test/CodeGenCXX/override-layout.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/override-layout.cpp?rev=230446&r1=230445&r2=230446&view=diff > ============================================================================== > --- cfe/trunk/test/CodeGenCXX/override-layout.cpp (original) > +++ cfe/trunk/test/CodeGenCXX/override-layout.cpp Tue Feb 24 20:16:09 2015 > @@ -1,6 +1,6 @@ > -// RUN: %clang_cc1 -fdump-record-layouts-simple %s > %t.layouts > -// RUN: %clang_cc1 -fdump-record-layouts-simple %s > %t.before > -// RUN: %clang_cc1 -DPACKED= -DALIGNED16= -fdump-record-layouts-simple > -foverride-record-layout=%t.layouts %s > %t.after > +// RUN: %clang_cc1 -w -fdump-record-layouts-simple %s > %t.layouts > +// RUN: %clang_cc1 -w -fdump-record-layouts-simple %s > %t.before > +// RUN: %clang_cc1 -w -DPACKED= -DALIGNED16= -fdump-record-layouts-simple > -foverride-record-layout=%t.layouts %s > %t.after > // RUN: diff -u %t.before %t.after > // RUN: FileCheck %s < %t.after > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
