This is an archive of the discontinued LLVM Phabricator instance.

[llvm-rc] Don't strictly require quotes around external file names
ClosedPublic

Authored by mstorsjo on May 6 2018, 2:42 PM.

Details

Summary

Regardless of what docs may say, existing resource files in the wild can use this syntax.

I know it isn't too nice to extend the concept of what can be tokenized as an identifier like this (extending it to anything that could be a pathname, like ../foo/bar), but on the other hand, I'd like this to be able to parse existing .rc files that all other rc tools (rc.exe, windres and wrc) can handle just fine. I ran into this issue with https://source.winehq.org/git/wine.git/blob/540c48b91175b11c7b8646d0a036b20c46425080:/programs/winemine/winemine.rc#l109.

Diff Detail

Event Timeline

mstorsjo created this revision.May 6 2018, 2:42 PM
zturner added inline comments.May 7 2018, 9:49 AM
test/tools/llvm-rc/Inputs/include.rc
3

Shouldn't we test both things?

tools/llvm-rc/ResourceScriptParser.h
87

Is it really on the case that MSVC rc special cases this for filenames and not other types of strings?

mstorsjo added inline comments.May 7 2018, 11:04 AM
test/tools/llvm-rc/Inputs/include.rc
3

I would have guessed that there are other tests that successfully include files, but otoh the icon/cursor test is still missing its inputs. I'll update this to make it test both.

tools/llvm-rc/ResourceScriptParser.h
87

I think so, I tested e.g. adding a string in a stringtable without quotes, and that didn't work. Not sure what the exact logic is though.

It would be nicer to request the tokenizer to (re)interpret whatever is next as a filename/path, though, instead of extending the tokenizer's view of what is an identifier. But that's kind of a layering violation (and I don't know if it's doable).

zturner accepted this revision.May 7 2018, 11:06 AM
This revision is now accepted and ready to land.May 7 2018, 11:06 AM
amccarth added inline comments.May 7 2018, 11:07 AM
test/tools/llvm-rc/Inputs/include.rc
3

And perhaps some of the more interesting resource types, like RCDATA, that can take either a filename or a block of raw data.

tools/llvm-rc/ResourceScriptToken.cpp
290

What about backslashes?

101 BITMAP "assets\foo.bmp"
mstorsjo added inline comments.May 7 2018, 11:32 AM
test/tools/llvm-rc/Inputs/include.rc
3

Well RCDATA isn't supported at all yet.

tools/llvm-rc/ResourceScriptToken.cpp
290

I'm not entirely sure how that should be handled, since backslash also is an escape char (and maybe the escape behaviour is different outisde of quotes). I think I'd just ignore backslashes for now...

amccarth added inline comments.May 7 2018, 11:36 AM
test/tools/llvm-rc/Inputs/include.rc
3

OK, but it looks like user-defined types are, and they work the same way as RCDATA.

tools/llvm-rc/ResourceScriptToken.cpp
290

My example was bad because I shouldn't have had the quotation marks.

101 BITMAP assets\foo.bmp

Is backslash an escape character outside of strings? The above line works in Microsoft's rc.exe, and I'm sure I've worked with code that specifies paths like that in resource scripts.

zturner added inline comments.May 7 2018, 11:39 AM
tools/llvm-rc/ResourceScriptToken.cpp
290

Might make sense to handle this in a separate patch that is specifically for correct handling of backslashes in resource scripts, since it sounds like there are potentially some complicated edge cases here.

amccarth accepted this revision.May 7 2018, 11:48 AM
amccarth added inline comments.
tools/llvm-rc/ResourceScriptToken.cpp
290

I'm OK with deferring it, as long as we see a path forward. I hope this doesn't break the approach of looking to see if the next token is a string or identifier.

mstorsjo added inline comments.May 7 2018, 1:50 PM
tools/llvm-rc/ResourceScriptToken.cpp
290

It turned out that backslashes work just fine as such with no extra effort by adding it to the list of allowed chars in that token.

However, a path like dir\bitmap.bmp probably doesn't work when testing on a unix host, so I'm omitting a test for that part.

mstorsjo updated this revision to Diff 145541.May 7 2018, 1:52 PM

Added a separate testcase for including external files without quotes, added handling of backslashes as well.

amccarth accepted this revision.May 7 2018, 2:21 PM

I'm happy. Thanks!

This revision was automatically updated to reflect the committed changes.