[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 ↗

(On Diff #145418)

Shouldn't we test both things?

tools/llvm-rc/ResourceScriptParser.h

87 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

What about backslashes?

101 BITMAP "assets\foo.bmp"

[mstorsjo]

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

3 ↗

(On Diff #145418)

Well RCDATA isn't supported at all yet.

tools/llvm-rc/ResourceScriptToken.cpp

290 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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

tools/llvm-rc/ResourceScriptToken.cpp

290 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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 ↗

(On Diff #145418)

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!

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

 // Whether this is found depends on whether the /I flag searches within the
 // "nested" subdirectory
-foo BITMAP "nested-bitmap.bmp"
+foo BITMAP "nested_bitmap.bmp"
 

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

+// Test including filenames without quotes.
+// In this form, the path can't include dashes.
+foo BITMAP ./nested/nested_bitmap.bmp

llvm/trunk/test/tools/llvm-rc/include-paths.test

 ; Should find the bitmap if it is in the same folder as the rc file.
 ; RUN: rm -f %t.include.res
 ; RUN: llvm-rc /FO %t.include.res %p/Inputs/include.rc
 ; RUN: llvm-readobj %t.include.res | FileCheck --check-prefix=FOUND %s
 
+; Try including files without quotes.
+; RUN: rm -f %t.noquotes.res
+; RUN: llvm-rc /FO %t.noquotes.res %p/Inputs/include-noquotes.rc
+; RUN: llvm-readobj %t.noquotes.res | FileCheck --check-prefix=FOUND %s
+
 ; Should find the bitmap if the folder is explicitly specified.
 ; RUN: rm -f %t.nested-include.res
 ; RUN: llvm-rc /FO %t.nested-include.res /I %p/Inputs/nested %p/Inputs/deep-include.rc
 ; RUN: llvm-readobj %t.nested-include.res | FileCheck --check-prefix=FOUND %s
 
 ; Otherwise, it should not find the bitmap.
 ; RUN: rm -f %t.nested-include.res
 ; RUN: not llvm-rc /FO %t.nested-include.res %p/Inputs/deep-include.rc 2>&1 \

 FOUND-NEXT:   0010: 00000000 38000000 00000000 00000000  |....8...........|
 FOUND-NEXT:   0020: 00000000 00000000 5BB3855B B3850000  |........[..[....|
 FOUND-NEXT:   0030: FFFFFFFF FFFF0000 FFFFFFFF FFFF0000  |................|
 FOUND-NEXT:   0040: FFFFFFFF FFFF0000 5BB385FF FFFF0000  |........[.......|
 FOUND-NEXT:   0050: FFFFFF0E C9FF0000 241CEDFF FFFF0000  |........$.......|
 FOUND-NEXT: )
 
 MISSING:      llvm-rc: Error in BITMAP statement (ID foo):
-MISSING-NEXT: error : file not found : nested-bitmap.bmp
+MISSING-NEXT: error : file not found : nested_bitmap.bmp

llvm/trunk/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

llvm/trunk/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.

llvm/trunk/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));

llvm/trunk/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 == '/' || CurChar == '\\';
 }
 
 bool Tokenizer::canStartInt() const {
   assert(!streamEof());
   return std::isdigit(Data[Pos]);
 }
 
 bool Tokenizer::canStartBlockComment() const {