> "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 > >> 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
