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

[mstorsjo]

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.

[zturner]

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]

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).

[amccarth]

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]

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]

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]

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]

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]

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]

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

[amccarth]

I'm happy. Thanks!

test/tools/llvm-rc/Inputs/include.rc

-// Found because bitmap.bmp is in same directory
-foo BITMAP "bitmap.bmp"
+// Found because bitmap.bmp is in same directory.
+// Test including filenames without quotes.
+foo BITMAP bitmap.bmp

test/tools/llvm-rc/parser.test

 ; PLANGUAGE1:  llvm-rc: Error parsing file: expected ',', got 7
 
 
 ; RUN: not llvm-rc /dry-run /V %p/Inputs/parser-language-too-many-commas.rc 2>&1 | FileCheck %s --check-prefix PLANGUAGE2
 
 ; PLANGUAGE2:  llvm-rc: Error parsing file: expected '-', '~', integer or '(', got ,
 
 
-; RUN: not llvm-rc /dry-run /V %p/Inputs/parser-html-bad-string.rc 2>&1 | FileCheck %s --check-prefix PHTML1
-
-; PHTML1:  llvm-rc: Error parsing file: expected string, got ThisPassesInTheOriginalToolButDocSaysItShouldBeQuoted
-
-
 ; RUN: not llvm-rc /dry-run /V %p/Inputs/parser-html-extra-comma.rc 2>&1 | FileCheck %s --check-prefix PHTML2
 
 ; PHTML2:  llvm-rc: Error parsing file: expected string, got ,
 
 
 ; RUN: not llvm-rc /dry-run /V %p/Inputs/parser-accelerators-bad-flag.rc 2>&1 | FileCheck %s --check-prefix PACCELERATORS1
 
 ; PACCELERATORS1:  llvm-rc: Error parsing file: expected ASCII/VIRTKEY/NOINVERT/ALT/SHIFT/CONTROL, got HELLO

tools/llvm-rc/ResourceScriptParser.h

private:
 
   // The following methods try to read a single token, check if it has the
   // correct type and then parse it.
   // Each integer can be written as an arithmetic expression producing an
   // unsigned 32-bit integer.
   Expected<RCInt> readInt();               // Parse an integer.
   Expected<StringRef> readString();        // Parse a string.
   Expected<StringRef> readIdentifier();    // Parse an identifier.
+  Expected<StringRef> readFilename();      // Parse a filename.
   Expected<IntOrString> readIntOrString(); // Parse an integer or a string.
   Expected<IntOrString> readTypeOrName();  // Parse an integer or an identifier.
 
   // Helper integer expression parsing methods.
   Expected<RCInt> parseIntExpr1();
   Expected<RCInt> parseIntExpr2();
 
   // Advance the state by one, discarding the current token.

tools/llvm-rc/ResourceScriptParser.cpp

 }
 
 Expected<StringRef> RCParser::readString() {
   if (!isNextTokenKind(Kind::String))
     return getExpectedError("string");
   return read().value();
 }
 
+Expected<StringRef> RCParser::readFilename() {
+  if (!isNextTokenKind(Kind::String) && !isNextTokenKind(Kind::Identifier))
+    return getExpectedError("string");
+  return read().value();
+}
+
 Expected<StringRef> RCParser::readIdentifier() {
   if (!isNextTokenKind(Kind::Identifier))
     return getExpectedError("identifier");
   return read().value();
 }
 
 Expected<IntOrString> RCParser::readIntOrString() {
   if (!isNextTokenKind(Kind::Int) && !isNextTokenKind(Kind::String))
ASSIGN_OR_RETURN(
                    AcceleratorsResource::Accelerator::OptionsFlags));
     Accels->addAccelerator(*EventResult, *IDResult, *FlagsResult);
   }
 
   return std::move(Accels);
 }
 
 RCParser::ParseType RCParser::parseCursorResource() {
-  ASSIGN_OR_RETURN(Arg, readString());
+  ASSIGN_OR_RETURN(Arg, readFilename());
   return llvm::make_unique<CursorResource>(*Arg);
 }
 
 RCParser::ParseType RCParser::parseDialogResource(bool IsExtended) {
   // Dialog resources have the following format of the arguments:
   //  DIALOG:   x, y, width, height [opt stmts...] {controls...}
   //  DIALOGEX: x, y, width, height [, helpID] [opt stmts...] {controls...}
   // These are very similar, so we parse them together.
RCParser::ParseType RCParser::parseDialogResource(bool IsExtended) {
   return std::move(Dialog);
 }
 
 RCParser::ParseType RCParser::parseUserDefinedResource(IntOrString Type) {
   if (isEof())
     return getExpectedError("filename, '{' or BEGIN");
 
   // Check if this is a file resource.
-  if (look().kind() == Kind::String)
+  switch (look().kind()) {
+  case Kind::String:
+  case Kind::Identifier:
     return llvm::make_unique<UserDefinedResource>(Type, read().value());
+  default:
+    break;
+  }
 
   RETURN_IF_ERROR(consumeType(Kind::BlockBegin));
   std::vector<IntOrString> Data;
 
   // Consume comma before each consecutive token except the first one.
   bool ConsumeComma = false;
   while (!consumeOptionalType(Kind::BlockEnd)) {
     if (ConsumeComma)
Expected<Control> RCParser::parseControl() {
   };
 
   return Control(*ClassResult, Caption, (*Args)[0], (*Args)[1], (*Args)[2],
                  (*Args)[3], (*Args)[4], TakeOptArg(5), TakeOptArg(6),
                  TakeOptArg(7));
 }
 
 RCParser::ParseType RCParser::parseBitmapResource() {
-  ASSIGN_OR_RETURN(Arg, readString());
+  ASSIGN_OR_RETURN(Arg, readFilename());
   return llvm::make_unique<BitmapResource>(*Arg);
 }
 
 RCParser::ParseType RCParser::parseIconResource() {
-  ASSIGN_OR_RETURN(Arg, readString());
+  ASSIGN_OR_RETURN(Arg, readFilename());
   return llvm::make_unique<IconResource>(*Arg);
 }
 
 RCParser::ParseType RCParser::parseHTMLResource() {
-  ASSIGN_OR_RETURN(Arg, readString());
+  ASSIGN_OR_RETURN(Arg, readFilename());
   return llvm::make_unique<HTMLResource>(*Arg);
 }
 
 RCParser::ParseType RCParser::parseMenuResource() {
   ASSIGN_OR_RETURN(OptStatements, parseOptionalStatements());
   ASSIGN_OR_RETURN(Items, parseMenuItemsList());
   return llvm::make_unique<MenuResource>(std::move(*OptStatements),
                                          std::move(*Items));

tools/llvm-rc/ResourceScriptToken.cpp

 bool Tokenizer::willNowRead(StringRef FollowingChars) const {
   return Data.drop_front(Pos).startswith(FollowingChars);
 }
 
 bool Tokenizer::canStartIdentifier() const {
   assert(!streamEof());
 
   const char CurChar = Data[Pos];
-  return std::isalpha(CurChar) || CurChar == '_';
+  return std::isalpha(CurChar) || CurChar == '_' || CurChar == '.';
 }
 
 bool Tokenizer::canContinueIdentifier() const {
   assert(!streamEof());
   const char CurChar = Data[Pos];
-  return std::isalnum(CurChar) || CurChar == '_';
+  return std::isalnum(CurChar) || CurChar == '_' || CurChar == '.' || CurChar == '/';
 }
 
 bool Tokenizer::canStartInt() const {
   assert(!streamEof());
   return std::isdigit(Data[Pos]);
 }
 
 bool Tokenizer::canStartBlockComment() const {