Add a Valgrind GitHub Actions workflow

[wilfwilson]

This is basically a copy of the file in Digraphs: thank you whoever made that!

To see that the workflow functions as expected, see here: https://github.com/wilfwilson/Semigroups/actions/runs/17740854488/job/50414239468

Note that the job fails, because Valgrind does indeed find some issues...

[james-d-mitchell]

I'm not seeing the valgrind job actually running, is that just me?

[james-d-mitchell]

I'm not seeing the valgrind job actually running, is that just me?

Oh right it's only triggered by changes in the kernel module, any chance you can make a white space change so we can see it failing?

[wilfwilson]

@james-d-mitchell It's running! Fun times.

[wilfwilson]

Looks like it failed after running testinstall with acting methods off... with 2834 errors 😱

[james-d-mitchell]

Looks like it failed after running testinstall with acting methods off... with 2834 errors 😱

Zoiks that is not a small number

[wilfwilson]

Hopefully it's the same one error happening 2834 times 🤞

[james-d-mitchell]

I'll have a look on Wednesday

[wilfwilson]

@james-d-mitchell I suggest that you give me sufficient permissions on this repository such that I can fiddle with the respository settings vis-a-vis GitHub Actions.

I propose that we merge this PR, but have the settings for the Valgrind CI job to not fail the PR status checks. i.e. it runs when necessary, and gives a pass/fail status on the relevant job, but it doesn't stop the PR from being merged, and on the lists of pull requests, it doesn't stop a PR from still having a green tick next to it (if everything is fine).

Once we've addressed all of the relevant issues (ahem), we can become strict about making sure that no PR introduces new Valgrind issues.

[Joseph-Edwards]

I've just tried to replicate this valgrind job locally, and my PC is consistently running out of memory whilst running the standard tests with valgrind enabled.

I'm going to do some digging to see if I can find the tests that cause this, and will report back.

[james-d-mitchell]

Thanks @Joseph-Edwards, we could also just run the quick tests, I don't think it's reasonable to expect all the tests to run in a reasonable amount of time/memory with valgrind enabled.

[Joseph-Edwards]

I ran the valgrind job again, this time omitting the tests in tst/standard/fp/tietze.tst. Valgrind managed to complete without running out of memory, and exited with this:

==181341== LEAK SUMMARY:
==181341==    definitely lost: 0 bytes in 0 blocks
==181341==    indirectly lost: 0 bytes in 0 blocks
==181341==      possibly lost: 808 bytes in 8 blocks
==181341==    still reachable: 67,211,879 bytes in 163,635 blocks
==181341==                       of which reachable via heuristic:
==181341==                         stdstring          : 232 bytes in 4 blocks
==181341==         suppressed: 32 bytes in 1 blocks
==181341==
==181341== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)
--181341--
--181341-- used_suppression:      1 dtv-addr-init /usr/libexec/valgrind/default.supp:1581 suppressed: 32 bytes in 1 blocks
==181341==
==181341== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)

The full output can be found here.

I've tried to wrap my head around the 4 errors that we are receiving. I think they're related to the 808 bytes that valgrind claims are "possibly lost". There's a good chance that these are false positives, so we might be best to ignore/suppress them.

From here, we have a few questions we may wish to answer:

1. Why do the tests in tst/standard/fp/tietze.tst consume so much memory when run in valgrind?
2. Are those 808 bytes actually lost?
3. How do we proceed with integrating valgrind into the CI?

Questions 1. and 2. may not be important enough to warrant exploring at the moment.

For question 3., I see a few reasonable options. The first is to not run valgrind on the standard tests at all. Another potential solution is to run valgrind on most of the standard tests apart from the ones in the offending Tietze file, only checking for "definitely lost" and "indirectly lost" leaks.

I think I prefer the second of these two cases, simply because it tests more things. I'm happy to hear other suggestions for how to proceed.

[wilfwilson]

Your preferred option sounds good Joe