Add Valgrind CI action#2921
Merged
amai2012 merged 34 commits intodanmar:mainfrom Dec 7, 2020
amai2012:valgrind
Merged
Conversation
Install valgrind
Rename valgrind action
firewave
reviewed
Nov 25, 2020
.github/workflows/valgrind.yml
Outdated
|
|
||
| - name: Run valgrind | ||
| run: | | ||
| valgrind ./testrunner |
Collaborator
There was a problem hiding this comment.
You need to pass --error-exitcode=1 so it will actually fail on any finding.
I also suggest adding the following options for better and more complete reporting --error-limit=no --leak-check=full --num-callers=50 --show-reachable=yes --track-fds=yes --track-origins=yes.
Collaborator
Author
There was a problem hiding this comment.
@firewave Yes, but then all CI build would suddenly break which is not desired IMHO.
We should fix those issues (outside this PR) and afterwards enable the check.
Thanks for the suggested command line flags!
Collaborator
There was a problem hiding this comment.
Fine with me. We should hurry though so we don't introduce even more while we are waiting :D
Collaborator
There was a problem hiding this comment.
We could also reduce the runtime by using -O1. That's what usually is suggested and won't mess too much with the stack traces in case of an error.
Collaborator
There was a problem hiding this comment.
Unfortunately that produces code which valgrind cannot handle
vex amd64->IR: unhandled instruction bytes: 0x62 0xF1 0x5D 0x48 0xEF 0xE4 0xC5 0xFB 0x11 0xA5
vex amd64->IR: REX=0 REX.W=0 REX.R=0 REX.X=0 REX.B=0
vex amd64->IR: VEX=0 VEX.L=0 VEX.nVVVV=0x0 ESC=NONE
vex amd64->IR: PFX.66=0 PFX.F2=0 PFX.F3=0
==7492== valgrind: Unrecognised instruction at address 0x7995ee.
The version is pretty old though so this might be expected. Will check with the latest version and open a ticket upstream if this persists.
So back to -Og...
Collaborator
|
As suggested in #2921 (comment)
firewave
reviewed
Nov 25, 2020
.github/workflows/valgrind.yml
Outdated
| - name: Run valgrind | ||
| run: | | ||
| valgrind ./testrunner | ||
| valgrind --error-limit=no --leak-check=full --num-callers=50 --show-reachable=yes --track-fds=yes --track-origins=yes ./testrunner |
Collaborator
There was a problem hiding this comment.
you forgot --error-exitcode=1
Aim: reduce the build duration
Collaborator
|
We should probably disable the glibcxx debug containers as well by passing |
Collaborator
|
It seems the memory leaks are caused by threads that are spawned and not properly cleaned up. I saw something similar while running ASAN which leads to the system being swamped with threads and then it runs out of memory because of the sanitizer overhead. Will have a look at that. |
and also limit valgrind output
Remove --track-fds=yes Add --track-origins=yes Add TestExprEngine inst
Collaborator
|
Try |
Collaborator
Author
Thanks, I'll try |
Collaborator
Author
Debug symbols are now available.
|
Collaborator
|
|
Collaborator
|
I found some issues with the forked processes which leads to zombies and too many processes being spawned. Will prepare a PR. |
Collaborator
|
You could also use |
Collaborator
Author
|
A run with all tests seems to generate too much output for now |
Collaborator
|
Will take a look tomorrow. |
Collaborator
|
Are there any improvements left? Otherwise i suggest to merge it. |
and one more test
Collaborator
Author
|
Currently only a small subset of testrunner testsuites are being run.
I've enabled the build breaker for now and will merge now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add valgrind CI action.
Inspired by #2920