ami-iit / bipedal-locomotion-framework

Suite of libraries for achieving bipedal locomotion on humanoid robots
https://ami-iit.github.io/bipedal-locomotion-framework/
BSD 3-Clause "New" or "Revised" License
136 stars 36 forks source link

Add support for testing if a portion of code allocates memory via MemoryAllocationMonitor #768

Closed traversaro closed 3 months ago

traversaro commented 8 months ago

This PR adds a MemoryAllocationMonitor class that part of a non-installed TestUtils library.

The MemoryAllocationMonitor class can be used to test if a portion of code is allocating (or de-allocating) memory dynamically, with the following structure:

// Code that can allocate memory
SomeSystem sys = SomeSystem();

// Check that code that is meant to be run
for (int i=0; i < 50; i++)
{
    MemoryAllocationMonitor::startMonitor();
    sys.advance();
    REQUIRE(MemoryAllocationMonitor::endMonitorAndCheckNoMemoryAllocationInLastMonitor());
}

The MemoryAllocationMonitor monitors memory allocation by overloading malloc and other memory allocation functions in the src/TestUtils/MemoryAllocationMonitorRedefinedMemoryFunctions.c. This function are contained in a small shared-only library called MemoryAllocationMonitorPreload, that is then injected in the tests via LD_PRELOAD (see the logic in cmake/AddBipedalLocomotionUnitTest.cmake).

These redefined functions are just incrementing a counter for each call, and then call the regular non-redefined functions. The counter are reset by the MemoryAllocationMonitor::startMonitor() function. MemoryAllocationMonitor::endMonitorAndCheckNoMemoryAllocationInLastMonitor() returns true if all counter are still zero, and false otherwise.

As demonstrated in the src/TestUtils/tests/MemoryAllocationMonitorTest.cpp test, this strategy is able to detect reliably allocation done via C's malloc, C++'s new, std's containers or Eigen vectors.

Redefining malloc and injecting it via LD_PRELOAD only works on Linux, so all the machinery is only present if the CMake option FRAMEWORK_RUN_MemoryAllocationMonitor_tests is enabled (that is enabled by default only on Linux). If FRAMEWORK_RUN_MemoryAllocationMonitor_tests is OFF, a dummy version of MemoryAllocationMonitor is compiled where MemoryAllocationMonitor::endMonitorAndCheckNoMemoryAllocationInLastMonitor() always return true, so that we can avoid to modify the tests code.

To add an example of usage of this new class, I added tests of non-allocation of memory in the advance portion of ContinuousDynamicalSystem tests. Apparently the Integrator class allocates some memory in the first run of the advance method, so I had to account for that and only do the checks

Eventually I may include this functionality in self-contained easy to re-use library, but I wanted first to start with a more simple example.

traversaro commented 8 months ago

The MemoryAllocationMonitor is inspired by the code added in YARP in https://github.com/robotology/yarp/pull/365, the main difference is that that code only intercepted memory allocation done via C++'s new, while by overloading malloc we can detect also code that is not allocating memory via new.

traversaro commented 8 months ago

cc @giotherobot @rozzong if you need to automatically monitor in a test if a piece of code perform dynamic memory allocation, you can refer to the code in this PR.

traversaro commented 8 months ago

The vcpkg/CI job failure is not related to the PR, see https://github.com/ami-iit/bipedal-locomotion-framework/issues/769 .

traversaro commented 8 months ago

The vcpkg/CI job failure is not related to the PR, see #769 .

Instead the apt failure is related, for some reason what is working with conda compilers is failing on apt on Ubuntu 20.04 .

traversaro commented 8 months ago

The vcpkg/CI job failure is not related to the PR, see #769 .

Instead the apt failure is related, for some reason what is working with conda compilers is failing on apt on Ubuntu 20.04 .

I think the issue was in glibc, not in the compilers. Anyhow, it seems that everything works fine if we link the library to preload with -ldl (that make sense as we actually use dlsym). Probably just with the glibc of Ubuntu 22.04 calling dlsym resulted in a dynamic loader looking for the symbol, resulting in a malloc call that itself created an infinite recursive call to malloc.

traversaro commented 8 months ago

The vcpkg/CI job failure is not related to the PR, see #769 .

Instead the apt failure is related, for some reason what is working with conda compilers is failing on apt on Ubuntu 20.04 .

I think the issue was in glibc, not in the compilers. Anyhow, it seems that everything works fine if we link the library to preload with -ldl (that make sense as we actually use dlsym). Probably just with the glibc of Ubuntu 22.04 calling dlsym resulted in a dynamic loader looking for the symbol, resulting in a malloc call that itself created an infinite recursive call to malloc.

Actually, this was just luck, other tests still fail with a infinite recursive calls. Probably we need some kind of dummy allocator as done in heaptrack: https://github.com/KDE/heaptrack/blob/0f5285ce7ba7dfe45a937398cbd88ffc29dfe828/src/track/heaptrack_preload.cpp#L158 .

traversaro commented 8 months ago

The vcpkg/CI job failure is not related to the PR, see #769 .

Instead the apt failure is related, for some reason what is working with conda compilers is failing on apt on Ubuntu 20.04 .

I think the issue was in glibc, not in the compilers. Anyhow, it seems that everything works fine if we link the library to preload with -ldl (that make sense as we actually use dlsym). Probably just with the glibc of Ubuntu 22.04 calling dlsym resulted in a dynamic loader looking for the symbol, resulting in a malloc call that itself created an infinite recursive call to malloc.

Actually, this was just luck, other tests still fail with a infinite recursive calls. Probably we need some kind of dummy allocator as done in heaptrack: https://github.com/KDE/heaptrack/blob/0f5285ce7ba7dfe45a937398cbd88ffc29dfe828/src/track/heaptrack_preload.cpp#L158 .

I tried to fix this, but it is not trivial. I guess an easier option is just to run the tests on system with glibc >= 2.35, I did this in https://github.com/ami-iit/bipedal-locomotion-framework/pull/768/commits/8a10a1888c42422165b59138afda5e13e7ee5063 and I think this should fix the CI.

traversaro commented 8 months ago

After limiting the check on glibc >= 2.35 systems, the only remaining failing jobs are due to https://github.com/ami-iit/bipedal-locomotion-framework/issues/769 .

GiulioRomualdi commented 3 months ago

Hi @traversaro, I don't remember the status of this PR. Do you think it is mergeable?

traversaro commented 3 months ago

Hi @traversaro, I don't remember the status of this PR. Do you think it is mergeable?

I just solved the conflicts, if the CI is happy it is good to go.

GiulioRomualdi commented 3 months ago

Let me rebase the PR on master.