[08/10] [WindowsResource] Remove use of global variables in WindowsResourceParser

[mstorsjo]

Summary

Instead of updating a global variable counter for the next index of strings and data blobs, pass along a reference to actual string vector and let the TreeNode insertion methods add their strings to the vector when a new entry is needed.

Additionally, if the resource tree had duplicates, that were ignored with -force:multipleres in lld, we no longer store all versions of the duplicated resource data, now we only keep the one that actually ends up referenced.

[mstorsjo]

Made the handling of the Data vector similar and consistent with StringTable, as patch [10/10] no longer needs doing it as I did before.

[thakis]

Nice!

llvm/include/llvm/Object/WindowsResource.h

192

Maybe comment what StringIndex and DataIndex are?

llvm/include/llvm/Object/WindowsResource.h

public:
     const Children<uint32_t> &getIDChildren() const { return IDChildren; }
     const Children<std::string> &getStringChildren() const {
       return StringChildren;
     }
 
   private:
     friend class WindowsResourceParser;
 
-    static uint32_t StringCount;
-    static uint32_t DataCount;
-
-    static std::unique_ptr<TreeNode> createStringNode();
+    static std::unique_ptr<TreeNode> createStringNode(uint32_t Index);
     static std::unique_ptr<TreeNode> createIDNode();
     static std::unique_ptr<TreeNode> createDataNode(uint16_t MajorVersion,
                                                     uint16_t MinorVersion,
                                                     uint32_t Characteristics,
-                                                    uint32_t Origin);
+                                                    uint32_t Origin,
+                                                    uint32_t DataIndex);
 
-    explicit TreeNode(bool IsStringNode);
+    explicit TreeNode(uint32_t StringIndex);
     TreeNode(uint16_t MajorVersion, uint16_t MinorVersion,
-             uint32_t Characteristics, uint32_t Origin);
+             uint32_t Characteristics, uint32_t Origin, uint32_t DataIndex);
 
     bool addEntry(const ResourceEntryRef &Entry, uint32_t Origin,
-                  bool &IsNewTypeString, bool &IsNewNameString,
+                  uint32_t DataIndex,
+                  std::vector<std::vector<UTF16>> &StringTable,
                   TreeNode *&Result);
-    TreeNode &addTypeNode(const ResourceEntryRef &Entry, bool &IsNewTypeString);
-    TreeNode &addNameNode(const ResourceEntryRef &Entry, bool &IsNewNameString);
+    TreeNode &addTypeNode(const ResourceEntryRef &Entry,
+                          std::vector<std::vector<UTF16>> &StringTable);
+    TreeNode &addNameNode(const ResourceEntryRef &Entry,
+                          std::vector<std::vector<UTF16>> &StringTable);
     bool addLanguageNode(const ResourceEntryRef &Entry, uint32_t Origin,
-                         TreeNode *&Result);
+                         uint32_t DataIndex, TreeNode *&Result);
     bool addDataChild(uint32_t ID, uint16_t MajorVersion, uint16_t MinorVersion,
                       uint32_t Characteristics, uint32_t Origin,
-                      TreeNode *&Result);
+                      uint32_t DataIndex, TreeNode *&Result);
     TreeNode &addIDChild(uint32_t ID);
-    TreeNode &addNameChild(ArrayRef<UTF16> NameRef, bool &IsNewString);
+    TreeNode &addNameChild(ArrayRef<UTF16> NameRef,
+                           std::vector<std::vector<UTF16>> &StringTable);
 
     bool IsDataNode = false;
     uint32_t StringIndex;
     uint32_t DataIndex;
     Children<uint32_t> IDChildren;
     Children<std::string> StringChildren;
     uint16_t MajorVersion = 0;
     uint16_t MinorVersion = 0;

llvm/lib/Object/WindowsResource.cpp

if (auto EC = X)                                                             \
     return EC;
 
 const uint32_t MIN_HEADER_SIZE = 7 * sizeof(uint32_t) + 2 * sizeof(uint16_t);
 
 // COFF files seem to be inconsistent with alignment between sections, just use
 // 8-byte because it makes everyone happy.
 const uint32_t SECTION_ALIGNMENT = sizeof(uint64_t);
 
-uint32_t WindowsResourceParser::TreeNode::StringCount = 0;
-uint32_t WindowsResourceParser::TreeNode::DataCount = 0;
-
 WindowsResource::WindowsResource(MemoryBufferRef Source)
     : Binary(Binary::ID_WinRes, Source) {
   size_t LeadingSize = WIN_RES_MAGIC_SIZE + WIN_RES_NULL_ENTRY_SIZE;
   BBS = BinaryByteStream(Data.getBuffer().drop_front(LeadingSize),
                          support::little);
 }
 
 // static
if (!EntryOrErr) {
     return E;
   }
 
   ResourceEntryRef Entry = EntryOrErr.get();
   uint32_t Origin = InputFilenames.size();
   InputFilenames.push_back(WR->getFileName());
   bool End = false;
   while (!End) {
-    Data.push_back(Entry.getData());
-
-    bool IsNewTypeString = false;
-    bool IsNewNameString = false;
 
     TreeNode *Node;
-    bool IsNewNode =
-        Root.addEntry(Entry, Origin, IsNewTypeString, IsNewNameString, Node);
-    if (!IsNewNode) {
+    uint32_t DataIndex = Data.size();
+    bool IsNewNode = Root.addEntry(Entry, Origin, DataIndex, StringTable, Node);
+    if (IsNewNode) {
+      Data.push_back(Entry.getData());
+    } else {
       Duplicates.push_back(makeDuplicateResourceError(
           Entry, InputFilenames[Node->Origin], WR->getFileName()));
     }
 
-    if (IsNewTypeString)
-      StringTable.push_back(Entry.getTypeString());
-
-    if (IsNewNameString)
-      StringTable.push_back(Entry.getNameString());
-
     RETURN_IF_ERROR(Entry.moveNext(End));
   }
 
   return Error::success();
 }
 
 void WindowsResourceParser::printTree(raw_ostream &OS) const {
   ScopedPrinter Writer(OS);
   Root.print(Writer, "Resource Tree");
 }
 
-bool WindowsResourceParser::TreeNode::addEntry(const ResourceEntryRef &Entry,
-                                               uint32_t Origin,
-                                               bool &IsNewTypeString,
-                                               bool &IsNewNameString,
-                                               TreeNode *&Result) {
-  TreeNode &TypeNode = addTypeNode(Entry, IsNewTypeString);
-  TreeNode &NameNode = TypeNode.addNameNode(Entry, IsNewNameString);
-  return NameNode.addLanguageNode(Entry, Origin, Result);
+bool WindowsResourceParser::TreeNode::addEntry(
+    const ResourceEntryRef &Entry, uint32_t Origin, uint32_t DataIndex,
+    std::vector<std::vector<UTF16>> &StringTable, TreeNode *&Result) {
+  TreeNode &TypeNode = addTypeNode(Entry, StringTable);
+  TreeNode &NameNode = TypeNode.addNameNode(Entry, StringTable);
+  return NameNode.addLanguageNode(Entry, Origin, DataIndex, Result);
 }
 
-WindowsResourceParser::TreeNode::TreeNode(bool IsStringNode) {
-  if (IsStringNode)
-    StringIndex = StringCount++;
-}
+WindowsResourceParser::TreeNode::TreeNode(uint32_t StringIndex)
+    : StringIndex(StringIndex) {}
 
 WindowsResourceParser::TreeNode::TreeNode(uint16_t MajorVersion,
                                           uint16_t MinorVersion,
                                           uint32_t Characteristics,
-                                          uint32_t Origin)
-    : IsDataNode(true), MajorVersion(MajorVersion), MinorVersion(MinorVersion),
-      Characteristics(Characteristics), Origin(Origin) {
-  DataIndex = DataCount++;
-}
+                                          uint32_t Origin, uint32_t DataIndex)
+    : IsDataNode(true), DataIndex(DataIndex), MajorVersion(MajorVersion),
+      MinorVersion(MinorVersion), Characteristics(Characteristics),
+      Origin(Origin) {}
 
 std::unique_ptr<WindowsResourceParser::TreeNode>
-WindowsResourceParser::TreeNode::createStringNode() {
-  return std::unique_ptr<TreeNode>(new TreeNode(true));
+WindowsResourceParser::TreeNode::createStringNode(uint32_t Index) {
+  return std::unique_ptr<TreeNode>(new TreeNode(Index));
 }
 
 std::unique_ptr<WindowsResourceParser::TreeNode>
 WindowsResourceParser::TreeNode::createIDNode() {
-  return std::unique_ptr<TreeNode>(new TreeNode(false));
+  return std::unique_ptr<TreeNode>(new TreeNode(0));
 }
 
 std::unique_ptr<WindowsResourceParser::TreeNode>
 WindowsResourceParser::TreeNode::createDataNode(uint16_t MajorVersion,
                                                 uint16_t MinorVersion,
                                                 uint32_t Characteristics,
-                                                uint32_t Origin) {
-  return std::unique_ptr<TreeNode>(
-      new TreeNode(MajorVersion, MinorVersion, Characteristics, Origin));
+                                                uint32_t Origin,
+                                                uint32_t DataIndex) {
+  return std::unique_ptr<TreeNode>(new TreeNode(
+      MajorVersion, MinorVersion, Characteristics, Origin, DataIndex));
 }
 
-WindowsResourceParser::TreeNode &
-WindowsResourceParser::TreeNode::addTypeNode(const ResourceEntryRef &Entry,
-                                             bool &IsNewTypeString) {
+WindowsResourceParser::TreeNode &WindowsResourceParser::TreeNode::addTypeNode(
+    const ResourceEntryRef &Entry,
+    std::vector<std::vector<UTF16>> &StringTable) {
   if (Entry.checkTypeString())
-    return addNameChild(Entry.getTypeString(), IsNewTypeString);
+    return addNameChild(Entry.getTypeString(), StringTable);
   else
     return addIDChild(Entry.getTypeID());
 }
 
-WindowsResourceParser::TreeNode &
-WindowsResourceParser::TreeNode::addNameNode(const ResourceEntryRef &Entry,
-                                             bool &IsNewNameString) {
+WindowsResourceParser::TreeNode &WindowsResourceParser::TreeNode::addNameNode(
+    const ResourceEntryRef &Entry,
+    std::vector<std::vector<UTF16>> &StringTable) {
   if (Entry.checkNameString())
-    return addNameChild(Entry.getNameString(), IsNewNameString);
+    return addNameChild(Entry.getNameString(), StringTable);
   else
     return addIDChild(Entry.getNameID());
 }
 
 bool WindowsResourceParser::TreeNode::addLanguageNode(
-    const ResourceEntryRef &Entry, uint32_t Origin, TreeNode *&Result) {
+    const ResourceEntryRef &Entry, uint32_t Origin, uint32_t DataIndex,
+    TreeNode *&Result) {
   return addDataChild(Entry.getLanguage(), Entry.getMajorVersion(),
                       Entry.getMinorVersion(), Entry.getCharacteristics(),
-                      Origin, Result);
+                      Origin, DataIndex, Result);
 }
 
 bool WindowsResourceParser::TreeNode::addDataChild(
     uint32_t ID, uint16_t MajorVersion, uint16_t MinorVersion,
-    uint32_t Characteristics, uint32_t Origin, TreeNode *&Result) {
-  auto NewChild =
-      createDataNode(MajorVersion, MinorVersion, Characteristics, Origin);
+    uint32_t Characteristics, uint32_t Origin, uint32_t DataIndex,
+    TreeNode *&Result) {
+  auto NewChild = createDataNode(MajorVersion, MinorVersion, Characteristics,
+                                 Origin, DataIndex);
   auto ElementInserted = IDChildren.emplace(ID, std::move(NewChild));
   Result = ElementInserted.first->second.get();
   return ElementInserted.second;
 }
 
 WindowsResourceParser::TreeNode &WindowsResourceParser::TreeNode::addIDChild(
     uint32_t ID) {
   auto Child = IDChildren.find(ID);
   if (Child == IDChildren.end()) {
     auto NewChild = createIDNode();
     WindowsResourceParser::TreeNode &Node = *NewChild;
     IDChildren.emplace(ID, std::move(NewChild));
     return Node;
   } else
     return *(Child->second);
 }
 
-WindowsResourceParser::TreeNode &
-WindowsResourceParser::TreeNode::addNameChild(ArrayRef<UTF16> NameRef,
-                                              bool &IsNewString) {
+WindowsResourceParser::TreeNode &WindowsResourceParser::TreeNode::addNameChild(
+    ArrayRef<UTF16> NameRef, std::vector<std::vector<UTF16>> &StringTable) {
   std::string NameString;
   convertUTF16LEToUTF8String(NameRef, NameString);
 
   auto Child = StringChildren.find(NameString);
   if (Child == StringChildren.end()) {
-    auto NewChild = createStringNode();
-    IsNewString = true;
+    auto NewChild = createStringNode(StringTable.size());
+    StringTable.push_back(NameRef);
     WindowsResourceParser::TreeNode &Node = *NewChild;
     StringChildren.emplace(NameString, std::move(NewChild));
     return Node;
   } else
     return *(Child->second);
 }
 
 void WindowsResourceParser::TreeNode::print(ScopedPrinter &Writer,