Skip to content

Conversation

@vbraun
Copy link

@vbraun vbraun commented Aug 6, 2023

Also tox & some simple tests

@tekknolagi
Copy link
Owner

Hi @vbraun thanks for the contribution! I will merge probably when I get home from my work trip. I have a few broader comments that you can either address before then or I will tackle after:

  • Please don't add new dependencies; unittest should be enough here
  • Please write callgrind files to a tempfile (using TemporaryFile or TemporaryDirectory) in tests; then you don't need any cleanup or gitignore
  • Should the supp file be part of the test directory? Does it need to be used in normal operation? I've never had luck running memcheck against CPython.
  • Related, how likely is the supp file to become out of date?

@vbraun
Copy link
Author

vbraun commented Aug 7, 2023

Just python -m unittest isn't enough, since the tests need to run in a Python instance that runs inside valgrind. Options are basically:

  • You need to remember all the valgrind command line options, good luck (and different tests need different valgrind options)
  • The tests could run valgind (and then a second python instance) as subprocess (Inception)
  • Or there could be a shell script to run valgind python unittest with the right parameters.
  • Or (thats the route I took) use tox which has custom test commands. Tox is also nice in that multiple python versions can be tested easily.

Either way is feasible, let me know which you prefer.

Related, the output file is a bit tricky:

  • The callgrind output file has to be specified on the valgrind commandline
  • valgrind executes Python
  • Python runs tests

So you can't just use TemporaryDirectory. Depending on how you want to launch valgind -> python -> unittest, the directory needs to be chosen first and then passed down to the test.

The suppression file changes rather slowly with CPython versions, but it does. You also need to take the one from the CPython repo and then make edits by hand (as indicated in comments), so you can't just download it. It is also incomplete, with different false positives for different Python versions :-/ . Here again tox is kind of nice since you can at least compare output for different versions easily.

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 23, 2023
sagemathgh-36046: Fix sqrt(sqrt(2)) memory leak in ginac numeric.cpp
    
Fix the memory leak when computing the outer sqrt in `sqrt(sqrt(2))`, as
reported on sage devel at https://groups.google.com/g/sage-
devel/c/6zpxQKXtJgk

The unit test is a WIP and depends on
tekknolagi/valgrind#1

- Resolves sagemath#33074
    
URL: sagemath#36046
Reported by: Volker Braun
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 27, 2023
sagemathgh-36046: Fix sqrt(sqrt(2)) memory leak in ginac numeric.cpp
    
Fix the memory leak when computing the outer sqrt in `sqrt(sqrt(2))`, as
reported on sage devel at https://groups.google.com/g/sage-
devel/c/6zpxQKXtJgk

The unit test is a WIP and depends on
tekknolagi/valgrind#1

- Resolves sagemath#33074
    
URL: sagemath#36046
Reported by: Volker Braun
Reviewer(s): Matthias Köppe
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.

2 participants