Skip to content

Comments

Add valgrind run for cppcheck / self-check#3022

Draft
amai2012 wants to merge 4 commits intodanmar:mainfrom
amai2012:valgrind-selfcheck
Draft

Add valgrind run for cppcheck / self-check#3022
amai2012 wants to merge 4 commits intodanmar:mainfrom
amai2012:valgrind-selfcheck

Conversation

@amai2012
Copy link
Collaborator

@amai2012 amai2012 commented Jan 7, 2021

No description provided.

@amai2012
Copy link
Collaborator Author

amai2012 commented Jan 7, 2021

See #3017

CXXFLAGS="-O1 -g" make -j$(nproc) testrunner USE_Z3=yes HAVE_RULES=yes MATCHCOMPILER=yes

- name: Run valgrind
CXXFLAGS="-O1 -g" make -j$(nproc) USE_Z3=yes HAVE_RULES=yes MATCHCOMPILER=yes CPPFLAGS="-DCHECK_INTERNAL"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regular self-check build also has VERIFY=1.

And remove gui/ directory from scan
@firewave
Copy link
Collaborator

firewave commented Jan 7, 2021

FYI valgrind errors will currently not fail the build because of a bug in the ThreadExecutor. I am preparing a PR. See #3023

run: |
valgrind --error-limit=yes --leak-check=full --num-callers=50 --show-reachable=yes --track-origins=yes --suppressions=valgrind/testrunner.supp --gen-suppressions=all --log-fd=9 --error-exitcode=42 ./testrunner TestGarbage TestOther TestSimplifyTemplate 9>memcheck.log
cat memcheck.log
valgrind --error-limit=yes --leak-check=full --num-callers=50 --show-reachable=yes --track-origins=yes --gen-suppressions=all --log-fd=9 --error-exitcode=42 ./cppcheck --enable=style,performance,portability,warning,internal --exception-handling --debug-warnings cli lib 9>memcheck_cppcheck.log
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are lacking -D__CPPCHECK__ --error-exitcode=1 --inline-suppr --suppressions-list=.travis_suppressions --library=cppcheck-lib -Ilib -Iexternals/simplecpp/ -Iexternals/tinyxml2/ -Icli --inconclusive for the Cppcheck execution.

Copy link
Collaborator

@firewave firewave Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot another one. Still lacking --template=selfcheck - please rebase to get that and having the build actually fail on an error as #3023 was merged.

We should put the options for the self-check in an external file. Those parameters are (largely) used in multiple places right now and we will most likely have more usages added in the future. With a single place we would make sure all of them are actually in sync. Just having a variable somewhere in the actions would be enough as a start.

When I want to test it locally I always have to copy it out of the YAML file. So having it available from the command-line would be helpful as well.

danmar
danmar previously approved these changes Jan 8, 2021
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this

@amai2012
Copy link
Collaborator Author

@firewave I think the full self-check in sync with other builds is way too slow...

@firewave
Copy link
Collaborator

@firewave I think the full self-check in sync with other builds is way too slow...

We should at least have the full job together so we can either trigger it manually or on a schedule for now - https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#onschedule

If it is working and we know how long it takes we can still optimize and/or integrate a subset into the triggered build - like just running the testrunner.

@firewave
Copy link
Collaborator

I published #8232 which implements this in selfcheck.yml which was already running Callgrind with a reduced selfcheck scenearion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants