Hi Justin, I do not have commit rights. Could you please direct how I should commit or commit the change on my behalf?
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
