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

>>
>> // If the body is a function-try-block, enter the try before
>> // anything else.
>> bool isTryBody = (Body && isa<CXXTryStmt>(Body));
>> if (isTryBody)
>>   EnterCXXTryStmt(*cast<CXXTryStmt>(Body), true);
>> EmitAsanPrologueOrEpilogue(false);
>
> Here. After the early return when delegating is possible, and inside the
> try body if there is one.
>
>>
>> // Enter the epilogue cleanups.
>> RunCleanupsScope DtorEpilogue(*this);
>> // If this is the complete variant, just invoke the base variant;
>> // the epilogue will destruct the virtual bases.  But we can't do
>> // this optimization if the body is a function-try-block, because
>> // we'd introduce *two* handler blocks.  In the Microsoft ABI, we
>> // always delegate because we might not have a definition in this TU.
>> switch (DtorType) {
>