Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Valgrind tests seem to work #22

Closed
rrthomas opened this issue Jan 12, 2018 · 27 comments
Closed

Valgrind tests seem to work #22

rrthomas opened this issue Jan 12, 2018 · 27 comments

Comments

@rrthomas
Copy link

If I comment out the plan skip_all line, and instead uncomment the plan 1 line, I get:

/home/rrt/.local/var/repo/fortune-mod/fortune-mod/tests/t/valgrind.t ................ ok

So are the tests passing, or am I somehow failing to run them?
@rrthomas
Copy link
Author

I see that the Ubuntu version which I am using has patches for at least one memory leak…

@rrthomas
Copy link
Author

I searched for and attempted to contact François Pinard: his email address bounces and I can't find any other contact for him.

I suggest therefore that I make a friendly fork of recode, which incorporates at least the Debian patches, and perhaps any others I can easily find in other major distros.

I'm having a go at getting the current Recode 3.7beta repo code up and running (if that doesn't work, I can revert to 3.6+patches); it seems fairly straightforward.

@shlomif
Copy link
Owner

shlomif commented Jan 12, 2018 via email

@shlomif
Copy link
Owner

shlomif commented Jan 12, 2018 via email

@rrthomas
Copy link
Author

rrthomas commented Jan 12, 2018 •
edited

Yes, I did make install.

$ ./fortune
No fortunes found
$ ~/.local/games/fortune
No fortunes found

I can see all the fortunes files in ~/.local/share/games/fortunes.

I tried make clean and remaking.

I ran cmake as:

cmake .. -DCMAKE_INSTALL_PREFIX=$HOME/.local

@shlomif
Copy link
Owner

shlomif commented Jan 13, 2018 via email

@rrthomas
Copy link
Author

Great, that works! Now I get two test failures (3 subtest failures):

/home/rrt/.local/var/repo/fortune-mod/fortune-mod/tests/t/trailing-space-and-CRs.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/3 subtests 
/home/rrt/.local/var/repo/fortune-mod/fortune-mod/tests/t/valgrind.t ................ 1/1

It's odd, as the Valgrind log file has no errors I can see:

==30682== Memcheck, a memory error detector
==30682== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==30682== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==30682== Command: ./fortune
==30682== Parent PID: 30603
==30682== 
==30682== 
==30682== HEAP SUMMARY:
==30682==     in use at exit: 9,714 bytes in 164 blocks
==30682==   total heap usage: 5,280 allocs, 5,116 frees, 252,644 bytes allocated
==30682== 
==30682== LEAK SUMMARY:
==30682==    definitely lost: 0 bytes in 0 blocks
==30682==    indirectly lost: 0 bytes in 0 blocks
==30682==      possibly lost: 0 bytes in 0 blocks
==30682==    still reachable: 9,714 bytes in 164 blocks
==30682==         suppressed: 0 bytes in 0 blocks
==30682== Reachable blocks (those to which a pointer was found) are not shown.
==30682== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==30682== 
==30682== For counts of detected and suppressed errors, rerun with: -v
==30682== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@shlomif
Copy link
Owner

shlomif commented Jan 13, 2018 via email

@rrthomas
Copy link
Author

OK, I've now got recode resurrected and ready to test with fortune-mod.

What is the problem here? Is it that the memory still in use should have been freed?

@shlomif
Copy link
Owner

shlomif commented Jan 13, 2018 via email

@rrthomas
Copy link
Author

It's OK, you don't need to educate me about basic concepts: I have two degrees in computer science and I have been working as a programmer for nearly 30 years. Thanks, though, for taking the time to add explanation in case I needed it.

The point is that having memory in use when a program exits is usually no problem (that is why I wouldn't usually regard it as an error), because, as you know, memory used by a process is returned to the system when the process exits. Hence, I would only usually worry about unreachable memory.

So I'm not quite sure you understood my question; I'd just like to check: I think in this case you want to check that the recode library has freed all the memory it allocated, and you're saying it has not; am I right?

@shlomif
Copy link
Owner

shlomif commented Jan 13, 2018 via email

@rrthomas
Copy link
Author

Just a quick (non-)update: in fact, I spent the last 24 hours battling with my attempts to fix all warnings in recode. I have completed this now. I have also found that the current code in fact already has all the Debian patches applied (including the space leak fix). I should get to testing with fortune-mod tomorrow.

@shlomif
Copy link
Owner

shlomif commented Jan 15, 2018 via email

@shlomif
Copy link
Owner

shlomif commented Jan 15, 2018

Seems like it is https://github.com/rrthomas/Recode

@rrthomas
Copy link
Author

Yes, that's it. I'm currently setting up Travis and AppVeyor CI.

@rrthomas
Copy link
Author

I ran the test you posted at pinard/Recode#7 and found all memory was freed.

@rrthomas
Copy link
Author

I ran the valgrind tests with --leak-check=full --show-leak-kinds=all, and found that all the remaining memory in use at exit is allocated by getargs, so it seems there's no memory being retained by librecode.

It's not clear to me that this is really a problem, so perhaps you want to relax the requirement, and only look for actual leaks, not memory in use when the program ends?

@shlomif
Copy link
Owner

shlomif commented Jan 16, 2018 via email

shlomif added a commit that referenced this issue Jan 16, 2018
@shlomif
Fix some memory leaks.
0c74581 
"valgrind doesn't lie.". See
#22 (comment)
. Now the valgrind test passes with the new Recode at
https://github.com/rrthomas/Recode .
@shlomif
Copy link
Owner

shlomif commented Jan 16, 2018

Thanks @rrthomas ! I have now fixed these mem leaks which were fortune.c's fault. The valgrind test now passes with your Recode.

@rrthomas
Copy link
Author

Great, thanks! Glad you don't now have to find an alternative to recode. I am still going through the list of Debian bugs and fixing all the easy ones and those with patches. Then I shall do the same for François Pinard's GitHub (unfortunately I found out he died a few years ago), and then I shall make a release.

@shlomif
Copy link
Owner

shlomif commented Jan 16, 2018 via email

@shlomif
Copy link
Owner

shlomif commented Jan 16, 2018

Closing this issue as FIXED. Thanks!

@shlomif shlomif closed this as completed Jan 16, 2018
@shlomif
Copy link
Owner

shlomif commented Jan 18, 2018

@rrthomas : I suggest you enable the Issues tracker on your fork of the recode repo - currently it is disabled. Please let me know when you mint a new release so I can release a new version of fortune-mod.

@rrthomas
Copy link
Author

Done, thanks for the hint.

@rrthomas
Copy link
Author

By the way, in case I forget to get in touch when I make the release, it will be a normal GitHub release, so subscribing to release notifications on my project will be sufficient.

@shlomif
Copy link
Owner

shlomif commented Jan 19, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@shlomif @rrthomas