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

AK: Enable format string checking in Clang builds #24400

Merged
merged 2 commits into from
May 29, 2024

Conversation

BertalanD
Copy link
Member

Format string checking was disabled in Clang-based builds due to a compiler bug: llvm/llvm-project#51182. Now that the requirement has been raised to Clang 17, that is no longer necessary.

This has been tested to work correctly with Apple Clang 15.0.0 (which is the least modern supported compiler), as well as CLion 2024.1's bundled Clangd.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 21, 2024
@BertalanD
Copy link
Member Author

This is unexpected, to say the least:

sql.cpp:(.text._ZN2AK6Format6Detail19CheckedFormatStringIJNS_10ByteStringEEE34check_format_parameter_consistencyILm16ELm1EEEbRAT__Kc[_ZN2AK6Format6Detail19CheckedFormatStringIJNS_10ByteStringEEE34check_format_parameter_consistencyILm16ELm1EEEbRAT__Kc]+0xfc): undefined reference to `auto AK::Format::Detail::count_fmt_params<16ul>(char const (&) [16ul])'

@BertalanD BertalanD marked this pull request as draft May 21, 2024 10:31
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 21, 2024
@trflynn89
Copy link
Member

That workaround looks fine to me if you want to just fixup the commit message

BertalanD added 2 commits May 28, 2024 12:02
There was an issue with Clang that causes `consteval` function calls
from default initializers of fields to be made at run-time. This
manifested itself in the case of `ByteString::formatted` as an undefined
reference to `check_format_parameter_consistency` once format string
checking was enabled for Clang builds. The workaround is simple (just
move it to the member initializer list), and unblocks a useful change.
Format string checking was disabled in Clang-based builds due to a
compiler bug: llvm/llvm-project#51182. Now
that the requirement has been raised to Clang 17, that is no longer
necessary.

This has been tested to work correctly with Apple Clang 15.0.0 (which is
the *least modern* supported compiler), as well as CLion 2024.1's
bundled Clangd.
@BertalanD BertalanD marked this pull request as ready for review May 29, 2024 16:35
@BertalanD BertalanD requested a review from trflynn89 as a code owner May 29, 2024 16:36
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 29, 2024
@ADKaster ADKaster merged commit 637ccac into SerenityOS:master May 29, 2024
12 of 14 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 29, 2024
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