After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794979 - libvala: leaks in flow analyzer
libvala: leaks in flow analyzer
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: 0.42
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-04 16:09 UTC by David Hewitt
Modified: 2018-04-12 07:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
libvalaleak.vala (805 bytes, text/x-vala)
2018-04-04 16:09 UTC, David Hewitt
  Details
vala: Fix return-type of BasicBlock.get_successors() (733 bytes, patch)
2018-04-05 06:46 UTC, Rico Tzschichholz
committed Details | Review
vala: Avoid and break some cyclic references in BasicBlock (1.46 KB, patch)
2018-04-05 06:46 UTC, Rico Tzschichholz
none Details | Review
vala: Check for matching ownership of type-arguments (2.30 KB, patch)
2018-04-05 06:46 UTC, Rico Tzschichholz
none Details | Review
break cyclic references patch (9.25 KB, patch)
2018-04-05 17:13 UTC, David Hewitt
committed Details | Review

Description David Hewitt 2018-04-04 16:09:10 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.
Comment 1 Rico Tzschichholz 2018-04-05 06:46:15 UTC
Created attachment 370544 [details] [review]
vala: Fix return-type of BasicBlock.get_successors()
Comment 2 Rico Tzschichholz 2018-04-05 06:46:22 UTC
Created attachment 370545 [details] [review]
vala: Avoid and break some cyclic references in BasicBlock
Comment 3 Rico Tzschichholz 2018-04-05 06:46:29 UTC
Created attachment 370546 [details] [review]
vala: Check for matching ownership of type-arguments
Comment 4 Rico Tzschichholz 2018-04-05 10:16:58 UTC
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()
Comment 5 David Hewitt 2018-04-05 10:23:42 UTC
(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.
Comment 6 Rico Tzschichholz 2018-04-05 12:58:51 UTC
(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.
Comment 7 Rico Tzschichholz 2018-04-05 13:01:26 UTC
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
Comment 8 David Hewitt 2018-04-05 17:13:39 UTC
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.
Comment 9 David Hewitt 2018-04-07 19:15:59 UTC
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.
Comment 10 Rico Tzschichholz 2018-04-07 19:37:57 UTC
Yes, this would be possible.
Comment 11 Rico Tzschichholz 2018-04-07 20:24:47 UTC
http://paldo.org:8010/builders/vala-staging/builds/495