Make the valgrind CI job actually check the test binaries - #1368
Open
ACSimon33 wants to merge 7 commits into
Open
Make the valgrind CI job actually check the test binaries#1368ACSimon33 wants to merge 7 commits into
ACSimon33 wants to merge 7 commits into
Conversation
…Make. The actual test binaries were never instrumented by valgrind because it was not tracing the children of the runtest.cmake wrapper.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1368 +/- ##
=======================================
Coverage 69.01% 69.01%
=======================================
Files 6122 6122
Lines 486123 486124 +1
Branches 23286 23286
=======================================
+ Hits 335514 335515 +1
Misses 150420 150420
Partials 189 189
Continue to review full report in Codecov by Harness.
|
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
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.
Summary
The
memory-checkCI job has been reporting a clean bill of health without everlooking at a LAPACK binary. Every test is registered as a
cmake -P runtest.cmakewrapper invocation, so
ctest -T memcheckran valgrind on cmake, and the realtest executable — started by
execute_process()inside the wrapper — was neverinstrumented at all.
This PR fixes the instrumentation, adds an explicit
LAPACK_MEMORY_CHECKoption toconfigure the test suite for it, and fixes the first three defects the working
memory check reported.
The fix
LAPACK_MEMORY_CHECKon Unix the command issh -c 'exec <binary> …'.execreplaces the shell instead of forking it, so exactly one process everwrites to the log file, and that process is the test binary.
runtest.cmakewrapper unchanged, soWindows, multi-config generators and failing-test output all behave as before.
The three call sites —
BLAS/TESTING,CBLAS/testingandTESTING— now gothrough this one helper instead of each spelling out the wrapper invocation, which
is also how the four near-duplicate
add_test()blocks collapse.New option:
LAPACK_MEMORY_CHECKREQUIRED) instead of quietly doing nothing;LAPACK_TESTING_USE_PYTHON=OFF, because the Python summary is a ctest test of its own and would otherwise have valgrind profile the interpreter rather than LAPACK;execform described above;--trace-children=yes, and narrows--show-leak-kindsfromalltodefinite,indirectto keep the still-reachable noise out of the report.The CI job in
.github/workflows/special.ymlnow passes-D LAPACK_MEMORY_CHECK:BOOL=ONin place of the old-D LAPACK_TESTING_USE_PYTHON:BOOL=OFF.Defects the working memory check found
cblas_sgemvread an uninitializedTAand called Fortran with it.The column-major branch called
cblas_xerbla()for an illegalTransAand thenfell straight through to
F77_sgemv(F77_TA, …)withTAnever assigned — becausereturnwas missing.cblas_xerbladoes not abort under the test harness, so thiswas a live uninitialized read followed by a Fortran call with a garbage
TRANScharacter. The row-major branch of the same routine, and both branches of
dgemv,cgemvandzgemv, already had thereturn; onlycblas_sgemvwas missing it.IWORKwas leaked by all four LIN test drivers.{s,d,c,z}chkaa.FallocateIWORK(34*NMAX)and deallocateE,S,WORKandRWORKon exit but neverIWORK.