Skip to content

[SPEC] Add compiler check for -fwrapv-pointer in xalancbmk #237

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

Merged

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Apr 22, 2025

-fwrapv-pointer is a relatively recent flag and older versions of clang may not support it.

Hopefully older versions of clang also don't perform the UB optimisation that warrants the use of the flag in the first place

-fwrapv-pointer is a relatively recent flag and older versions of clang may not support it.

Hopefully older versions of clang also don't perform the UB optimisation that warrants the use of the flag in the first place
@citymarina
Copy link

I'm not currently in front of the right machine to test, but would add_cxx_compiler_flag take care of this check for you?

@lukel97
Copy link
Contributor Author

lukel97 commented Apr 23, 2025

I'm not currently in front of the right machine to test, but would add_cxx_compiler_flag take care of this check for you?

It looks like that's only available in the MicroBenchmarks directory, I can't seem to import include(AddCXXCompilerFlag) from the xalancbmk module.

Maybe it would be worthwhile extracting that out in a separate PR so we can use it in other places? It looks useful

@citymarina
Copy link

It looks like that's only available in the MicroBenchmarks directory, I can't seem to import include(AddCXXCompilerFlag) from the xalancbmk module.

Oops, I missed that. LGTM then.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@lukel97 lukel97 merged commit 0c27673 into llvm:main Apr 24, 2025
1 check passed
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