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.

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

Fix memory leaks due to double allocations on pointers #1160

Merged

Conversation

kuanchihwang
Copy link
Contributor

@kuanchihwang kuanchihwang commented Apr 12, 2024

This PR fixes memory leaks due to duplicate allocate calls on pointers in the file src/core_atmosphere/inc/structs_and_variables.inc. This file is auto-generated and then included by src/core_atmosphere/mpas_atm_core_interface.F (among others) at compile time. No functional changes (NFC) are introduced by this PR.

For example, in the auto-generated file src/core_atmosphere/inc/structs_and_variables.inc, there are code snippets similar to the following:

subroutine atm_generate_pool_mesh(block, structPool, dimensionPool, packagePool)

! ... (Unrelated lines omitted) ...

      type(mpas_pool_type), pointer :: newSubPool

! ... (Unrelated lines omitted) ...

      allocate(newSubPool)
      call mpas_pool_create_pool(newSubPool)
      call mpas_pool_add_subpool(structPool, 'mesh', newSubPool)
      call mpas_pool_add_subpool(block % allStructs, 'mesh', newSubPool)

! ... (Unrelated lines omitted) ...

end subroutine atm_generate_pool_mesh

The problem is that the mpas_pool_create_pool subroutine already allocates the pointer in its arguments internally. Therefore, calling allocate before mpas_pool_create_pool actually causes memory leaks.

Consider this minimal reproducible example (MRE):

! $ cat mem_leak_demo.f90
program mem_leak_demo
    use mpas_derived_types, only: mpas_pool_type
    use mpas_pool_routines, only: mpas_pool_create_pool, mpas_pool_destroy_pool

    implicit none

    type(mpas_pool_type), pointer :: mpas_pool

    allocate(mpas_pool)
    call mpas_pool_create_pool(mpas_pool)
    call mpas_pool_destroy_pool(mpas_pool)
end program mem_leak_demo
! $ ifort -o mem_leak_demo mem_leak_demo.f90 -Isrc/external/esmf_time_f90 -Isrc/framework -Lsrc -lframework

Running the MRE with valgrind --leak-check=full ./mem_leak_demo produces the following report:

==32228== Memcheck, a memory error detector
==32228== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==32228== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==32228== Command: ./mem_leak_demo
==32228==
==32228==
==32228== HEAP SUMMARY:
==32228==     in use at exit: 144 bytes in 1 blocks
==32228==   total heap usage: 4 allocs, 3 frees, 1,432 bytes allocated
==32228==
==32228== 144 bytes in 1 blocks are definitely lost in loss record 1 of 1
==32228==    at 0x40366EB: malloc (vg_replace_malloc.c:393)
==32228==    by 0x4C7164: _mm_malloc (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==    by 0x477B0C: for_allocate_handle (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==    by 0x40A899: MAIN__ (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==    by 0x40A83C: main (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==
==32228== LEAK SUMMARY:
==32228==    definitely lost: 144 bytes in 1 blocks
==32228==    indirectly lost: 0 bytes in 0 blocks
==32228==      possibly lost: 0 bytes in 0 blocks
==32228==    still reachable: 0 bytes in 0 blocks
==32228==         suppressed: 0 bytes in 0 blocks
==32228==
==32228== For lists of detected and suppressed errors, rerun with: -s
==32228== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Deleting the line with allocate(mpas_pool) in the MRE results in a clean report.

==112916== Memcheck, a memory error detector
==112916== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==112916== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==112916== Command: ./mem_leak_demo
==112916==
==112916==
==112916== HEAP SUMMARY:
==112916==     in use at exit: 0 bytes in 0 blocks
==112916==   total heap usage: 3 allocs, 3 frees, 1,288 bytes allocated
==112916==
==112916== All heap blocks were freed -- no leaks are possible
==112916==
==112916== For lists of detected and suppressed errors, rerun with: -s
==112916== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@mgduda mgduda self-requested a review April 15, 2024 23:00
@mgduda mgduda changed the base branch from develop to release-v8.1.0 April 15, 2024 23:05
@mgduda
Copy link
Contributor

mgduda commented Apr 15, 2024

@kuanchihwang Excellent work in finding this memory leak! We're planning to release v8.1.0 later this week, and since this is a bug fix, I think it makes sense to set the base branch to release-v8.1.0. Would you mind rebasing your PR branch on, say, the v8.0.0 tag? If you'd rather, I can handle the rebase and force-push the branch to your fork; just let me know what you'd prefer.

The `mpas_pool_create_pool` subroutine already allocates the pointer in its arguments.
Therefore, calling `allocate` before `mpas_pool_create_pool` actually causes memory leaks.
@kuanchihwang kuanchihwang force-pushed the develop-fix-memory-leaks branch from a21f318 to 279df94 Compare April 16, 2024 03:21
@kuanchihwang
Copy link
Contributor Author

Yes, I have rebased this PR onto the release-v8.1.0 branch. The history is now linear again.

@mgduda mgduda merged commit f9cfd78 into MPAS-Dev:release-v8.1.0 Apr 16, 2024
@kuanchihwang kuanchihwang deleted the develop-fix-memory-leaks branch April 17, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants