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

CI: Move sanitizer output into a separate CI step #2511

Merged

Conversation

AtkinsSJ
Copy link
Member

Instead of writing ASAN and UBSAN output into the same stream we use for test logging, direct them to log files, named asan.log.$PID and ubsan.log.$PID, and then output them in a separate CI job that runs afterwards. This should hopefully make it easier to see which tests are failing.

The downside is that it's now harder to tell which tests the *SAN errors are related to. Any ideas on that are appreciated. Right now, I think making the test logs actually readable is more important, (after all, we've had thousands-of-lines-long *SAN outputs for months and not solved those issues,) but maybe other people disagree.

For now, a failing workflow showing the results of this can be seen at https://github.com/AtkinsSJ/ladybird/actions/runs/11979541011?pr=1

@AtkinsSJ
Copy link
Member Author

Lol, I didn't need to link to my fork's failing tests, we've got the genuine san error output here!

@ADKaster
Copy link
Member

We're likely going to leak allll sorts of things unless we take extra care to do 'full' GC collections b/w tests or destroy the VM on exit.

@ADKaster
Copy link
Member

For some actual actionable feedback, I think that we should absolutely have this, and that it failing in the sanitizer step is appropriate.

A blurb of text to say "This failed because of a test, see the test step for more details" might be useful.
And, since there's 6k lines of LSAN output that we aren't sure why it's there, we should disable LSAN for the CI runs specifically. That would be export LSAN_OPTIONS=detect_leaks=0 in the CMakePresets.json(?) and perhaps in the CI yml as well. If that still doesn't disable it, we would want to remove (if present) detect_leaks from the ASAN_OPTIONS.

@ADKaster
Copy link
Member

... and an issue to remind us to look into the detected leaks. I suspect (like I mentioned above) that it's related to not collecting the GC heap, but we have annotations on the GC heap to treat GC memory as LSAN roots. We also have a bunch of leaks that appear to be coming from ImageDecoder.

@AtkinsSJ AtkinsSJ force-pushed the ci-sanitizer-separate-step branch from b968863 to 0c4c6fc Compare December 2, 2024 17:05
@AtkinsSJ
Copy link
Member Author

AtkinsSJ commented Dec 2, 2024

I've added the note to check the Test step. Though, that doesn't seem to print much that's useful other than giving you a rough idea of when it happened.

I've put an issue up about the leak: #2711

Otherwise, disabling it sounds good but I'll leave that for a separate PR, if nothing else than to make sure this one actually does print the output. 😅

Instead of writing ASAN and UBSAN output into the same stream we use for
test logging, direct them to log files, named asan.log.$PID and
ubsan.log.$PID, and then output them in a separate CI job that runs
afterwards. This should hopefully make it easier to see which tests are
failing.

The downside is that it's now harder to tell which tests the *SAN errors
are related to.
@AtkinsSJ AtkinsSJ force-pushed the ci-sanitizer-separate-step branch from 0c4c6fc to 1d51ccd Compare December 4, 2024 13:57
@AtkinsSJ
Copy link
Member Author

AtkinsSJ commented Dec 4, 2024

Changes: I thought putting ::notice in the log would make that line more visible, but it's actually a special workflow command that expects certain arguments, and was just printing ::notice verbatim. So, I've removed that.

The output-reduction Andrew suggested has been implemented separately by Tim in #2624 so I think that's everything addressed.

@AtkinsSJ AtkinsSJ merged commit 1081a7f into LadybirdBrowser:master Dec 4, 2024
4 of 6 checks passed
@AtkinsSJ AtkinsSJ deleted the ci-sanitizer-separate-step branch December 10, 2024 23:41
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