ping On Mon, Sep 12, 2016 at 5:20 PM David Blaikie <dblai...@gmail.com> wrote:
> On Mon, Sep 12, 2016 at 5:09 PM Reid Kleckner via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > Author: rnk > Date: Mon Sep 12 19:01:23 2016 > New Revision: 281278 > > URL: http://llvm.org/viewvc/llvm-project?rev=281278&view=rev > Log: > [DebugInfo] Deduplicate debug info limiting logic > > We should be doing the same checks when a type is completed as we do > when a complete type is used during emission. Previously, we duplicated > the logic, and it got out of sync. This could be observed with > dllimported classes. > > > Thanks for having a go at this! > > The logic in completeType (& maybe also completeClassData) still seems to > duplicate some of the logic in shouldOmitDefinition - I think the original > idea I had was a series of cascading entry points (completeType checking > the "oh, the type now has a definition", then falling into > completeRequiredType, etc) > > In theory the "shouldOmitDefinition" could be split into the extra > conditions for each of the parts of that cascade, so the checks wouldn't be > redundantly executed when we were already being called about the completion > or required completion of a type, etc. But I'm not sure the performance > gain would be measurable, etc. > > If not, we might as well just have one entry point that's something like > "revisit type because something interesting happened to it" (it became > complete, it became required to be complete, etc)... but that seems a bit > weird too. > > > > Also reduce a test case for this slightly. > > > The UseCompleteType is still there/unnecessary (as is the 'private' in > UnicodeString) - and I'd probably rename it to foo/bar and throw it in a > namespace like the rest for consistency - having semantic names in a test > when they're just remnants from whatever code it was reduced from seems > confusing to me. But I realize I'm a bit more pedantic about that than most. > > - Dave > > > > Implementing review feedback from David Blaikie on r281057. > > Modified: > cfe/trunk/lib/CodeGen/CGDebugInfo.cpp > cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp > cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp > > Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=281278&r1=281277&r2=281278&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Sep 12 19:01:23 2016 > @@ -1644,27 +1644,6 @@ void CGDebugInfo::completeType(const Rec > completeRequiredType(RD); > } > > -void CGDebugInfo::completeRequiredType(const RecordDecl *RD) { > - if (DebugKind <= codegenoptions::DebugLineTablesOnly) > - return; > - > - // If this is a dynamic class and we're emitting limited debug info, > wait > - // until the vtable is emitted to complete the class debug info. > - if (DebugKind <= codegenoptions::LimitedDebugInfo) { > - if (const auto *CXXDecl = dyn_cast<CXXRecordDecl>(RD)) > - if (CXXDecl->isDynamicClass()) > - return; > - } > - > - if (DebugTypeExtRefs && RD->isFromASTFile()) > - return; > - > - QualType Ty = CGM.getContext().getRecordType(RD); > - llvm::DIType *T = getTypeOrNull(Ty); > - if (T && T->isForwardDecl()) > - completeClassData(RD); > -} > - > void CGDebugInfo::completeClassData(const RecordDecl *RD) { > if (DebugKind <= codegenoptions::DebugLineTablesOnly) > return; > @@ -1763,6 +1742,16 @@ static bool shouldOmitDefinition(codegen > return false; > } > > +void CGDebugInfo::completeRequiredType(const RecordDecl *RD) { > + if (shouldOmitDefinition(DebugKind, DebugTypeExtRefs, RD, > CGM.getLangOpts())) > + return; > + > + QualType Ty = CGM.getContext().getRecordType(RD); > + llvm::DIType *T = getTypeOrNull(Ty); > + if (T && T->isForwardDecl()) > + completeClassData(RD); > +} > + > llvm::DIType *CGDebugInfo::CreateType(const RecordType *Ty) { > RecordDecl *RD = Ty->getDecl(); > llvm::DIType *T = cast_or_null<llvm::DIType>(getTypeOrNull(QualType(Ty, > 0))); > > Modified: cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp?rev=281278&r1=281277&r2=281278&view=diff > > ============================================================================== > --- cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp (original) > +++ cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp Mon Sep 12 > 19:01:23 2016 > @@ -6,10 +6,7 @@ > // more general than that. > > struct UnicodeString; > -struct GetFwdDecl { > - static UnicodeString format; > -}; > -GetFwdDecl force_fwd_decl; > +UnicodeString *force_fwd_decl; > struct UnicodeString { > private: > virtual ~UnicodeString(); > > Modified: cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp?rev=281278&r1=281277&r2=281278&view=diff > > ============================================================================== > --- cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp > (original) > +++ cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp Mon Sep > 12 19:01:23 2016 > @@ -4,6 +4,10 @@ > // be imported from a DLL. Otherwise, the debugger wouldn't be able to > show the > // members. > > +// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: > "ImportedAfterCompletion", > +// CHECK-NOT: DIFlagFwdDecl > +// CHECK-SAME: ){{$}} > + > // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: > "OutOfLineCtor", > // CHECK-SAME: DIFlagFwdDecl > // CHECK-SAME: ){{$}} > @@ -16,6 +20,13 @@ > // CHECK-NOT: DIFlagFwdDecl > // CHECK-SAME: ){{$}} > > + > +struct ImportedAfterCompletion; > +ImportedAfterCompletion *force_fwd_decl; > +struct __declspec(dllimport) ImportedAfterCompletion { > + virtual ~ImportedAfterCompletion(); > +}; > + > struct OutOfLineCtor { > OutOfLineCtor(); > virtual void Foo(); > @@ -35,6 +46,7 @@ struct ImportedMethod { > }; > > int main() { > + ImportedAfterCompletion c; > OutOfLineCtor o; > DerivedFromImported d; > ImportedMethod m; > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
