-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add ASAN job in CI #3182
Add ASAN job in CI #3182
Conversation
So in https://github.com/eregon/yarp/actions/runs/11333924655/job/31519044817 there are reported leaks from:
|
I guess one possibility is |
Looks like it was all one leak that @peterzhu2118 fixed. It looks like this is detecting a leak that memcheck isn't. Peter do you know why that might be? |
It looks like this leak is happening inside of Ruby. The heuristics in ruby_memcheck are being used for prism, which filters out memory leaks from Ruby. Line 57 in fd899e8
|
So do I understand this that this is duplicative of the memcheck CI run? |
I think ASAN |
Nice, I guess ruby/ruby@90aa6ae was the fix. I think ideally we'd have ASAN detect_leaks=1 and/or valgrind in CRuby to find leaks in CRuby sooner and catch them directly in CI. Given it's slow probably with a subset of test-all, sounds fine to me and certainly better than nothing. And then in Prism the same, ASAN detect_leaks=1 and/or valgrind, to avoid Prism changes to introduce leaks without noticing. Not sure how much overlap between the two tools though. |
This has been on my todo list but I haven't been able to find the time to push it past the finish line.
The issue is that the Valgrind in the apt repository was too old and had bugs. Looks like the valgrind in Ubuntu 24.04 is finally new enough (>= 3.20.0): https://packages.ubuntu.com/noble/valgrind |
* By using ubuntu-24.04 which has a recent enough valgrind: #3182 (comment)
* By using ubuntu-24.04 which has a recent enough valgrind: #3182 (comment)
* By using ubuntu-24.04 which has a recent enough valgrind: #3182 (comment)
So what is the next step here — should we wait until this is a part of memcheck or try to add this separately? If we're going to add this separately I definitely want to add a suppression for this leak since I don't want it to be red when we merge. |
I think waiting for adding valgrind and/or ASAN in CRuby CI would make most sense. That's also what we need to make |
Another benefit in having this in CI is that it makes it easier for downstream gems (e.g. DataDog/dd-trace-rb#3864 ) to enable ASAN testing without needing to deal with too many exceptions and whatnot ;) |
@ivoanjo Yep, that's what I meant by "That's also what we need to make ruby-asan builds usable with detect_leaks=1".
One idea might be to add it in CI with some suppressions initially and then address these suppressions progressively (potentially by filing separate b.r-l.o bugs about them)? It would prevent new leaks. Otherwise it feels like there will likely be more leaks and maybe never a time where CRuby has no leaks (when running a significant part of the test suite with leak detection). In general this is a bit the wrong place to discuss this, I think it'd make sense to create a https://bugs.ruby-lang.org/ ticket to add leak checking in CRuby CI. @ivoanjo maybe you could do that since you seem most interested in it? |
I'll close this because I don't think it makes sense to merge until CRuby checks leaks in its CI. |
build with a list of leaks: https://github.com/eregon/yarp/actions/runs/11333924655/job/31519044817
cc @KJTsanaktsidis