[clang] Introduce "binary" StringLiteral for #embed data #127629
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StringLiteral is used as internal data of EmbedExpr and we directly use it as an initializer if a single EmbedExpr appears in the initializer list of a char array. It is fast and convenient, but it is causing problems when string literal character values are checked because #embed data values are within a range [0-2^(char width)] but ordinary StringLiteral is of maybe signed char type. This PR introduces new kind of StringLiteral to hold binary data coming from an embedded resource to mitigate these problems. The new kind of StringLiteral is not assumed to have signed char type. The new kind of StringLiteral also helps to prevent crashes when trying to find StringLiteral token locations since these simply do not exist for binary data. Fixes llvm#119256
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesStringLiteral is used as internal data of EmbedExpr and we directly use it as an initializer if a single EmbedExpr appears in the initializer list of a char array. It is fast and convenient, but it is causing problems when string literal character values are checked because #embed data values are within a range [0-2^(char width)] but ordinary StringLiteral is of maybe signed char type. Fixes #119256 Full diff: https://github.com/llvm/llvm-project/pull/127629.diff 5 Files Affected:
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
cor3ntin
reviewed
Feb 18, 2025
Comment on lines
+4970
to
+4971
| llvm::APInt(N->getValue().getBitWidth(), | ||
| EExpr->Data->BinaryData->getCodeUnit(CurOffset), |
There was a problem hiding this comment.
If you change that line (and nothing else) does it still works?
I have a feeling this is enough
There was a problem hiding this comment.
I wish it did. This particular line helps for generic case of #embed inside of an initializer list. The rest that this patch is adding is for #embed "fast path" where we simply put StringLiteral to the initializer list instead of EmbedExpr when a single #embed is used to initialize char array.
When we do that, the iterators of EmbedExpr won't be in use and the fail from #119256 is still in place.
|
ping |
There was a problem hiding this comment.
Thanks for this! It also needs a release note for the fix. :-)
In general, I think this seems reasonable, but I'd like confirmation from @cor3ntin given his somewhat recent thinking about unevaluated string literals and where those end up touching the rest of the compiler.
cor3ntin
approved these changes
Mar 15, 2025
| @@ -1752,7 +1752,8 @@ enum class StringLiteralKind { | |||
| UTF8, | |||
| UTF16, | |||
| UTF32, | |||
| Unevaluated | |||
| Unevaluated, | |||
| Binary | |||
There was a problem hiding this comment.
Can you add a comment explaining this is for embed?
I'm sorry it took me a while to understand how this patch works.
(The reason is that this allows us to not "cast" to char in getCodeUnitS() - which is only used in C23 mode)
Maybe also add a comment in getCodeUnitS and/or CheckC23ConstexprInitStringLiteral
There was a problem hiding this comment.
I added some comments in 8ff7d03 . Do you think it turned out helpful?
No changelog because I forgot. Should we backport though? |
This seems simple enough to warrant backporting. |
cor3ntin
approved these changes
Mar 20, 2025
| @@ -1752,7 +1752,8 @@ enum class StringLiteralKind { | |||
| UTF8, | |||
| UTF16, | |||
| UTF32, | |||
| Unevaluated | |||
| Unevaluated, | |||
| Binary | |||
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/12901 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/9355 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/9949 Here is the relevant piece of the build log for the reference |
|
Ooops, I'll fix the bots quickly |
Fznamznon
added a commit
to Fznamznon/llvm-project
that referenced
this pull request
Mar 20, 2025
The bots are upset by llvm#127629 . Fix that.
Fznamznon
added a commit
that referenced
this pull request
Mar 20, 2025
The bots are upset by #127629 . Fix that.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 20, 2025
…(#132201) The bots are upset by llvm/llvm-project#127629 . Fix that.
Fznamznon
added a commit
to Fznamznon/llvm-project
that referenced
this pull request
Mar 28, 2025
StringLiteral is used as internal data of EmbedExpr and we directly use it as an initializer if a single EmbedExpr appears in the initializer list of a char array. It is fast and convenient, but it is causing problems when string literal character values are checked because #embed data values are within a range [0-2^(char width)] but ordinary StringLiteral is of maybe signed char type. This PR introduces new kind of StringLiteral to hold binary data coming from an embedded resource to mitigate these problems. The new kind of StringLiteral is not assumed to have signed char type. The new kind of StringLiteral also helps to prevent crashes when trying to find StringLiteral token locations since these simply do not exist for binary data. Fixes llvm#119256
Fznamznon
added a commit
to Fznamznon/llvm-project
that referenced
this pull request
Mar 28, 2025
The bots are upset by llvm#127629 . Fix that.
swift-ci
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 14, 2025
StringLiteral is used as internal data of EmbedExpr and we directly use it as an initializer if a single EmbedExpr appears in the initializer list of a char array. It is fast and convenient, but it is causing problems when string literal character values are checked because #embed data values are within a range [0-2^(char width)] but ordinary StringLiteral is of maybe signed char type. This PR introduces new kind of StringLiteral to hold binary data coming from an embedded resource to mitigate these problems. The new kind of StringLiteral is not assumed to have signed char type. The new kind of StringLiteral also helps to prevent crashes when trying to find StringLiteral token locations since these simply do not exist for binary data. Fixes llvm#119256
swift-ci
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 14, 2025
The bots are upset by llvm#127629 . Fix that.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 15, 2025
…(#132201) The bots are upset by llvm/llvm-project#127629 . Fix that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
StringLiteral is used as internal data of EmbedExpr and we directly use it as an initializer if a single EmbedExpr appears in the initializer list of a char array. It is fast and convenient, but it is causing problems when string literal character values are checked because #embed data values are within a range [0-2^(char width)] but ordinary StringLiteral is of maybe signed char type.
This PR introduces new kind of StringLiteral to hold binary data coming from an embedded resource to mitigate these problems. The new kind of StringLiteral is not assumed to have signed char type. The new kind of StringLiteral also helps to prevent crashes when trying to find StringLiteral token locations since these simply do not exist for binary data.
Fixes #119256