"Betul Buyukkurt" <[email protected]> writes: > Hi Justin, > > I do not have commit rights. Could you please direct how I should commit or > commit the change on my behalf?
r237804. > Thanks, > -Betul > > -----Original Message----- > From: Justin Bogner [mailto:[email protected]] On Behalf Of Justin > Bogner > Sent: Tuesday, May 19, 2015 11:05 PM > To: Betul Buyukkurt > Cc: [email protected]; [email protected]; > [email protected]; > [email protected] > Subject: Re: [PATCH] -Wprofile-instr-out-of-date warnings due to certain > destructor types not being assigned increment intrinsics > > "Betul Buyukkurt" <[email protected]> writes: >>> "Betul Buyukkurt" <[email protected]> writes: >>>>> Betul Buyukkurt <[email protected]> writes: >>>>>> Hi bogner, dsule, davidxl, >>>>>> >>>>>> -fprofile-instr-generate does not emit counter increment >>>>>> intrinsics for Dtor_Deleting and Dtor_Complete destructors with >>>>>> assigned counters. This causes unnecessary >>>>>> [-Wprofile-instr-out-of-date] warnings during profile-use runs >>>>>> even if the source has never been modified since profile collection. >>>>>> >>>>>> http://reviews.llvm.org/D9861 >>>>>> >>>>>> Files: >>>>>> lib/CodeGen/CGClass.cpp >>>>>> test/Profile/cxx-virtual-destructor-calls.cpp >>>>>> >>>>>> Index: lib/CodeGen/CGClass.cpp >>>>>> ================================================================== >>>>>> = >>>>>> --- lib/CodeGen/CGClass.cpp >>>>>> +++ lib/CodeGen/CGClass.cpp >>>>>> @@ -1366,6 +1366,10 @@ >>>>>> const CXXDestructorDecl *Dtor = >>>>>> cast<CXXDestructorDecl>(CurGD.getDecl()); >>>>>> CXXDtorType DtorType = CurGD.getDtorType(); >>>>>> >>>>>> + Stmt *Body = Dtor->getBody(); >>>>>> + if (Body) >>>>>> + incrementProfileCounter(Body); >>>>>> + >>>>> >>>>> I think it makes more sense to do this after the delegation and >>>>> entering the try body, like we do in constructors, no? >>>> >>>> I've been observing plenty warnings emitted from C++ codes. Tracking >>>> the issue I saw that the Dtor_Deleting and other Dtor types have >>>> counters assigned to them but no counter increment code is emitted. >>>> This causes no profile data structures to be emitted for those >>>> functions as well. In CodeGenPGO.cpp line 649, it says that: >>>> >>>> " Clang emits several functions for the constructor and the >>>> destructor of a class. Every function is instrumented, but we only >>>> want to provide coverage for one of them. Because of that we only >>>> emit the coverage mapping for the base constructor/destructor. " >>>> >>>> I also see that the counter increment is currently done for base >>>> destructor only. This patch removes all the >>>> [-Wprofile-instr-out-of-date] warnings from the spec2000/2006 >>>> benchmarks. >>> >>> Well, yes. You explained that in the commit message. What I mean is, >>> I think this belongs slightly later in the function to match how we >>> do it in the constructor, ie: >>> >>> from CodeGenFunction::EmitDestructorBody(FunctionArgList &Args): >>>> // The call to operator delete in a deleting destructor happens // >>>> outside of the function-try-block, which means it's always // >>>> possible to delegate the destructor body to the complete // >>>> destructor. Do so. >>>> if (DtorType == Dtor_Deleting) { >>>> EnterDtorCleanups(Dtor, Dtor_Deleting); >>>> EmitCXXDestructorCall(Dtor, Dtor_Complete, /*ForVirtualBase=*/false, >>>> /*Delegating=*/false, LoadCXXThis()); >>>> PopCleanupBlock(); >>>> return; >>>> } >> >> Dtor_Deleting returns here. So the increment also has to be inside the >> above if statement, if the other location were to be below where you >> proposed. The above location would diminish the code duplication. > > Hm, I guess I wasn't thinking clearly - in the case where we delegate the > call, we should still instrument the calling function. Your change is > correct - feel free to commit. > > However, could you please look at EmitConstructorBody as well? It does not > emit a counter increment today in the case where it delegates from the > complete constructor to the base constructor. I suspect that's another > instance of this same problem. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
