GNOME Bugzilla – Bug 794979
libvala: leaks in flow analyzer
Last modified: 2018-04-12 07:11:12 UTC
Created attachment 370527 [details] libvalaleak.vala Running the flow analyzer on a context results in memory being leaked when the context is freed. The attached vala application demonstrates this issue. If you run this with the following: G_DEBUG=gc-friendly G_SLICE=always-malloc G_MESSAGES_DEBUG=all valgrind --leak-check=full --leak-resolution=high --num-callers=100 ./libvalaleak libvalaleak.vala The following leak summary is produced: ==13310== LEAK SUMMARY: ==13310== definitely lost: 560 bytes in 4 blocks ==13310== indirectly lost: 297,564 bytes in 4,086 blocks ==13310== possibly lost: 4,416 bytes in 64 blocks ==13310== still reachable: 25,141 bytes in 57 blocks ==13310== of which reachable via heuristic: ==13310== length64 : 968 bytes in 23 blocks ==13310== newarray : 1,616 bytes in 21 blocks ==13310== suppressed: 73,504 bytes in 883 blocks ==13310== Reachable blocks (those to which a pointer was found) are not shown. ==13310== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==13310== ==13310== For counts of detected and suppressed errors, rerun with: -v ==13310== ERROR SUMMARY: 68 errors from 68 contexts (suppressed: 35 from 35) More iterations of the loop produce more leaks. If the flow analyzer line is removed from the code, there are no leaks. Using the flow analyzer several times on a context containing a lot of Vala code can quite easily produce leaks of gigabytes. Valgrind shows that the memory lost is mostly CodeNodes like IfStatements etc... I believe this is due to circular references within the Vala.BasicBlock class. Using the following python gdb script, we can test the ref/unref count of the Vala.BasicBlocks: ############################################### instances = [] ref = 0 unref = 0 class InstanceFinder (gdb.Breakpoint): def stop (self): global instances global ref instances.append(gdb.selected_frame().read_var("self")) ref+=1 return False class MyBreakpoint (gdb.Breakpoint): def stop (self): global instances global ref global unref if gdb.selected_frame().read_var("instance") in instances: if "_unref" in gdb.selected_frame().name(): unref+=1 if "_ref" in gdb.selected_frame().name(): ref+=1 return False InstanceFinder ("vala_basic_block_instance_init") MyBreakpoint("vala_basic_block_ref") MyBreakpoint("vala_basic_block_unref") def exit_handler (event): print ("ref: %d" % ref) print ("unref: %d" % unref) gdb.events.exited.connect (exit_handler) ################################################## david@david-XPS-13-9343:~/Documents/Projects$ gdb -q -x ref_unref.py ./libvalaleak Reading symbols from ./libvalaleak...(no debugging symbols found)...done. Function "vala_basic_block_instance_init" not defined. Breakpoint 1 (vala_basic_block_instance_init) pending. Function "vala_basic_block_ref" not defined. Breakpoint 2 (vala_basic_block_ref) pending. Function "vala_basic_block_unref" not defined. Breakpoint 3 (vala_basic_block_unref) pending. (gdb) r libvalaleak.vala Starting program: /home/david/Documents/Projects/libvalaleak libvalaleak.vala [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". [Inferior 1 (process 15319) exited normally] ref: 1332 unref: 1074 #################################################### Looking at valabasicblock.vala, there are a few places where circular references could occur. They hold a list of CodeNodes and there are CodeNodes that reference BasicBlocks, for example Methods via the subroutine interface. It looks as though the FlowAnalyzer class creates a circular reference for every method as it adds a BasicBlock to the Method and then connects the BasicBlock to another BasicBlock that references the original method. It looks as though the dominator tree in BasicBlock also has the potential to create circular references with the concept of parents and children, none of which are weak references. I've had a go at fixing this by making some of the references I suspect to be problematic into weak references, and it definitely improves the ref/unref ratio as tested by the python script above, but I haven't been able to solve the memory leak. I hope I have provided enough information to assist with tracking this down, if not, please let me know and I will be happy to help out where I can.
Created attachment 370544 [details] [review] vala: Fix return-type of BasicBlock.get_successors()
Created attachment 370545 [details] [review] vala: Avoid and break some cyclic references in BasicBlock
Created attachment 370546 [details] [review] vala: Check for matching ownership of type-arguments
Comment on attachment 370544 [details] [review] vala: Fix return-type of BasicBlock.get_successors() Attachment 370544 [details] pushed as f5a12ac - vala: Fix return-type of BasicBlock.get_successors()
(In reply to Rico Tzschichholz from comment #2) > Created attachment 370545 [details] [review] [review] > vala: Avoid and break some cyclic references in BasicBlock I've not had chance to test these patches yet, but is it correct to prevent some of these methods from adding "this" to the lists? From what I understand, this is building a flow graph of the code, and it's usually valid for nodes in a flow graph to connect back to themselves. For example, a while loop can have two successors, one being itself, when the loop jumps back to the top, and then the other being the code blocks following the loop. Equally, this successors array could still be causing reference cycles indirectly. Node A could connect to Node B and then Node B could connect back to Node A. I believe you could produce that flow graph with a combination of loops and flow control statements. The only way I can think of to break those kind of loops is having all of the graph references as weak references and then having a flat structure to hold the strong references to all of the BasicBlocks. This would then get cleaned up when the flow analyzer or context goes out of scope. I'll do some more testing/debugging on this tonight and see if I can come up with anything else that might be useful.
(In reply to David Hewitt from comment #5) > (In reply to Rico Tzschichholz from comment #2) > > Created attachment 370545 [details] [review] [review] [review] > > vala: Avoid and break some cyclic references in BasicBlock > > I've not had chance to test these patches yet, but is it correct to prevent > some of these methods from adding "this" to the lists? No, this patch is not correct and your are right that this can't be done like that. > The only way I can think of to break those kind of loops is having all of > the graph references as weak references and then having a flat structure to > hold the strong references to all of the BasicBlocks. This would then get > cleaned up when the flow analyzer or context goes out of scope. > > I'll do some more testing/debugging on this tonight and see if I can come up > with anything else that might be useful. Thanks for looking into it.
Review of attachment 370546 [details] [review]: This shows a serious memory leak issue while there were no errors/warnings so far. It affects several projects. http://paldo.org:8010/builders/vala-staging/builds/493 With a less stricter version of that patch http://paldo.org:8010/builders/vala-staging/builds/494
Created attachment 370558 [details] [review] break cyclic references patch I can confirm that the attached patch breaks all of the cyclic references and fixes all of the memory leaks in the flow analyzer. Just needs some thorough testing.
Is it possible to have a vala-staging buildbot run for my above patch to see if it causes any regressions? I've run all the tests in the source tree and recompiled Vala with that patch applied and it all seems good so far.
Yes, this would be possible.
http://paldo.org:8010/builders/vala-staging/builds/495