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.

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

Testament does not check memory leaks with Valgrind #15631

Closed
n5m opened this issue Oct 19, 2020 · 1 comment · Fixed by #15632
Closed

Testament does not check memory leaks with Valgrind #15631

n5m opened this issue Oct 19, 2020 · 1 comment · Fixed by #15632

Comments

@n5m
Copy link
Contributor

n5m commented Oct 19, 2020

Testament docs claim the following:

valgrind: false   # Can use Valgrind to check for memory leaks, or not (Linux 64Bit only).

This is incorrect. Testament tests do not check for memory leaks even when valgrind: true is set.

Example

# tests/leak.nim
discard """
  valgrind: true
  cmd: "nim $target --gc:arc -d:useMalloc $options $file"
"""
var mem: pointer = cast[pointer](alloc(sizeof(int)))
mem = nil

Current Output

leak.nim definitely leaks memory, but testament does not catch it.

user@user:~$ testament run leak.nim
PASS: tests/leak.nim C                                             ( 0.61 sec)

Possible Solution

The reason is that valgrind returns an exit code of 0 even if memory leaks are detected unless --leak-check=yes|full is set. Evidence:

user@user:~$ nim compile --gc:arc -d:useMalloc leak.nim 
Hint: used config file '/home/user/nim-1.4.0/config/nim.cfg' [Conf]
Hint: used config file '/home/user/nim-1.4.0/config/config.nims' [Conf]
....
Hint:  [Link]
Hint: 19076 lines; 0.165s; 16.574MiB peakmem; Debug build; proj: /home/user/leak.nim; out: /home/user/leak [SuccessX]
user@user:~$ valgrind --error-exitcode=1 ./leak
==3525== Memcheck, a memory error detector
==3525== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3525== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==3525== Command: ./leak
==3525== 
==3525== 
==3525== HEAP SUMMARY:
==3525==     in use at exit: 8 bytes in 1 blocks
==3525==   total heap usage: 1 allocs, 0 frees, 8 bytes allocated
==3525== 
==3525== LEAK SUMMARY:
==3525==    definitely lost: 8 bytes in 1 blocks
==3525==    indirectly lost: 0 bytes in 0 blocks
==3525==      possibly lost: 0 bytes in 0 blocks
==3525==    still reachable: 0 bytes in 0 blocks
==3525==         suppressed: 0 bytes in 0 blocks
==3525== Rerun with --leak-check=full to see details of leaked memory
==3525== 
==3525== For counts of detected and suppressed errors, rerun with: -v
==3525== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
user@user:~$ echo $?
0

With --leak-check=yes|full set, Valgrind returns a non-zero exit code.

user@user:~$ valgrind --error-exitcode=1 --leak-check=yes ./leak
==5155== Memcheck, a memory error detector
==5155== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==5155== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==5155== Command: ./leak
==5155== 
==5155== 
==5155== HEAP SUMMARY:
==5155==     in use at exit: 8 bytes in 1 blocks
==5155==   total heap usage: 1 allocs, 0 frees, 8 bytes allocated
==5155== 
==5155== 8 bytes in 1 blocks are definitely lost in loss record 1 of 1
==5155==    at 0x483577F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==5155==    by 0x10D04B: allocImpl__KzdpcuLT9aef9bsiSHlIu9aFg_3 (in /home/user/leak)
==5155==    by 0x10D8BD: NimMainModule (in /home/user/leak)
==5155==    by 0x10D7F0: NimMainInner (in /home/user/leak)
==5155==    by 0x10D81D: NimMain (in /home/user/leak)
==5155==    by 0x10D857: main (in /home/user/leak)
==5155== 
==5155== LEAK SUMMARY:
==5155==    definitely lost: 8 bytes in 1 blocks
==5155==    indirectly lost: 0 bytes in 0 blocks
==5155==      possibly lost: 0 bytes in 0 blocks
==5155==    still reachable: 0 bytes in 0 blocks
==5155==         suppressed: 0 bytes in 0 blocks
==5155== 
==5155== For counts of detected and suppressed errors, rerun with: -v
==5155== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
user@user:~$ echo $?
1

Currently, Testament only sets --error-exitcode=1.

Additional Information

git hash: bdcd87a

user@user:~$ nim -v
Nim Compiler Version 1.4.0 [Linux: amd64]
Compiled at 2020-10-16
Copyright (c) 2006-2020 by Andreas Rumpf

git hash: bdcd87afca238a0a7b2c70971827cf9172817b12
active boot switches: -d:release
@n5m n5m mentioned this issue Oct 19, 2020
@Araq
Copy link
Member

Araq commented Oct 19, 2020

Yeah, I'm aware, IMO the documentation is wrong but of course we can also fix testament instead.

@ghost ghost added the Test Suite label Oct 19, 2020
Clyybber added a commit that referenced this issue Oct 19, 2020
* trigger valgrind failure on memory leak

* remove non-malloc tests

* remove ORC test

is redundant because we already have an ARC test

* only run valgrind tests on 64-bit Linux

* disable freebsd and openbsd

* Remove tleak_refc

As to not test implementation details (or bug)

* Fix test failures by removing redundant test

Since this tests/shoulfail/tvalgrind.nim was specified here to fail this test
itself fails since it will be skipped on non-linux CI

* Remove test, reason detailed in the previous commit

* Remove redundant disables

* Revert removing disables

* Add and use valgrind: leaks

* Fix

Co-authored-by: Clyybber <darkmine956@gmail.com>
Co-authored-by: n5m
narimiran pushed a commit that referenced this issue Oct 21, 2020
* trigger valgrind failure on memory leak

* remove non-malloc tests

* remove ORC test

is redundant because we already have an ARC test

* only run valgrind tests on 64-bit Linux

* disable freebsd and openbsd

* Remove tleak_refc

As to not test implementation details (or bug)

* Fix test failures by removing redundant test

Since this tests/shoulfail/tvalgrind.nim was specified here to fail this test
itself fails since it will be skipped on non-linux CI

* Remove test, reason detailed in the previous commit

* Remove redundant disables

* Revert removing disables

* Add and use valgrind: leaks

* Fix

Co-authored-by: Clyybber <darkmine956@gmail.com>
Co-authored-by: n5m
(cherry picked from commit 436e1fa)
mildred pushed a commit to mildred/Nim that referenced this issue Jan 11, 2021
* trigger valgrind failure on memory leak

* remove non-malloc tests

* remove ORC test

is redundant because we already have an ARC test

* only run valgrind tests on 64-bit Linux

* disable freebsd and openbsd

* Remove tleak_refc

As to not test implementation details (or bug)

* Fix test failures by removing redundant test

Since this tests/shoulfail/tvalgrind.nim was specified here to fail this test
itself fails since it will be skipped on non-linux CI

* Remove test, reason detailed in the previous commit

* Remove redundant disables

* Revert removing disables

* Add and use valgrind: leaks

* Fix

Co-authored-by: Clyybber <darkmine956@gmail.com>
Co-authored-by: n5m
irdassis pushed a commit to irdassis/Nim that referenced this issue Mar 16, 2021
* trigger valgrind failure on memory leak

* remove non-malloc tests

* remove ORC test

is redundant because we already have an ARC test

* only run valgrind tests on 64-bit Linux

* disable freebsd and openbsd

* Remove tleak_refc

As to not test implementation details (or bug)

* Fix test failures by removing redundant test

Since this tests/shoulfail/tvalgrind.nim was specified here to fail this test
itself fails since it will be skipped on non-linux CI

* Remove test, reason detailed in the previous commit

* Remove redundant disables

* Revert removing disables

* Add and use valgrind: leaks

* Fix

Co-authored-by: Clyybber <darkmine956@gmail.com>
Co-authored-by: n5m
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants