[llvm-rc] Allow string table values split into multiple string literals

[mstorsjo]

Summary

This can practically easily be a product of combining strings with macros in resource files.

This fixes https://github.com/mstorsjo/llvm-mingw/issues/140.

As string literals within llvm-rc are handled as StringRefs, each referencing an uninterpreted slice of the input file, with actual interpretation of the input string (codepage handling, unescaping etc) done only right before writing them out to disk, it's hard to concatenate them other than just bundling them up in a vector, without rearchitecting a large part of llvm-rc

This matches how the same already is supported in VersionInfoValue, with a std::vector<IntOrString> Values.

MS rc.exe only supports concatenated string literals in version info values (already supported), string tables (implemented in this patch) and user data resources (easily implemented in a separate patch, but hasn't been requested by any end user yet), while GNU windres supports string immediates split into multiple strings anywhere (e.g. like (100 ICON "myicon" ".ico"). Not sure if concatenation in other statements actually is used in the wild though, in resource files normally built by GNU windres.

I have a separate patch that implements concatenation of all input string literals already at the tokenizer stage, which allows any string token to be split up into separate substrings like with GNU windres, without needing custom code for each resource statement that might contain it. That implementation allows simplifying some of the existing code for version info values as well, but probably is a bit more risky.

@hans - This isn't a regression fix, but should be a fairly safe, backportable improvement, if we're still at a stage when we can take that kind of changes.

[amccarth]

I'll defer to @thakis, but I have a couple suggestions inline.

llvm/tools/llvm-rc/ResourceFileWriter.h

106

Would it be more straightforward to represent the string as a Twine rather than a vector for StringRefs?

162

I don't agree with the name change from singular to plural. Conceptually, it is addjust a single StringID and String into the string table's Bundle. The fact that the string's text may be a concatenation of several pieces doesn't change that.

[mstorsjo]

llvm/tools/llvm-rc/ResourceFileWriter.h

106

No - we need the individual strings stored separarely as we need to call processString on each of them on their own.

162

Ok, good point, will revert to the original name.

[mstorsjo]

llvm/tools/llvm-rc/ResourceFileWriter.h

106

Oh, and also, Twines can't be stored, as they basically just consist of references to the temporary twines/objects that they're concatenated from.

[amccarth]

OK, makes sense. I was hoping the processing could happen as you build up the concatenated string rather than at the end. But now I see the design doesn't really allow for that.

LGTM if you keep insertStringIntoBundle singular.

[mstorsjo]

@hans - ok with this for the branch as well? This fixes a recently reported bug by a user.

[hans]

In D85183#2195426, @mstorsjo wrote:

@hans - ok with this for the branch as well? This fixes a recently reported bug by a user.

Yes, seems safe enough. Pushed as 0835988de17ef8ea70ac790d3d99fea832fcc3e6.

llvm/test/tools/llvm-rc/Inputs/tag-stringtable-basic.rc

 STRINGTABLE
 VERSION 32
 CHARACTERISTICS 0x32 {
   0 "a"
 }
 
 // Commas are optional, so we make sure to test both cases.
 STRINGTABLE {
   1, "b"
   16, "bb"
 }
 
 STRINGTABLE
 VERSION 100
 LANGUAGE 4, 7 {
-  16 "hello"
-  17 "world"
+  16 "hel" "lo"
+  17 "wor" L"ld"
 }
 
 STRINGTABLE
 VERSION 50
 CHARACTERISTICS 0x32 {
   17 "cc"
   32 "ccc"
   2 "c"

llvm/tools/llvm-rc/ResourceFileWriter.h

public:
   } ObjectData;
 
   struct StringTableInfo {
     // Each STRINGTABLE bundle depends on ID of the bundle and language
     // description.
     using BundleKey = std::pair<uint16_t, uint16_t>;
     // Each bundle is in fact an array of 16 strings.
     struct Bundle {
-      std::array<Optional<StringRef>, 16> Data;
+      std::array<Optional<std::vector<StringRef>>, 16> Data;
       ObjectInfo DeclTimeInfo;
       uint16_t MemoryFlags;
       Bundle(const ObjectInfo &Info, uint16_t Flags)
           : DeclTimeInfo(Info), MemoryFlags(Flags) {}
     };
     std::map<BundleKey, Bundle> BundleData;
     // Bundles are listed in the order of their first occurrence.
     std::vector<BundleKey> BundleList;
Error writeMenuDefinition(const std::unique_ptr<MenuDefinition> &,
                             uint16_t Flags);
   Error writeMenuDefinitionList(const MenuDefinitionList &List);
   Error writeMenuBody(const RCResource *);
 
   // StringTableResource
   Error visitStringTableBundle(const RCResource *);
   Error writeStringTableBundleBody(const RCResource *);
   Error insertStringIntoBundle(StringTableInfo::Bundle &Bundle,
-                               uint16_t StringID, StringRef String);
+                               uint16_t StringID,
+                               const std::vector<StringRef> &String);
 
   // User defined resource
   Error writeUserDefinedBody(const RCResource *);
 
   // VersionInfoResource
   Error writeVersionInfoBody(const RCResource *);
   Error writeVersionInfoBlock(const VersionInfoBlock &);
   Error writeVersionInfoValue(const VersionInfoValue &);
 

llvm/tools/llvm-rc/ResourceFileWriter.cpp

public:
   Twine getResourceTypeName() const override { return "STRINGTABLE"; }
 };
 
 Error ResourceFileWriter::visitStringTableBundle(const RCResource *Res) {
   return writeResource(Res, &ResourceFileWriter::writeStringTableBundleBody);
 }
 
 Error ResourceFileWriter::insertStringIntoBundle(
-    StringTableInfo::Bundle &Bundle, uint16_t StringID, StringRef String) {
+    StringTableInfo::Bundle &Bundle, uint16_t StringID,
+    const std::vector<StringRef> &String) {
   uint16_t StringLoc = StringID & 15;
   if (Bundle.Data[StringLoc])
     return createError("Multiple STRINGTABLE strings located under ID " +
                        Twine(StringID));
   Bundle.Data[StringLoc] = String;
   return Error::success();
 }
 
 Error ResourceFileWriter::writeStringTableBundleBody(const RCResource *Base) {
   auto *Res = cast<BundleResource>(Base);
   for (size_t ID = 0; ID < Res->Bundle.Data.size(); ++ID) {
     // The string format is a tiny bit different here. We
     // first output the size of the string, and then the string itself
     // (which is not null-terminated).
-    bool IsLongString;
     SmallVector<UTF16, 128> Data;
-    RETURN_IF_ERROR(processString(Res->Bundle.Data[ID].getValueOr(StringRef()),
-                                  NullHandlingMethod::CutAtDoubleNull,
+    if (Res->Bundle.Data[ID]) {
+      bool IsLongString;
+      for (StringRef S : *Res->Bundle.Data[ID])
+        RETURN_IF_ERROR(processString(S, NullHandlingMethod::CutAtDoubleNull,
                                       IsLongString, Data, Params.CodePage));
-    if (AppendNull && Res->Bundle.Data[ID])
+      if (AppendNull)
         Data.push_back('\0');
+    }
     RETURN_IF_ERROR(
         checkNumberFits<uint16_t>(Data.size(), "STRINGTABLE string size"));
     writeInt<uint16_t>(Data.size());
     for (auto Char : Data)
       writeInt(Char);
   }
   return Error::success();
 }

llvm/tools/llvm-rc/ResourceScriptParser.cpp

RCParser::ParseType RCParser::parseStringTableResource() {
 
   // Read strings until we reach the end of the block.
   while (!consumeOptionalType(Kind::BlockEnd)) {
     // Each definition consists of string's ID (an integer) and a string.
     // Some examples in documentation suggest that there might be a comma in
     // between, however we strictly adhere to the single statement definition.
     ASSIGN_OR_RETURN(IDResult, readInt());
     consumeOptionalType(Kind::Comma);
+
+    std::vector<StringRef> Strings;
     ASSIGN_OR_RETURN(StrResult, readString());
-    Table->addString(*IDResult, *StrResult);
+    Strings.push_back(*StrResult);
+    while (isNextTokenKind(Kind::String))
+      Strings.push_back(read().value());
+
+    Table->addStrings(*IDResult, std::move(Strings));
   }
 
   return std::move(Table);
 }
 
 Expected<std::unique_ptr<VersionInfoBlock>>
 RCParser::parseVersionInfoBlockContents(StringRef BlockName) {
   RETURN_IF_ERROR(consumeType(Kind::BlockBegin));

llvm/tools/llvm-rc/ResourceScriptStmt.h

public:
   }
 };
 
 // STRINGTABLE resource. Contains a list of strings, each having its unique ID.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa381050(v=vs.85).aspx
 class StringTableResource : public OptStatementsRCResource {
 public:
-  std::vector<std::pair<uint32_t, StringRef>> Table;
+  std::vector<std::pair<uint32_t, std::vector<StringRef>>> Table;
 
   StringTableResource(OptionalStmtList &&List, uint16_t Flags)
       : OptStatementsRCResource(std::move(List), Flags) {}
-  void addString(uint32_t ID, StringRef String) {
-    Table.emplace_back(ID, String);
+  void addStrings(uint32_t ID, std::vector<StringRef> &&Strings) {
+    Table.emplace_back(ID, Strings);
   }
   raw_ostream &log(raw_ostream &) const override;
   Twine getResourceTypeName() const override { return "STRINGTABLE"; }
   Error visit(Visitor *V) const override {
     return V->visitStringTableResource(this);
   }
 };
 

llvm/tools/llvm-rc/ResourceScriptStmt.cpp

raw_ostream &MenuResource::log(raw_ostream &OS) const {
   OS << "Menu (" << ResName << "):\n";
   OptStatements->log(OS);
   return Elements.log(OS);
 }
 
 raw_ostream &StringTableResource::log(raw_ostream &OS) const {
   OS << "StringTable:\n";
   OptStatements->log(OS);
-  for (const auto &String : Table)
-    OS << "  " << String.first << " => " << String.second << "\n";
+  for (const auto &String : Table) {
+    OS << "  " << String.first << " =>";
+    for (const auto &S : String.second)
+      OS << " " << S;
+    OS << "\n";
+  }
   return OS;
 }
 
 const StringMap<Control::CtlInfo> Control::SupportedCtls = {
     {"LTEXT", CtlInfo{0x50020000, ClsStatic, true}},
     {"CTEXT", CtlInfo{0x50020001, ClsStatic, true}},
     {"RTEXT", CtlInfo{0x50020002, ClsStatic, true}},
     {"ICON", CtlInfo{0x50000003, ClsStatic, true}},