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 nightly job for x64 Windows with ASan enabled #17087

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 8, 2024

It seems reasonable to have an ASan job on Windows, especially to be able to check Windows specific code. Since the tests may take about 70 minutes, it doesn't make sense to add these for pushes, but for a nightly job that should be okay.


I expect some tests to fail yet (see PR #17086).

It seems reasonable to have an ASan job on Windows, especially to be
able to check Windows specific code.  Since the tests may take about
70 minutes, it doesn't make sense to add these for pushes, but for a
nightly job that should be okay.
@cmb69 cmb69 requested a review from TimWolla as a code owner December 8, 2024 22:08
@cmb69 cmb69 mentioned this pull request Dec 8, 2024
5 tasks
@iluuu1994
Copy link
Member

iluuu1994 commented Dec 8, 2024

Very nice! From what I understand, ASAN is more effective in debug builds (whether that also applies to MSVC, I don't know). However, it seems .github/scripts/windows/build_task.bat always sets --disable-debug-pack, which I assume is the equivalent of --disable-debug? In that case, it might be worth enabling debug builds for this job.

@cmb69
Copy link
Member Author

cmb69 commented Dec 8, 2024

Argh, completely forgot to commit this after having tested locally: a bit of a hack, but ASan builds should also pass --enable-debug-pack which is then effective. The difference between --enable-debug-pack and --enable-debug is that the former only produces debug symbol files, while the latter also disables optimizations (basically -O0) and uses debug CRT DLLs. This slows down testing without real benefits, so --enable-debug is (currently) not even supported for --enable-sanitizer under MSVC (see #16999).

PS: see also https://learn.microsoft.com/en-us/cpp/sanitizers/asan#command-prompt

@cmb69
Copy link
Member Author

cmb69 commented Dec 8, 2024

Cancelled the workflow since it is meaningless anyway for this PR.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust you on the Windows changes, the changes to the GHA workflow LGTM.

@cmb69 cmb69 closed this in 85731e8 Dec 9, 2024
@cmb69 cmb69 deleted the cmb/asan-nightly-win branch December 9, 2024 11:01
@cmb69
Copy link
Member Author

cmb69 commented Dec 9, 2024

Let's see how this goes tonight. :)

@iluuu1994
Copy link
Member

@cmb69 You probably saw, but there were some failures. 🙂
https://github.com/php/php-src/actions/runs/12247662853/job/34165930799

@cmb69
Copy link
Member Author

cmb69 commented Dec 10, 2024

@iluuu1994, yes, these have been expected; see #17086. And sapi_windows_set_ctrl_handler.phpt likely blocked.

@cmb69
Copy link
Member Author

cmb69 commented Dec 10, 2024

Oh, but now I saw that the ASan job was also run for other branches than master, where it makes no sense, since --enable-sanitizer doesn't do anything there (for MSVC). Obviously, https://github.com/php/php-src/pull/17087/files#diff-0d5658b415099a82c11c03a06ca4ec765b4003a1f4b2f3f1943980a882cf8aa6R975 didn't do what I had expected.

Any hints how this can be solved?

@iluuu1994
Copy link
Member

@cmb69 It's only a cosmetic issue, but you can add an input about whether windows should run with asan, and pass this value from root.yml depending on the version. Then you can extend the matrix.asan condition, or even use exclude: to skip the asan job.

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.

3 participants