[PATCH] -Wprofile-instr-out-of-date warnings due to certain destructor types not being assigned increment intrinsics

[Justin Bogner]

"Betul Buyukkurt" <betulb at codeaurora.org> 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:justin at justinbogner.com] On Behalf Of Justin
> Bogner
> Sent: Tuesday, May 19, 2015 11:05 PM
> To: Betul Buyukkurt
> Cc: dsule at codeaurora.org; davidxl at google.com;
> reviews+d9861+public+a0e96a4b284ffb06 at reviews.llvm.org;
> cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] -Wprofile-instr-out-of-date warnings due to certain
> destructor types not being assigned increment intrinsics
>
> "Betul Buyukkurt" <betulb at codeaurora.org> writes:
>>> "Betul Buyukkurt" <betulb at codeaurora.org> writes:
>>>>> Betul Buyukkurt <betulb at codeaurora.org> 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.