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