search

compTAG / r-tda

Other
4 stars 6 forks source link

Memory error on CRAN with test of memory access errors using AddressSanitizer #17

Closed dlm closed 5 years ago

dlm commented 5 years ago

Memory test is failing on CRAN when performing some memory checks. See https://www.stats.ox.ac.uk/pub/bdr/memtests/gcc-ASAN/TDA/00check.log for error log.

dlm commented 5 years ago

Looks like you fixed the memory error. Did you push a new release to CRAN? If so, we should tag the commit as a release.

jkim82133 commented 5 years ago

It was not the memory error from https://www.stats.ox.ac.uk/pub/bdr/memtests/gcc-ASAN/TDA/00check.log but for the latest version of TDA to 1.6.2 (which was also related to another memory error).

But I think I roughly figured out the issue. Seems like upcasting from Gudhi::Alpha_complex to Gudhi::Simplex_tree when at the function return from AlphaComplexFiltrationGudhi() function in tdautils/gudhiUtils.h to alphaComplexDiag() function in alphaComplex.h. Then stack-use-after-scope error happens when num_simplices() is called from the upcasted returned object.

I didn't investigate in full detail, but seems like upcasting inside the function AlphaComplexFiltrationGudhi() resolves the stack-use-after-scope error. So after checking few more things, I am planning to push this to CRAN.

jkim82133 commented 5 years ago

So I think I figured it out. https://github.com/compTAG/r-tda/commit/f15baef774622dd47e2f2c1549da673b0fdf923e The memory issue is due to something like a shallow move constructor of 'Simplex_tree' class in Simplex_tree.h. In 'Simplextree' class, 'root' is of type 'Simplex_tree::Siblings = Simplex_tree_siblings' and 'filtrationvect' is of type 'std::vector'. Both type contains 'Simplex_tree_siblings *', and a 'Simplextree' class variable contains the pointer to its 'root'. Hence, when the move constructor is called, then the address of 'old.root_' is still preserved in new 'Simplex_tree', although 'old' is deleted. Temporarily disable move constructor so that copy constructor is used instead.

dlm commented 5 years ago

Great! I was looking through the commit history. I see that you made a branch fix-memory. Let me know if you want any help merging it into master or tagging the version that you released.

jkim82133 commented 5 years ago

Yes, I merged it into master and deleted fix-memory branch.