Add ASAN CRuby build#653
Conversation
Just saving other people some clicks, that's "Ruby with ASAN", more context in ruby/ruby-dev-builder#10 |
|
@ioquatix From DataDog@0c7206d (found in DataDog/dd-trace-rb#3864) it looks like some more changes are needed? |
|
Seems to be building okay: https://github.com/ruby/setup-ruby/actions/runs/11171164332/job/31055440201?pr=653 |
|
@KJTsanaktsidis are those builds ready to be publicized? One issue is that build is marked as optional since ruby/ruby-dev-builder@19d4327 as it was failing for a while. Also there seems to be some issues with ruby-head since a few days: https://github.com/ruby/ruby-dev-builder/actions/runs/11168122071/job/31045848723 |
|
Maybe it might be best to give me a week or so to try and get everything back into shape here, I haven't touched any of this ASAN stuff for a little while and I should probably get back into it before we pull the trigger perhaps? |
|
On reflection - I think i'm happy to merge this I guess, the only way to shake out the kinks is if a few brave souls give it a spin. Maybe let's soft-launch this without a README update since Samuel has a use-case for it now, and we can work through any issues and come back in a little while and add an entry to the README. |
|
Thanks y'all for looking into this! We had to disable asan testing on DataDog/dd-trace-rb#3915 since the builds were broken for a few days, but I'm happy to work with the fact they are considered experimental and these kinds of things may show up from time to time -- I think they provide enough value as-is ;) |
|
I have reverted ruby/ruby-dev-builder@19d4327 to try to always have builds with ASAN, if there are problems for a few days I might make it optional again. I would also like to get all ruby-dev-builder jobs fixed before merging this. |
|
Thanks everyone! Super excited to try this out! |
|
Looks like it's working quite okay: https://github.com/socketry/io-event/actions/runs/11196675213/job/31125798429?pr=115 |
|
@ioquatix Could you create a PR to document this in the README? That way we don't forget about it and we can merge it after a bit more testing. |
|
Sure: #654 |
**What does this PR do?** This PR reverts #3915 where we temporarily disable memory leak testing using the asan tool because upstream builds were broken. Additionally, since ruby/setup-ruby#653 the setup-ruby action now fully supports the asan builds, so we no longer require our fork. **Motivation:** The asan tool does quite extensive checks in the profiler test suite, so having it running in CI helps catch issues that may otherwise slip by. **Additional Notes:** N/A **How to test the change?** Validate that the "Test for memory leaks -> test-asan" CI job is running and passing successfully.
|
I noticed https://github.com/DataDog/dd-trace-rb/actions/runs/11214528450/job/31169787558 so that seems to report some leaks in the Prism compiler in CRuby e.g. I don't know if it's accurate or maybe some false positives, but either way @ivoanjo could you report that to https://github.com/ruby/prism/issues? |
|
It's interesting @ioquatix's build in https://github.com/socketry/io-event/actions/runs/11196675213/job/31125798429?pr=115 didn't see those. |
|
I think adding documentation is a good idea. |
|
Well spotted indeed! I saw the new prism complaints, but there were failures on a few of our specs caused by latest rubygems/bundler so I decided to tackle those first before circling back to the prism stuff, and then got sidetracked a bit. In particular, maybe prism can also be added to suppressions for now. |
|
I guess this kind of goes back to adding ASAN in ruby/ruby CI, I think @KJTsanaktsidis was working on that. Adding valgrind in ruby/ruby CI would also be good (even if only running Prism tests or some small workload it would still find many things), I suggested that to @kddnewton since the |
|
I want to offer a counter point: ASAN is enabled in |
|
I don’t have a heap to add here other that I’m sorry I haven’t had much time to work on ASAN stuff this week - it should be added to Ruby CI and I do want to do that.
I will say I think this is not the default: https://github.com/ruby/ruby/blob/c43be94f76982d3ffa2ecd28d34172600b81ca31/main.c#L70 I haven’t tackled Asan detected leaks yet (although Peter probably already found most of them lol) |
|
I see, so by default there won't be extra leaks coming from CRuby, so I think that's fine then and OK to document the new build now in the README. |
|
Sorry just to confirm are there currently leaks being reported in the Prism compiler or no? |
|
There were some detected in #653 (comment) and I assume are still there, but I don't know if they are false positives or real leaks. |
|
I managed to repro at ruby/prism#3182 |
Expose this build: https://github.com/ruby/ruby-dev-builder/blob/19d43276e3490260c6785742d5c5b93858149f6d/.github/workflows/build.yml#L138-L167
Example usage: socketry/io-event#115