Solution for memory leak in tblgen

Mon Jul 28 19:07:00 PDT 2014

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.

In fact, I am a newer to Tablgen, with this hand-made ref counted class, the compiler can find out most of places need to
be modified. By my experience, its capability to highlight the most possible rule breaking points make the solution figure 
out easier, and introduces less errors during develop.

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/b854e542/attachment.html>