Solution for memory leak in tblgen

[wuhui1973]

Yes, at first, I thought of std::shared_ptr. But it is for general use, you can new an object but not hand it to shared_ptr,
then you may lose it forever! The very error-prone methods are TreePatternNode::Clone & TreePatternNode::InlinePatternFragments,
it is impossible to avoid misusing by someone.
When fix like that submitted, it is possible other people unware of it, and code as he/she
before, then make memory leak again.


With this hand-made ref counted class, TreePatternNode::Clone & TreePatternNode::InlinePatternFragments won't be
the pitfall anymore. The compiler will force you to assign the return value to the ref counted class. 


Besides, shared_ptr provides "explicit T* operator()", which makes it less visible, and I am always afraid this convertor 
will give us surprise someday. For example, the referred object is gone as someone forget to use shared_ptr to lock it
unwarly (that is why I don't define this operator in the hand-made ref counted class).


So I select hand-made ref counted class because it can enforce the rule by giving compiling error if you break it.
that will make the system more robustness.



At 2014-07-28 08:27:48, "Anton Korobeynikov" <anton at korobeynikov.info> wrote:
>Why can't you use std::shared_ptr (possibly with custom deleter)
>instead of hand-made ref counted class?
>
>On Mon, Jul 28, 2014 at 12:53 PM, wuhui1973 <wuhui1973 at 163.com> wrote:
>> Hello Hal, Andy, Nadav, Tom & all:
>>
>> I have worked out a solution for the memory leak problem in tblgen. This
>> memory leak problem is known and deliberatly left by the author for later
>> fix.
>>
>> My solution follows the author's suggestion - using reference counter to
>> track the TreePatternNode & TreePattern objects.
>>
>> Besides, though TreePatternNodes form DAGs, but with TreePattern, they can
>> form cyclic graph, the solution also includes
>>
>> a method to break the cyclic reference. The solution detail please refer to
>> the attachment.
>>
>> I have checked the solution with following actions:
>>
>> 1> Check tblgen running log, it looks like this:
>>
>>      llvm[3]: Building X86.td register info implementation with tblgen
>>      llvm[3]: Building X86.td instruction information with tblgen
>>     1. NumTPNAlloc: 84347, NumTPNRel: 20190            <--- TPN:
>> TreePatternNode
>>     1. NumTPAlloc: 13441, NumTPRel: 9215                  <--- TP:
>> TreePattern
>>     2. NumTPNAlloc: 84347, NumTPNRel: 83879
>>     2. NumTPAlloc: 13441, NumTPRel: 13271
>>     468 TPNs remain, 170 TPs remain                            <--- These
>> are cyclic referred nodes
>>     Release done
>>     3. NumTPNAlloc: 84347, NumTPNRel: 84347
>>     3. NumTPAlloc: 13441, NumTPRel: 13441
>>
>>     Has confirmed all allocated objects get released during the loadbuild
>> for all supported targets.
>>
>> 2> Compare the generated .inc file with those generated by sainty code. Has
>> confirmed they all the same.
>>
>> 3> run "make check" to verify the generated llvm-programs, gets following
>> result:
>> It contains one error, but the sainty code also gives the same result! I
>> will raise the bug report later.
>> FAIL: LLVM :: ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s
>> (6206 of 11281)
>> ******************** TEST 'LLVM ::
>> ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s' FAILED
>> ********************
>> Script:
>> --
>> /sources/new/build_llvm/Debug+Asserts/bin/llvm-mc
>> -triple=arm64-apple-ios7.0.0 -code-model=small -relocation-model=pic
>> -filetype=obj -o
>> /sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o
>> /sources/new/llvm/test/ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s
>> sed
>> "s,<filename>,/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o,g"
>> /sources/new/llvm/test/ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s
>>>
>> /sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.s
>> /sources/new/build_llvm/Debug+Asserts/bin/llvm-rtdyld
>> -triple=arm64-apple-ios7.0.0 -verify
>> -check=/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.s
>> /sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o
>> --
>> Exit Code: 1
>>
>> Command Output (stderr):
>> --
>> Expression 'decode_operand(adrp2, 1) =
>> (stub_addr(/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o,
>> __text, _ptr)[32:12] - adrp2[32:12])' is false: 0xfff00000 != 0x0
>> /sources/new/build_llvm/Debug+Asserts/bin/llvm-rtdyld: error: some checks in
>> '/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.s'
>> failed
>> --
>> ********************
>> Testing Time: 3155.15s
>> ********************
>> Failing Tests (1):
>>     LLVM :: ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s
>>
>>   Expected Passes    : 11144
>>   Expected Failures  : 101
>>   Unsupported Tests  : 35
>>   Unexpected Failures: 1
>>
>> So I think my solution is correct. Could you review it? Or help to forward
>> to someone concerned? Thanks!
>>
>> Regards
>> Hui Wu
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
>
>-- 
>With best regards, Anton Korobeynikov
>Faculty of Mathematics and Mechanics, Saint Petersburg State University
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140729/62cedb23/attachment.html>