-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
BLD/CI: Add ASAN suppressions and CI job #23936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4ad1dd5 to
713f23f
Compare
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Adapted from https://github.com/rgommers/scipy/tree/ci-asan Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com> Signed-off-by: Filipe Laíns <lains@riseup.net>
|
Thanks for working on this @FFY00. The job failure should be easy to resolve, it's just missing I'd expect more suppressions are needed, which you may find if you re-run the job multiple times. E.g., #23612 (comment) identified a Also, it's be useful for the suppressions file to gain comments since from the symbol names it's very much non-obvious where the problematic code/component is. Re the |
Sorry, no help here. My approach for fixing these in numpy has been to fix the issues rather than worrying about suppressions. |
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
|
Looks like |
|
Seems like the CI is passing 😊 |
That |
Signed-off-by: Filipe Laíns <lains@riseup.net>
rgommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good! Two minor requests:
- Can you move
asan-ignore.txtundertools/? - There is a serious amount of build log pollution, due to come we can't just update in this repo (e.g.,
f2py, SuperLU). Can you add-Wno-deprecated-declarationstoCFLAGSin the CI job to patch that over?
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
|
The CI is failing due to a flaky job. I am just gonna push a commit removing my accidental whitespace change to |
Signed-off-by: Filipe Laíns <lains@riseup.net>
| # from scipy.optimize._minpack._lmdif | ||
| fun:enorm | ||
|
|
||
| # from scipy.interpolate._dierckx.qr_reduce_periodic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one we just added, would be nice to investigate @czgdp1807 (does not block this PR of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Will look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take this up tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first try for a low-hanging fruit would be to apply Sebastian's fix from CuPy , cupy/cupy#9486
Short version: CuPy version of deBoor_D is a direct port from SciPY; the bug is a ~20 yr old use of unitialized memory; it never gave issues in SciPy but was causing segfaults in CuPy. I think Sebastian is right that in SciPy that unitialized memory is zero-initialized by std::vector<double>, but I did not gdb'd it myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove it tomorrow or on Monday. Will test on my fork first and then send a PR to the main SciPy repository.
Signed-off-by: Filipe Laíns <lains@riseup.net>
|
Seems like I forgot to update the paths when I moved the |
rgommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to get this in, thanks @FFY00!
| fetch-tags: true | ||
| persist-credentials: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there particular motivation for these options here?
Reference issue
Previous discussion in GH-23612.
What does this implement/fix?
This PR adds a Clang sanitizer ignore list (
asan-ignore.txt; used with Clang's-fsanitize-ignorelistoption) covering the existing issues, and adds a CI job that runs the test suite with the address sanitizer enabled.Additional information
The
-fsanitize-ignorelistoption only seems to be able to match the first backtrace entry, so in certain situations, it doesn't seem to be possible to add well-scoped suppressions.Consider the following example from
test_sepfir2d_strided_3, where the error comes from a C++ type template method.It doesn't seem to be possible to add a suppression scoped only to
_fir_mirror_symmetric.To handle edge cases such as this one, I also added a
fail_asanpytest marker, allowing us to easily skip these tests when running the test suite with the address sanitizer.