Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Memory leak when using the @disable macro #419

Open
danielhollas opened this issue Apr 16, 2023 · 5 comments
Open

Memory leak when using the @disable macro #419

danielhollas opened this issue Apr 16, 2023 · 5 comments

Comments

@danielhollas
Copy link
Contributor

It looks like there is a memory leak when I used a @disable macro annotating the pFUnit test case

    @test
    @disable
    subroutine test_disabled()
       ! This test needs to be skipped
       ...
    end subroutine

Here's the output I got when compiling and running my test suite with gfortran and -fsanitize=address,leak


=================================================================
==2109==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x7fd3d5e56808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x55c76536db53 in __pf_stringtestannotationmap_MOD_ti_set_capacity /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:639
make[1]: Leaving directory '/home/runner/work/ABIN/ABIN/unit_tests'
    #2 0x55c76536d9f7 in __pf_stringtestannotationmap_MOD_ti_grow_to /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:664
    #3 0x55c76536e522 in __pf_stringtestannotationmap_MOD_ti_push_back /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:401
    #4 0x55c765378b92 in __pf_stringtestannotationmap_MOD_s_insert /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/altSet_impl.inc:347
    #5 0x55c765375f03 in __pf_stringtestannotationmap_MOD_m_insert_key_value /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/map_impl.inc:84
    #6 0x55c764ebf34a in test_spline_suite_ (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x46b34a)
    #7 0x55c764eba753 in __loader_MOD_load_tests (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x466753)
    #8 0x55c7653bc2d3 in __funit_MOD_generic_run /home/runner/pfunit/src/funit/FUnit.F90:111
    #9 0x55c7653c558a in __funit_MOD_run /home/runner/pfunit/src/funit/FUnit.F90:33
    #10 0x55c7653c55eb in funit_main_ /home/runner/pfunit/src/funit/funit_main.F90:16
    #11 0x55c764ebb1bd in MAIN__ (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x4671bd)
    #12 0x55c764ebb20b in main (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x46720b)
    #13 0x7fd3d4b57082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x7fd3d5e56808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x55c76536db53 in __pf_stringtestannotationmap_MOD_ti_set_capacity /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:639
    #2 0x55c76536d9f7 in __pf_stringtestannotationmap_MOD_ti_grow_to /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:664
    #3 0x55c76536e522 in __pf_stringtestannotationmap_MOD_ti_push_back /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:401
    #4 0x55c765378bd7 in __pf_stringtestannotationmap_MOD_s_insert /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/altSet_impl.inc:348
    #5 0x55c765375f03 in __pf_stringtestannotationmap_MOD_m_insert_key_value /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/map_impl.inc:84
    #6 0x55c764ebf34a in test_spline_suite_ (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x46b34a)
    #7 0x55c764eba753 in __loader_MOD_load_tests (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x466753)
    #8 0x55c7653bc2d3 in __funit_MOD_generic_run /home/runner/pfunit/src/funit/FUnit.F90:111
    #9 0x55c7653c558a in __funit_MOD_run /home/runner/pfunit/src/funit/FUnit.F90:33
    #10 0x55c7653c55eb in funit_main_ /home/runner/pfunit/src/funit/funit_main.F90:16
    #11 0x55c764ebb1bd in MAIN__ (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x4671bd)
    #12 0x55c764ebb20b in main (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x46720b)
    #13 0x7fd3d4b57082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x7fd3d5e56808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x55c76536db53 in __pf_stringtestannotationmap_MOD_ti_set_capacity /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:639
    #2 0x55c76536d9f7 in __pf_stringtestannotationmap_MOD_ti_grow_to /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:664
    #3 0x55c76536e522 in __pf_stringtestannotationmap_MOD_ti_push_back /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/vector_impl.inc:401
    #4 0x55c765378b4d in __pf_stringtestannotationmap_MOD_s_insert /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/altSet_impl.inc:346
    #5 0x55c765375f03 in __pf_stringtestannotationmap_MOD_m_insert_key_value /home/runner/pfunit/build/extern/fArgParse/extern/gFTL-shared/extern/gFTL/include/templates/map_impl.inc:84
    #6 0x55c764ebf34a in test_spline_suite_ (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x46b34a)
    #7 0x55c764eba753 in __loader_MOD_load_tests (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x466753)
    #8 0x55c7653bc2d3 in __funit_MOD_generic_run /home/runner/pfunit/src/funit/FUnit.F90:111
    #9 0x55c7653c558a in __funit_MOD_run /home/runner/pfunit/src/funit/FUnit.F90:33
    #10 0x55c7653c55eb in funit_main_ /home/runner/pfunit/src/funit/funit_main.F90:16
    #11 0x55c764ebb1bd in MAIN__ (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x4671bd)
    #12 0x55c764ebb20b in main (/home/runner/work/ABIN/ABIN/unit_tests/spline+0x46720b)
    #13 0x7fd3d4b57082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 3 allocation(s).

The leak is probably not so serious per se, but it is annoying in practice if I want to run my test suite with the memory leak detection enabled since it produces false positives. In effect, I am not able to use the @disable macro in my CI.
@danielhollas danielhollas mentioned this issue Apr 16, 2023
Merged
@tclune
Copy link
Member

tclune commented Apr 16, 2023

To be honest, I'm a bit (pleasantly) surprised that you are not seeing a modest leak for the non-disabled case. Been a while since I looked into the @disable logic, but will try to see if I can spot anything obvious.

@tclune
Copy link
Member

tclune commented Apr 16, 2023

OK - I think I have at least a glimmer as to what is going on now, but I think it is the compiler that is likely at fault. I took a shortcut when introducing "test annotations" of which Disable is the main use. Each test is itself a dictionary of annotations. (My shortcut was to use inheritance (is-a) rather than composition (has-a).) But the dictionary itself uses allocatable components and should be deallocated when the test goes out of scope. For cases without Disable, the dictionary remains empty.

The problem with my theory is that there is no guarantee that another implementation would solve the problem, and none of the approaches I've contemplated are quite trivial. For my own notes, I'll list what I've thought of so far:

  1. Add a clear() method to the Test class hierarchy and make sure that it is called in the driver. High probability of solving the memory leak, but rather a step in the wrong direction in terms of basic design.
  2. Upgrade the dictionary to from my v1 implementation to v1. A useful thing on its own, but not very likely to change the results.
  3. Switch things so that the Test class "has-a" dictionary of annotations. This would improve the design from a loose-coupling perspective, but probably has other design implications that I'm not immediately seeing.
  4. Just realized a variant on (1) Just make the top most TestSuite object in the driver allocatable, and deallocate it at the end. Much easier than(1), does not really impact design. Might leave the internal dictionary as a leak, as I still think this is a compiler "bug".

I'll try to attempt (4) next weekend.

@tclune
Copy link
Member

tclune commented Apr 30, 2023

Apologies for the delay in following up. I took some downtime last weekend and jumped back in this weekend. Unfortunately there is something wrong with the gfortran configuration on my laptop, as I was unable to build pFUnit. (CI tests say all is good, so?) So went to build on our computing center cluster instead, and ... somehow I'm locked out.

Hopefully one or both of these issues are resolved quickly tomorrow morning and I can attempt some of the workarounds I mentioned previously.

tclune added a commit that referenced this issue May 1, 2023
@tclune
Fixes #419 - memory leak.
551a018
@tclune
Copy link
Member

tclune commented May 1, 2023

OK - am now in an environment where I can reproduce the issue. Amusingly the first few things I tried actually made the leak slightly worse.

Switching to the newer implementation of my poor-man STL container analogs seems to have eliminated the leaks. But while I was at it, I went ahead and switched the treatment of annotations to "has-a" from "is-a". That was easier than I thought it would be, and is generally cleaner code.

Running CI for merge into develop branch now.

@tclune
Copy link
Member

tclune commented May 2, 2023

Alas, my changes apparently break Intel in a way that I cannot readily find a workaround. Some sort of memory corruption so that it ends up with invalid pointers to individual tests, but some tests run through without issue.

I'm actually a bit surprised because I figured that a shallower inheritance chain would be less stressful for the compiler.

It also broke NAG in a somewhat surprising way, but there I was able to find a simple workaround.

tclune added a commit that referenced this issue Mar 5, 2024
@tclune
Merge branch 'develop' into feature/#419-memory-leak
a53f808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tclune @danielhollas