Skip to content
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 CRuby build #653

Merged
merged 4 commits into from
Oct 5, 2024
Merged

Add ASAN CRuby build #653

merged 4 commits into from
Oct 5, 2024

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Oct 3, 2024

@dentarg
Copy link

dentarg commented Oct 3, 2024

Expose this build: https://github.com/ruby/ruby-dev-builder/blob/19d43276e3490260c6785742d5c5b93858149f6d/.github/workflows/build.yml#L138-L167

Just saving other people some clicks, that's "Ruby with ASAN", more context in ruby/ruby-dev-builder#10

@dentarg
Copy link

dentarg commented Oct 3, 2024

@ioquatix From DataDog@0c7206d (found in DataDog/dd-trace-rb#3864) it looks like some more changes are needed?

@ioquatix
Copy link
Member Author

ioquatix commented Oct 3, 2024

@dentarg thanks for the links and feedback - @ivoanjo thanks for already investigating this!

@ioquatix
Copy link
Member Author

ioquatix commented Oct 3, 2024

@eregon
Copy link
Member

eregon commented Oct 4, 2024

@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

@KJTsanaktsidis
Copy link

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?

@KJTsanaktsidis
Copy link

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.

@ivoanjo
Copy link

ivoanjo commented Oct 4, 2024

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 ;)

@eregon eregon changed the title Update ruby-builder-versions.json Add ASAN CRuby build Oct 4, 2024
@eregon
Copy link
Member

eregon commented Oct 4, 2024

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.

@eregon eregon merged commit 086ffb1 into ruby:master Oct 5, 2024
182 checks passed
@ioquatix ioquatix deleted the patch-1 branch October 5, 2024 22:01
@ioquatix
Copy link
Member Author

ioquatix commented Oct 5, 2024

Thanks everyone! Super excited to try this out!

@ioquatix
Copy link
Member Author

ioquatix commented Oct 5, 2024

@eregon
Copy link
Member

eregon commented Oct 6, 2024

@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.

@ioquatix
Copy link
Member Author

ioquatix commented Oct 6, 2024

Sure: #654

ivoanjo added a commit to DataDog/dd-trace-rb that referenced this pull request Oct 7, 2024
**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.
@eregon
Copy link
Member

eregon commented Oct 10, 2024

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.

 Direct leak of 1232 byte(s) in 22 object(s) allocated from:
    #0 0x5630b0a263dd in calloc (/home/runner/.rubies/ruby-asan/bin/ruby+0xc73dd) (BuildId: d4c818bb4e8ac5804a6050a3d1ebcb0a3ad54afe)
    #1 0x7f83566ee6fa in pm_node_alloc /home/runner/work/ruby-dev-builder/ruby-dev-builder/prism/prism.c:1913:20
    #2 0x7f83566ee6fa in pm_program_node_create /home/runner/work/ruby-dev-builder/ruby-dev-builder/prism/prism.c:6382:31
    #3 0x7f83566ee6fa in parse_program /home/runner/work/ruby-dev-builder/ruby-dev-builder/prism/prism.c:21970:26
    #4 0x7f83566ee6fa in pm_parse /home/runner/work/ruby-dev-builder/ruby-dev-builder/prism/prism.c:22364:12
    #5 0x7f835604a478 in pm_parse_string /home/runner/work/ruby-dev-builder/ruby-dev-builder/./prism_compile.c:11013:23
    #6 0x7f835629d962 in pm_iseq_compile_with_option /home/runner/work/ruby-dev-builder/ruby-dev-builder/iseq.c:1312:17
    #7 0x7f835629d962 in iseqw_s_compile_parser /home/runner/work/ruby-dev-builder/ruby-dev-builder/iseq.c:1520:16

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?

@eregon
Copy link
Member

eregon commented Oct 10, 2024

It's interesting @ioquatix's build in https://github.com/socketry/io-event/actions/runs/11196675213/job/31125798429?pr=115 didn't see those.
Maybe the difference is @ivoanjo's job sets RUBY_FREE_AT_EXIT=1, as well as LSAN_OPTIONS=verbosity=0:log_threads=1:suppressions=pwd/suppressions/lsan.supp ASAN_OPTIONS=detect_leaks=1.
Should we maybe always set (some of?) that for asan builds @KJTsanaktsidis ? Or document it in #654?

@ioquatix
Copy link
Member Author

I think adding documentation is a good idea.

@ivoanjo
Copy link

ivoanjo commented Oct 10, 2024

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.

@eregon
Copy link
Member

eregon commented Oct 10, 2024

I guess this kind of goes back to adding ASAN in ruby/ruby CI, I think @KJTsanaktsidis was working on that.
That should find these Prism compiler cases for instance.
I think it's probably best to not publicize ASAN builds until that's done, as otherwise it'll be basically guaranteed to have ASAN errors which need to be solved in CRuby.

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 memcheck job in ruby/prism has been finding lots of CRuby leaks (unrelated to Prism).

@ioquatix
Copy link
Member Author

ioquatix commented Oct 10, 2024

I want to offer a counter point: ASAN is enabled in io-event gem and I've found bugs using it (including issues in CRuby itself). It has been extremely useful, even in it's current form. Let's not let perfect be the enemy of good enough. As it stands, I believe this build variant can help other native extensions find and fix issues.

@KJTsanaktsidis
Copy link

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.

ASAN_OPTIONS=detect_leaks=1

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)

@eregon
Copy link
Member

eregon commented Oct 11, 2024

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.

@kddnewton
Copy link

Sorry just to confirm are there currently leaks being reported in the Prism compiler or no?

@eregon
Copy link
Member

eregon commented Oct 14, 2024

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.
You'll need this new ruby-asan build (or CRuby built with ASAN) and ASAN_OPTIONS=detect_leaks=1 to repro and maybe other env vars set in that CI job.

@eregon
Copy link
Member

eregon commented Oct 14, 2024

I managed to repro at ruby/prism#3182

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants