[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:
>> 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;
> }
>
> // 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) {