Memory Leaks

[jkyang92]

trafficstars

This is work in progress to make it so that all tests run without reported memory leaks from valgrind. So far the pull request contains the cases I understand how to fix. There are four main changes in this pull request right now

• Use valgrind client requests and function replacement to silence undefined value errors from valgrind related to how gc works. The vast majority of the lines added are in the memcheck.h and valgrind.h files copied from valgrind so that we don't depend on valgrind.
• Make ConcreteRing explicitly own the RingType object using a unique_ptr. Previously, no one was calling the destructor for the corresponding RingType.
• Make FreeAlgebra use a shared_ptr for the FreeMonoid. I'd like to use a unique_ptr, but when I tried, it seems we're using the copy constructor for FreeAlgebra somewhere.
• A mathicgb change see https://github.com/Macaulay2/mathicgb/pull/40

Here's an approximate list of what remains:

• [x] flint leaks
• [ ] factory leaks
• [x] ntl/LLL leaks
• [x] MutableMatrix + ARingQQGMP leaks
• [x] createF4Res leaks
• [x] PolyRing::power leaks (this leak makes no sense to me, it seems impossible)
• [ ] mathigcb-unit-test IO related leaks (these are issues with the test themselves I believe)
  • unit-tests:IO.ideal
  • unit-tests:Ideal.readwrite

My guess is that the flint and ntl/LLL leaks just need calls to mpz_realloc_limbs or similar, but I don't yet understand the code involved. The factory leaks I can't figure out who's fault they are, since I have no idea how factory's memory management works yet. The MutableMatrix and the createF4Res leaks are likely eventually simple fixes, but right now the challenge is figuring out which bit of code should actually be responsible for deallocation.

There are one or two issues that are not a real leaks

• unit-tests:Arena.BigAndOverflow expects and needs new to throw an exception, which valgrind can't replicate
• rawRandomInitialize and in particular gmp_randinit_mt seems to leak, but this is a one time allocation so is safe regardless.

[moorewf]

I'll see if I can track down why unique_ptr is not working for FreeMonoid.

[jkyang92]

So I learned that make_shared is not safe for use with libgc. I believe this is what's causing the test failures. (The build failures on clang are a small issue). In particular make_shared is allowed to allocate the required memory itself and call placement new instead of the usual new and there is no way to override this behavior and no way to specify the allocator to use for the memory. Note shared_ptr itself is safe to use.

[jkyang92]

I've marked this pull request as ready, but I have a few remarks on what remains and the current changes.

1. The factory leak is a bug in factory. At some point, I'll submit a bug report to singular about it.
2. Some of the code in aring-gf-flint-big.h is misusing ring_elem. In particular, the ring_elem union probably needs more entries added to it for things like this.
3. I've decided not to do anything about the remaining mathicgb-unit-test leaks. They are mostly test specific, and not really representative of realistic use cases.
4. The recent change adding exceptions to some parts of the engine has added a new set of memory leaks, but I'd like to address that separately. In particular the problem is much of the code is simply not exception safe, and would need a significant rewrite to be made so. As such I'm going to start working on those changes, but they should probably be reviewed separately since there's a lot involved.

[mikestillman]

@jkyang92 Is this pull request ready to go? Could you first merge in development branch into your branch?

[jkyang92]

@mikestillman In theory it's ready. But now I have these weird openmp build failures on mac that I can't test.

[DanGrayson]

That same openmp problem is afflicting all of our pull requests. Maybe something changed in the OS on the build machine. Mike, any ideas?

[DanGrayson]

I'm trying something with https://github.com/Macaulay2/M2/pull/2649 -- upgrading the OSes.

[DanGrayson]

Well, that didn't work.