Skip to content

Conversation

@nikic
Copy link
Contributor

@nikic nikic commented Aug 29, 2025

If the macro was previously defined with -Wp,-D then a later -U is not going to take effect, the -Wp flag takes precedence.

Instead use -Wp,-U. This works regardless of whether the original definition was provided via -D or -Wp,-D.

Also make sure these flags only get passed to the C++ compiler -- they are only relevant there, and flang does not support -Wp.

@nikic nikic closed this Aug 29, 2025
@nikic nikic reopened this Aug 29, 2025
@nikic

This comment was marked as outdated.

@nikic nikic marked this pull request as draft August 29, 2025 19:13
@nikic nikic requested a review from Meinersbur September 1, 2025 07:24
@nikic nikic marked this pull request as ready for review September 1, 2025 07:24
@kwk
Copy link
Contributor

kwk commented Sep 2, 2025

For me this patch works. Thank you @nikic for creating it.

@nikic
Copy link
Contributor Author

nikic commented Sep 11, 2025

Ping

To provide a bit of extra context here, this patch is not needed when the LLVM assertions build sets these macros (using -D), this problem shows up in our distro builds where these are injected via CFLAGS with -Wp,-D.

@Meinersbur
Copy link
Member

FLANG_RT_SUPPORTS_UNDEFINE_FLAG checks whether -U is supported (e.g. in msvc it would be /U, but GLIBCXX/LIBCPP does not support those). I think

check_cxx_compiler_flag("-UTESTFLAG" FLANG_RT_SUPPORTS_UNDEFINE_FLAG)

should be updated to test for -Wp,-U

If the macro was previously defined with `-Wp,-D` then a later
`-U` is *not* going to take effect, the `-Wp` flag takes precedence.

Instead use `-Wp,-U`. This works regardless of whether the original
definition was provided via `-D` or `-Wp,-D`.
@nikic nikic force-pushed the flang-rt-glibcxx-assertions branch from cb80f64 to ec8b811 Compare September 11, 2025 13:06
@nikic nikic force-pushed the flang-rt-glibcxx-assertions branch from ec8b811 to 3e34a88 Compare September 11, 2025 13:26
Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@nikic nikic merged commit e1aa2df into llvm:main Sep 11, 2025
9 checks passed
@nikic nikic deleted the flang-rt-glibcxx-assertions branch September 11, 2025 13:47
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