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

[Betul Buyukkurt]

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: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.