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

[Betul Buyukkurt]

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

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