refresh-bio / KMC

Fast and frugal disk based k-mer counter
256 stars 73 forks source link

check with valgrind #90

Closed notestaff closed 5 years ago

notestaff commented 6 years ago

I see various warnings about possible memory corruption when running KMC under valgrind ( http://valgrind.org/ ). @marekkokot maybe try it? In my experience valgrind warnings often point to real problems.

marekkokot commented 6 years ago

Thanks for the suggestion. I hope I will find the time to check it with Valgrind, but probably it will not be soon (I last used Valgrind a couple years ago and I suspect it will take a significant amount of time to run in and find causes of its warnings).

notestaff commented 6 years ago

@marekkokot it’s actually doesn’t take much time. E.g. it quickly found that in kmer_file.cpp when you fread max_count it should now be sizeof(uint64) not uint32.

notestaff commented 6 years ago

(or if have to read a 32-bit value for compatibility, read into a uint32 then assign that to max_count?)

notestaff commented 6 years ago

btw maybe mention in the docs that kmc databases are not portable across architectures https://stackoverflow.com/questions/2193472/are-files-dumped-with-fwrite-portable-across-different-systems

marekkokot commented 6 years ago

Hi, thanks for pointing that. In fact we was intended to make it portable, and I believe KMC database format itself was portable. We have dedicated functions like: https://github.com/refresh-bio/KMC/blob/master/kmer_counter/kb_completer.cpp#L303 KMC database format was portable, but API was not. Probably when implementing new features I I neglected portability calling fwrite with sizeof. I have created to remember to check this #93

notestaff commented 6 years ago

@marekkokot can also use gcc's built-in runtime error detector: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html

marekkokot commented 5 years ago

KMC was checked with Valgrind (at least partially): #94 #96 so I am going to close this issue. If you will spot any more valgrind errors please create new issue.