From: Warren Hunt Date: Fri, 11 Apr 2014 22:05:28 +0000 (+0000) Subject: [MS-ABI] Update to vtordisp computation X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4639d293beef1db30b3523f1ad00c19754fa6f99;p=clang [MS-ABI] Update to vtordisp computation A portion of the vtordisp computation that was previously unguarded by a test for the declaration of user defined constructors/destructors was erroniously adding vtordisps to things that shouldn't have them. This patch correctly guards that codepath. In addition, it updates the comments to make them more clear. Test case is included. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@206077 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/AST/RecordLayoutBuilder.cpp b/lib/AST/RecordLayoutBuilder.cpp index c88423371a..73267f70a6 100644 --- a/lib/AST/RecordLayoutBuilder.cpp +++ b/lib/AST/RecordLayoutBuilder.cpp @@ -2655,16 +2655,20 @@ void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) { } } -static bool -RequiresVtordisp(const llvm::SmallPtrSet &HasVtordisp, - const CXXRecordDecl *RD) { - if (HasVtordisp.count(RD)) +// Recursively walks the non-virtual bases of a class and determines if any of +// them are in the bases with overridden methods set. +static bool RequiresVtordisp( + const llvm::SmallPtrSet & + BasesWithOverriddenMethods, + const CXXRecordDecl *RD) { + if (BasesWithOverriddenMethods.count(RD)) return true; // If any of a virtual bases non-virtual bases (recursively) requires a // vtordisp than so does this virtual base. for (const auto &I : RD->bases()) if (!I.isVirtual() && - RequiresVtordisp(HasVtordisp, I.getType()->getAsCXXRecordDecl())) + RequiresVtordisp(BasesWithOverriddenMethods, + I.getType()->getAsCXXRecordDecl())) return true; return false; } @@ -2703,38 +2707,38 @@ MicrosoftRecordLayoutBuilder::computeVtorDispSet(const CXXRecordDecl *RD) { if (bi.second.hasVtorDisp()) HasVtordispSet.insert(bi.first); } - // If we define a constructor or destructor and override a function that is - // defined in a virtual base's vtable, that virtual bases need a vtordisp. - // Here we collect a list of classes with vtables for which our virtual bases - // actually live. The virtual bases with this property will require - // vtordisps. In addition, virtual bases that contain non-virtual bases that - // define functions we override also require vtordisps, this case is checked - // explicitly below. - if (RD->hasUserDeclaredConstructor() || RD->hasUserDeclaredDestructor()) { - llvm::SmallPtrSet Work; - // Seed the working set with our non-destructor virtual methods. - for (const auto *I : RD->methods()) - if (I->isVirtual() && !isa(I)) - Work.insert(I); - while (!Work.empty()) { - const CXXMethodDecl *MD = *Work.begin(); - CXXMethodDecl::method_iterator i = MD->begin_overridden_methods(), - e = MD->end_overridden_methods(); - if (i == e) - // If a virtual method has no-overrides it lives in its parent's vtable. - HasVtordispSet.insert(MD->getParent()); - else - Work.insert(i, e); - // We've finished processing this element, remove it from the working set. - Work.erase(MD); - } + // If we do not have a user declared constructor or destructor then we don't + // introduce any additional vtordisps. + if (!RD->hasUserDeclaredConstructor() && !RD->hasUserDeclaredDestructor()) + return HasVtordispSet; + // Compute a set of base classes which define methods we override. A virtual + // base in this set will require a vtordisp. A virtual base that transitively + // contains one of these bases as a non-virtual base will also require a + // vtordisp. + llvm::SmallPtrSet Work; + llvm::SmallPtrSet BasesWithOverriddenMethods; + // Seed the working set with our non-destructor virtual methods. + for (const auto *I : RD->methods()) + if (I->isVirtual() && !isa(I)) + Work.insert(I); + while (!Work.empty()) { + const CXXMethodDecl *MD = *Work.begin(); + CXXMethodDecl::method_iterator i = MD->begin_overridden_methods(), + e = MD->end_overridden_methods(); + // If a virtual method has no-overrides it lives in its parent's vtable. + if (i == e) + BasesWithOverriddenMethods.insert(MD->getParent()); + else + Work.insert(i, e); + // We've finished processing this element, remove it from the working set. + Work.erase(MD); } - // Re-check all of our vbases for vtordisp requirements (in case their - // non-virtual bases have vtordisp requirements). + // For each of our virtual bases, check if it is in the set of overridden + // bases or if it transitively contains a non-virtual base that is. for (const auto &I : RD->vbases()) { const CXXRecordDecl *BaseDecl = I.getType()->getAsCXXRecordDecl(); if (!HasVtordispSet.count(BaseDecl) && - RequiresVtordisp(HasVtordispSet, BaseDecl)) + RequiresVtordisp(BasesWithOverriddenMethods, BaseDecl)) HasVtordispSet.insert(BaseDecl); } return HasVtordispSet; diff --git a/test/Layout/ms-x86-vtordisp.cpp b/test/Layout/ms-x86-vtordisp.cpp index 1f03b4ad44..1619d1cb3b 100644 --- a/test/Layout/ms-x86-vtordisp.cpp +++ b/test/Layout/ms-x86-vtordisp.cpp @@ -234,6 +234,9 @@ struct C : virtual B { int c; }; // CHECK-NEXT: 24 | int b // CHECK-NEXT: | [sizeof=28, align=4 // CHECK-NEXT: | nvsize=8, nvalign=4] +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout } namespace pragma_test2 { @@ -260,6 +263,9 @@ struct C : virtual B { int c; }; // CHECK-NEXT: 32 | int b // CHECK-NEXT: | [sizeof=36, align=4 // CHECK-NEXT: | nvsize=8, nvalign=4] +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout } namespace pragma_test3 { @@ -284,6 +290,9 @@ struct C : virtual B { int c; }; // CHECK-NEXT: 24 | int b // CHECK-NEXT: | [sizeof=28, align=4 // CHECK-NEXT: | nvsize=8, nvalign=4] +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout } namespace pragma_test4 { @@ -324,8 +333,54 @@ struct C : virtual B { int c; }; // CHECK-NEXT: 28 | int b // CHECK-NEXT: | [sizeof=32, align=4 // CHECK-NEXT: | nvsize=8, nvalign=4] +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout } +struct GA { + virtual void fun() {} +}; +struct GB: public GA {}; +struct GC: public virtual GA { + virtual void fun() {} + GC() {} +}; +struct GD: public virtual GC, public virtual GB {}; + +// CHECK: *** Dumping AST Record Layout +// CHECK: *** Dumping AST Record Layout +// CHECK: *** Dumping AST Record Layout +// CHECK: *** Dumping AST Record Layout +// CHECK-NEXT: 0 | struct GD +// CHECK-NEXT: 0 | (GD vbtable pointer) +// CHECK-NEXT: 4 | (vtordisp for vbase GA) +// CHECK-NEXT: 8 | struct GA (virtual base) +// CHECK-NEXT: 8 | (GA vftable pointer) +// CHECK-NEXT: 12 | struct GC (virtual base) +// CHECK-NEXT: 12 | (GC vbtable pointer) +// CHECK-NEXT: 16 | struct GB (virtual base) +// CHECK-NEXT: 16 | struct GA (primary base) +// CHECK-NEXT: 16 | (GA vftable pointer) +// CHECK-NEXT: | [sizeof=20, align=4 +// CHECK-NEXT: | nvsize=4, nvalign=4] +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64: *** Dumping AST Record Layout +// CHECK-X64-NEXT: 0 | struct GD +// CHECK-X64-NEXT: 0 | (GD vbtable pointer) +// CHECK-X64-NEXT: 12 | (vtordisp for vbase GA) +// CHECK-X64-NEXT: 16 | struct GA (virtual base) +// CHECK-X64-NEXT: 16 | (GA vftable pointer) +// CHECK-X64-NEXT: 24 | struct GC (virtual base) +// CHECK-X64-NEXT: 24 | (GC vbtable pointer) +// CHECK-X64-NEXT: 32 | struct GB (virtual base) +// CHECK-X64-NEXT: 32 | struct GA (primary base) +// CHECK-X64-NEXT: 32 | (GA vftable pointer) +// CHECK-X64-NEXT: | [sizeof=40, align=8 +// CHECK-X64-NEXT: | nvsize=8, nvalign=8] + int a[ sizeof(A)+ sizeof(C)+ @@ -335,4 +390,6 @@ sizeof(XC)+ sizeof(pragma_test1::C)+ sizeof(pragma_test2::C)+ sizeof(pragma_test3::C)+ -sizeof(pragma_test4::C)]; +sizeof(pragma_test4::C)+ +sizeof(GD)+ +0];