llvm-rc: not merging string literals for paths #51286
Comments
|
This project was already using meson |
|
Another, more common instance seems to exist in an rc file provided by wxWidgets (intended to be included in the rc file for programs using wxWidgets) for their default manifest: https://github.com/wxWidgets/wxWidgets/blob/866cdfe20000178145896497c23ce4598cc88068/include/wx/msw/wx.rc#L122 |
jeremyd2019
changed the title
|
What seems to be odd is that the issue Jeremy is referencing is what I'm experiencing in an clang msys2 environment in a ci setup. But locally it builds just fine. Anyways here's the ci logs https://github.com/ZachBacon/visualboyadvance-m/runs/5057487429?check_suite_focus=true |
|
I was able to reproduce this while building GIMP for Windows using Clang in MSYS2. The rc file contents: |
EugeneZelenko
added
the
llvm-tools
|
I finally took some time to finish my half-done attempt at fixing this, and posted it for review at #68685. |
This should hopefully fix this error in the CI: > llvm-rc: Error in ICON statement (ID 1): See: llvm/llvm-project#51286
This should hopefully fix this error in the CI: > llvm-rc: Error in ICON statement (ID 1): See: llvm/llvm-project#51286
A number of constructs allow more than one string literal for what represents one single string - version info values, string tables, user data resources, etc. This mostly works like how a C compiler merges consecutive string literals, like "foo" "bar" as producing the same as "foobar". This is beneficial for producing strings with some elements provided by the preprocessor. MS rc.exe only supports this in a few fixed locations (most of which are already supported), while GNU windres supports this essentially anywhere in any string. See b989fcb for one recent change that extended support for this in one specific resource. A reasonable use case for multiple concatenated string literals that GNU windres accepts is `1 ICON DIR "/name.ico"`, where the directory is provided via the preprocessor, expanding to another string literal; this is llvm#51286. To support this, extend the tokenizer to concatenate consecutive quoted string tokens, when invoked in windres mode. For cases where the difference between narrow and wide strings is significant (e.g. in userdata resources), GNU windres treats `L"aaa" "bbb"` the same as `L"aaabbb"`, contrary to rc.exe which treats it as the wide string `"aaa"` followed by the narrow string `"bbb"`. Similarly, windres rejects the sequence `"aaa" L"bbb"`. However, in contexts where the end result actually is UTF16, GNU windres does accept such mismatched string representations. Therefore, it seems clear that GNU windres doesn't do the merging on the string token level. Many of the existing windres tests happen to use tag-stringtable-basic.rc as test input; this file contains a case of `"wor" L"ld"` which now is rejected by the tokenizer in windres mode - therefore switch those tests, where the actual input file doesn't matter, to a different file.
This should hopefully fix this error in the CI: > llvm-rc: Error in ICON statement (ID 1): See: llvm/llvm-project#51286
This should hopefully fix this error in the CI: > llvm-rc: Error in ICON statement (ID 1): See: llvm/llvm-project#51286
This should hopefully fix this error in the CI: > llvm-rc: Error in ICON statement (ID 1): See: llvm/llvm-project#51286
This should hopefully fix this error in the CI: > llvm-rc: Error in ICON statement (ID 1): See: llvm/llvm-project#51286
GNU windres supports this, while MS rc.exe doesn't. MS rc.exe only supports treating consecutive string literals as if they were fused into one in a few fixed locations (most of which are already supported), while GNU windres supports this essentially anywhere in any string. See b989fcb for one recent change that extended support for this in one specific resource. A reasonable use case for multiple concatenated string literals that GNU windres accepts is `1 ICON DIR "/name.ico"`, where the directory is provided via the preprocessor, expanding to another string literal; this is llvm#51286. Extend the parser to try to consume all consecutive string tokens, whenever reading a filename. Adjust the handling of user data resources read from a file to use the readFilename() helper. While this probably doesn't cover every single case where GNU windres might accept concatenated string literals, this is the primary missing case that has been reported so far.
GNU windres supports this, while MS rc.exe doesn't. MS rc.exe only supports treating consecutive string literals as if they were fused into one in a few fixed locations (most of which are already supported), while GNU windres supports this essentially anywhere in any string. See b989fcb for one recent change that extended support for this in one specific resource. A reasonable use case for multiple concatenated string literals that GNU windres accepts is `1 ICON DIR "/name.ico"`, where the directory is provided via the preprocessor, expanding to another string literal; this is #51286. Extend the parser to try to consume all consecutive string tokens, whenever reading a filename. Adjust the handling of user data resources read from a file to use the readFilename() helper. While this probably doesn't cover every single case where GNU windres might accept concatenated string literals, this is the primary missing case that has been reported so far.
|
Fixed by 6f41510. @mati865 and others - you may want to cherrypick this into msys2. I'm not entirely sure about requesting a backport to 17.x here for this (it's too late for 17.0.3 which is cut tomorrow, and we're pretty far down the release branch already so the threshold for what to backport is pretty high by 17.0.4). Or any opinions from @cjacek or @tru? (Also, theoretically, this could affect using llvm-rc in the MS rc mode, as it interprets things slightly differently, but I think it's mostly theoretical to come up with a legal case that now would be parsed differently?) |
|
It doesn't look like a regression to me. So my vote is for it to wait until 18.x unless I am misunderstanding the PR. |
Yep, not a regression; I was contemplating the backport-worthyness in the category "other small and safe bugfix/improvement", but I agree that this can wait to 18.x. |
It was only used for the gimp-win-a64 job and was coming from MSYS2 repository which already dropped it: msys2/MINGW-packages@a98352b The first patch is still needed as the upstream fix is meant to appear in clang 18 according to bug report, yet our CI still uses clang 17.0.6. See: llvm/llvm-project#51286
It was only used for the gimp-win-a64 job and was coming from MSYS2 repository which already dropped it: msys2/MINGW-packages@a98352b The first patch is still needed as the upstream fix is meant to appear in clang 18 according to bug report, yet our CI still uses clang 17.0.6. See: llvm/llvm-project#51286
Extended Description
I'm not really sure if this is something that only works in windres or if it also works in rc.exe. I came across this rc in virt-viewer
llvm-rc: Error in ICON statement (ID 2):
Is a directory
'['D:/a/temp/msys/msys64/clang64/bin/windres.EXE', '-DICONDIR=\"C://mingw-w64-virt-viewer/src/build-x86_64-w64-mingw32/icons\"', '-DMANIFESTDIR=\"C:/_/mingw-w64-virt-viewer/src/virt-viewer-10.0/src\"', '-i', 'src/virt-viewer.rc', '-o', 'src/virt-viewer-rc.o']' returned non-zero exit status 1.
The RC file in question is https://gitlab.com/virt-viewer/virt-viewer/-/blob/v10.0/src/virt-viewer.rc.in , and the ICON statement is
2 ICON ICONDIR "/virt-viewer.ico"
I assume they meant for that to somehow transform into
2 ICON "C:/_/mingw-w64-virt-viewer/src/build-x86_64-w64-mingw32/icons/virt-viewer.ico". This reminded me of https://reviews.llvm.org/D105621#inline-1004064 but this is in an ICON statement not a user defined resource.The text was updated successfully, but these errors were encountered: