search

TheochemUI / eOn

eOn v3 and beyond
https://theochemui.github.io/eOn/
BSD 3-Clause "New" or "Revised" License
2 stars 2 forks source link

Memory leaks (`Potential` redesign) #45

Open HaoZeke opened 1 year ago

HaoZeke commented 1 year ago

In what should not be surprising, the current implementation of Potential leaks, due to unrestricted usage of new.

==38903== 
==38903== HEAP SUMMARY:
==38903==     in use at exit: 3,144 bytes in 5 blocks
==38903==   total heap usage: 261 allocs, 256 frees, 159,618 bytes allocated
==38903== 
==38903== 48 bytes in 1 blocks are definitely lost in loss record 1 of 5
==38903==    at 0x4842003: operator new(unsigned long) (vg_replace_malloc.c:422)
==38903==    by 0x491BD6C: Potential::getPotential(Parameters*) (Potential.cpp:146)
==38903==    by 0x111DAC: tests::PotTest_callForce_Test::TestBody() (PotTest.cpp:85)
==38903==    by 0x49D61ED: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D64C0: testing::Test::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D6886: testing::TestInfo::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D70A3: testing::TestSuite::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49DF18D: testing::internal::UnitTestImpl::RunAllTests() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D6B88: testing::UnitTest::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x494606E: main (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest_main.so.1.12.1)
==38903== 
==38903== 56 bytes in 1 blocks are definitely lost in loss record 3 of 5
==38903==    at 0x4842003: operator new(unsigned long) (vg_replace_malloc.c:422)
==38903==    by 0x491BE02: Potential::getPotential(Parameters*) (Potential.cpp:152)
==38903==    by 0x10F28B: tests::PotTest_getName_Test::TestBody() (PotTest.cpp:39)
==38903==    by 0x49D61ED: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D64C0: testing::Test::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D6886: testing::TestInfo::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D70A3: testing::TestSuite::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49DF18D: testing::internal::UnitTestImpl::RunAllTests() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D6B88: testing::UnitTest::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x494606E: main (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest_main.so.1.12.1)
==38903== 
==38903== 2,984 (48 direct, 2,936 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 5
==38903==    at 0x4842003: operator new(unsigned long) (vg_replace_malloc.c:422)
==38903==    by 0x491BD6C: Potential::getPotential(Parameters*) (Potential.cpp:146)
==38903==    by 0x10F1CE: tests::PotTest_getName_Test::TestBody() (PotTest.cpp:36)
==38903==    by 0x49D61ED: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D64C0: testing::Test::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D6886: testing::TestInfo::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D70A3: testing::TestSuite::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49DF18D: testing::internal::UnitTestImpl::RunAllTests() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x49D6B88: testing::UnitTest::Run() (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest.so.1.12.1)
==38903==    by 0x494606E: main (in /home/rgoswami/micromamba/envs/eongit/lib/libgtest_main.so.1.12.1)
==38903== 
==38903== LEAK SUMMARY:
==38903==    definitely lost: 152 bytes in 3 blocks
==38903==    indirectly lost: 2,936 bytes in 1 blocks
==38903==      possibly lost: 0 bytes in 0 blocks
==38903==    still reachable: 56 bytes in 1 blocks
==38903==         suppressed: 0 bytes in 0 blocks
==38903== Reachable blocks (those to which a pointer was found) are not shown.
==38903== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==38903== 
==38903== For lists of detected and suppressed errors, rerun with: -s
==38903== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
HaoZeke commented 1 year ago

Actually this is because of unpaired new calls. They need to either be unique_ptr objects or have corresponding delete calls. The makePotential is suspect for returnig raw pointers via new without callers managing delete.

HaoZeke commented 2 months ago

Consider the simplest call: valgrind --log-file="valgrind_lj_sp" --leak-check=full --show-leak-kinds=all --track-origins=yes ./eonclient -s example/pos.con -p lj

For eonSVN at https://github.com/TheochemUI/eOn/commit/48372a16a1f9e8ba8b413a94e408a2a9c8700772

==418106== Memcheck, a memory error detector
==418106== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==418106== Using Valgrind-3.23.0 and LibVEX; rerun with -h for copyright info
==418106== Command: ./eonclient -s example/pos.con -p lj
==418106== Parent PID: 413942
==418106== 
==418106== 
==418106== HEAP SUMMARY:
==418106==     in use at exit: 520 bytes in 2 blocks
==418106==   total heap usage: 38 allocs, 36 frees, 165,768 bytes allocated
==418106== 
==418106== 48 bytes in 1 blocks are still reachable in loss record 1 of 2
==418106==    at 0x4842F73: operator new(unsigned long) (vg_replace_malloc.c:487)
==418106==    by 0x1A9368: Potential::getPotential(Parameters*) (Potential.cpp:80)
==418106==    by 0x18946C: Matter::computePotential() (Matter.cpp:933)
==418106==    by 0x187800: Matter::getPotentialEnergy() (Matter.cpp:586)
==418106==    by 0x1E4638: singlePoint(Parameters*, Matter*) (CommandLine.cpp:16)
==418106==    by 0x1E500E: commandLine(int, char**) (CommandLine.cpp:147)
==418106==    by 0x113981: main (ClientEON.cpp:232)
==418106== 
==418106== 472 bytes in 1 blocks are still reachable in loss record 2 of 2
==418106==    at 0x4842788: malloc (vg_replace_malloc.c:446)
==418106==    by 0x5074A68: __fopen_internal (iofopen.c:65)
==418106==    by 0x1E5750: log_init(Parameters*, char*) (Log.cpp:14)
==418106==    by 0x1E4ED9: commandLine(int, char**) (CommandLine.cpp:134)
==418106==    by 0x113981: main (ClientEON.cpp:232)
==418106== 
==418106== LEAK SUMMARY:
==418106==    definitely lost: 0 bytes in 0 blocks
==418106==    indirectly lost: 0 bytes in 0 blocks
==418106==      possibly lost: 0 bytes in 0 blocks
==418106==    still reachable: 520 bytes in 2 blocks
==418106==         suppressed: 0 bytes in 0 blocks
==418106== 
==418106== For lists of detected and suppressed errors, rerun with: -s
==418106== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Which makes sense, given the logging mechanism. Unfortunately it is pretty bad for v3_client at https://github.com/TheochemUI/eOn/commit/e9395cb5b1b757ef82d8147aa8fdf93ee898ccc6 too.

➜ tail -n 50 val_lj_sp
==547111==    by 0x489665F: log<double> (logger.h:80)
==547111==    by 0x489665F: log<double> (logger.h:85)
==547111==    by 0x489665F: info<double> (logger.h:140)
==547111==    by 0x489665F: eonc::PointJob::run[abi:cxx11]() (PointJob.cpp:30)
==547111==    by 0x143931: eonc::commandLine(int, char**) (CommandLine.cpp:158)
==547111==    by 0x116921: main (ClientEON.cpp:309)
==547111==  Uninitialised value was created by a stack allocation
==547111==    at 0x4895C5A: void spdlog::logger::log_<double>(spdlog::source_loc, spdlog::level::level_enum, fmt::v10::basic_string_view<char>, double&&) (logger.h:317)
==547111== 
==547111== Conditional jump or move depends on uninitialised value(s)
==547111==    at 0x4895DA7: copy_str<char, char*, fmt::v10::appender> (core.h:796)
==547111==    by 0x4895DA7: flush (core.h:938)
==547111==    by 0x4895DA7: out (core.h:949)
==547111==    by 0x4895DA7: get_iterator<fmt::v10::detail::iterator_buffer<fmt::v10::appender, char, fmt::v10::detail::buffer_traits>, fmt::v10::appender> (core.h:1147)
==547111==    by 0x4895DA7: vformat_to<fmt::v10::appender> (core.h:2844)
==547111==    by 0x4895DA7: void spdlog::logger::log_<double>(spdlog::source_loc, spdlog::level::level_enum, fmt::v10::basic_string_view<char>, double&&) (logger.h:328)
==547111==    by 0x489665F: log<double> (logger.h:80)
==547111==    by 0x489665F: log<double> (logger.h:85)
==547111==    by 0x489665F: info<double> (logger.h:140)
==547111==    by 0x489665F: eonc::PointJob::run[abi:cxx11]() (PointJob.cpp:30)
==547111==    by 0x143931: eonc::commandLine(int, char**) (CommandLine.cpp:158)
==547111==    by 0x116921: main (ClientEON.cpp:309)
==547111==  Uninitialised value was created by a stack allocation
==547111==    at 0x4895C5A: void spdlog::logger::log_<double>(spdlog::source_loc, spdlog::level::level_enum, fmt::v10::basic_string_view<char>, double&&) (logger.h:317)
==547111== 
==547111== Conditional jump or move depends on uninitialised value(s)
==547111==    at 0x4895DFF: copy_str<char, char*, fmt::v10::appender> (core.h:796)
==547111==    by 0x4895DFF: flush (core.h:938)
==547111==    by 0x4895DFF: out (core.h:949)
==547111==    by 0x4895DFF: get_iterator<fmt::v10::detail::iterator_buffer<fmt::v10::appender, char, fmt::v10::detail::buffer_traits>, fmt::v10::appender> (core.h:1147)
==547111==    by 0x4895DFF: vformat_to<fmt::v10::appender> (core.h:2844)
==547111==    by 0x4895DFF: void spdlog::logger::log_<double>(spdlog::source_loc, spdlog::level::level_enum, fmt::v10::basic_string_view<char>, double&&) (logger.h:328)
==547111==    by 0x489665F: log<double> (logger.h:80)
==547111==    by 0x489665F: log<double> (logger.h:85)
==547111==    by 0x489665F: info<double> (logger.h:140)
==547111==    by 0x489665F: eonc::PointJob::run[abi:cxx11]() (PointJob.cpp:30)
==547111==    by 0x143931: eonc::commandLine(int, char**) (CommandLine.cpp:158)
==547111==    by 0x116921: main (ClientEON.cpp:309)
==547111==  Uninitialised value was created by a stack allocation
==547111==    at 0x4895C5A: void spdlog::logger::log_<double>(spdlog::source_loc, spdlog::level::level_enum, fmt::v10::basic_string_view<char>, double&&) (logger.h:317)
==547111== 
==547111== 
==547111== HEAP SUMMARY:
==547111==     in use at exit: 0 bytes in 0 blocks
==547111==   total heap usage: 4,144 allocs, 4,144 frees, 298,076 bytes allocated
==547111== 
==547111== All heap blocks were freed -- no leaks are possible
==547111== 
==547111== For lists of detected and suppressed errors, rerun with: -s
==547111== ERROR SUMMARY: 224 errors from 84 contexts (suppressed: 0 from 0)

There aren't any real leaks in either, but it is rather disappointing.