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

Fix Clang sanitizers on Windows #17475

Merged
merged 2 commits into from
Jan 17, 2025
Merged

Fix Clang sanitizers on Windows #17475

merged 2 commits into from
Jan 17, 2025

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 14, 2025

As is, sanitizers (ASan and UBSan) can't be enabled for somewhat recent versions of LLVM, and even for older version it would not work for custom installation folders.

Furthermore we cater to #17462.

cmb69 added 2 commits January 14, 2025 23:50
First, more recent versions of Clang do no longer have trailing info in
parentheses, so the detection would fail.  Since we're not interested
in this information, we just ignore it.

Then we should not rely on Clang being installed in the default program
folder.  Instead we look up where the first clang.exe can be found, and
assume that it is located in a bin/ folder in the installation path.
From there we construct the library path, which is
`lib\clang\<ver>\lib\windows` where `<ver>` is either the full version
(older Clang) or only the major version.  Note that this is the case
for stand-alone LLVM installations as well as Visual Studio supplied
ones.

Finally, we clean up by improving the error messages, and removing the
duplicate clang version detection in `add_asan_opts()`.

While we're at it, we also apply a cosmetic improvement to avoid
(trailing) whitespace in the compiler name (e.g. shown by `-v`).
Like for POSIX systems, we pass `-fno-sanitize=function`.

Closes phpGH-17462.
@cmb69 cmb69 marked this pull request as ready for review January 15, 2025 12:01
}

if (TARGET_ARCH != 'x86') {
ret = PROGRAM_FILES + "\\LLVM\\lib\\clang\\" + ver + "\\lib";
var cmd = "cmd /c where clang.exe";
Copy link
Member

Choose a reason for hiding this comment

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

was wondering if llvm-config.exe exists/is well supported on windows but maybe not too useful in those cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I wasn't aware of this, but apparently it is still not shipped with Windows binaries (see https://stackoverflow.com/questions/17096804/where-is-llvm-config-in-windows).

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

fair enough, makes sense as is.

@cmb69 cmb69 merged commit 7dbfacb into php:master Jan 17, 2025
10 checks passed
@cmb69 cmb69 deleted the cmb/clang-sanitizers branch January 17, 2025 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants