[PATCH] Bury leaked pointers in a global array to silence a leak detector in --disable-free mode

[Kostya Serebryany]

On Fri, Dec 27, 2013 at 11:44 AM, Kostya Serebryany <kcc at google.com> wrote:

>
>
>
> On Fri, Dec 27, 2013 at 11:39 AM, Chandler Carruth <chandlerc at gmail.com>wrote:
>
>>
>> On Fri, Dec 27, 2013 at 2:10 AM, Kostya Serebryany <kcc at google.com>wrote:
>>
>>> +  // This function may be called only a small fixed amount of times per
>>> each
>>> +  // invocation, otherwise we do actually have a leak which we want to
>>> report.
>>> +  // If this function is called more than kGraveYardMaxSize times, the
>>> pointers
>>> +  // will not be properly buried and a leak detector will report a
>>> leak, which
>>> +  // is what we want in such case.
>>>
>>
>> Interesting. I didn't realize it was going to be *that* tightly bounded.
>>
>> This makes me wonder if the whole disable free thing should just be
>> removed at this point.
>>
> I am all for it, --disable-free looks like a hack and we did not see any
> compile-time loss from removing it (measure on bootstrap).
>

I take these my words back. Building 483.xalancbmk with -O0 (make -j32)
shows around 0.5%-1% compile-time difference:
with -disable-free :
TIME: real: 8.140; user: 177.970; system: 13.180
TIME: real: 8.070; user: 177.940; system: 13.090
TIME: real: 8.103; user: 177.960; system: 13.410
TIME: real: 8.122; user: 178.110; system: 13.180
TIME: real: 8.093; user: 177.660; system: 13.210
without -disable-free :
TIME: real: 8.140; user: 179.750; system: 13.260
TIME: real: 8.113; user: 178.770; system: 13.710
TIME: real: 8.101; user: 179.830; system: 13.180
TIME: real: 8.262; user: 180.160; system: 12.760
TIME: real: 8.125; user: 179.510; system: 13.070

0.5% may be worth this relatively innocent hack.



>
>
>> Back in the day, we didn't so carefully use a BumpPtrAllocator in the
>> ASTContext. Today, we might be fine to call free 10 times.
>>
>
> But this is not 10 calls to free. This is 10-ish calls to delete, where
> the DTOR destructs other objects and so on. It may call much more than 10
> frees.
> Still, see above.
>
>
>> Anyways, not trying to shift the goal posts; I mostly wanted to mention
>> it in case someone gets some time to look into removing all of the leak
>> stuff.
>>
>>
>>> +  static const size_t kGraveYardMaxSize = 16;
>>> +  static const void *GraveYard[kGraveYardMaxSize];
>>> +  static llvm::sys::cas_flag GraveYardSize;
>>>
>>
>> Do these being function-local statics defeat the purpose of using a basic
>> atomic increment? I can't recall if we correctly eliminate the cxa_guard in
>> these cases...
>>
> We will not have  cxa_guard here because there is no dynamic init -- both
> objects (the size and the array) are linker-initialized.
> Atomic increment is here for the future case when clang becomes
> multi-threaded.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131227/313a958c/attachment.html>