Let llvm-cvtres (and lld-link) report duplicate resources

[thakis]

Summary

If two .res files contain the same resource, cvtres.exe (and hence
link.exe) reject the input with this message:

CVTRES : fatal error CVT1100: duplicate resource.  type:STRING, name:101, language:0x0409
LINK : fatal error LNK1123: failure during conversion to COFF: file invalid or corrupt

llvm-cvtres (and lld-link) used to silently pick one of the duplicate
resources instead. This patch makes them report an error as well.
We slightly improve on cvtres by printing the name of two .res files
containing duplicate entries as well.

[thakis]

better name test

[mstorsjo]

LGTM

I guess it's better to keep tests standalone and not use llvm-rc for the input to the test here.

llvm/test/tools/llvm-cvtres/duplicate.test

11 ↗

(On Diff #196367)

What output file does this write into? A temporary file that is removed afterwards - in what directory? (Would it be better to pass a named -out:%t.obj to make sure it doesn't do stray writes outside of the designated directory?)

llvm/tools/llvm-cvtres/llvm-cvtres.cpp

105 ↗

(On Diff #196367)

What does this particular changed line do, with respect to the rest of the patch?

[thakis]

Thanks! Please let me know what to do about the two comments below :)

llvm/test/tools/llvm-cvtres/duplicate.test

11 ↗

(On Diff #196367)

The default output name is the name of the first input file, with .res replaced with .obj, so that's in %t.dir already. Furthermore, the output file is only created once all input files have been parsed successfully, and parsing input files fails here. I can add an explicit out: flag if you think it makes the test easier to read, but it doesn't have an effect.

llvm/tools/llvm-cvtres/llvm-cvtres.cpp

105 ↗

(On Diff #196367)

It's unrelated to the patch. I had read this file while working on this patch, wondered what the false meant, opened the .h file that declares this function to add a /*ArgName=*/ comment, and then realized that false is the default argument for this parameter already, so I figured I'd just remove it. I can land this in a separate commit if you want.

[thakis]

I'll land the false removal in a separate commit and keep the test as-is.

[mstorsjo]

llvm/test/tools/llvm-cvtres/duplicate.test

11 ↗

(On Diff #196367)

Ok, so it's not an issue. Go ahead whichever way you prefer then, it doesn't really hurt this way (and the risk of it becoming an issue later is also minimal), it just came to my mind while reviewing it.

llvm/tools/llvm-cvtres/llvm-cvtres.cpp

105 ↗

(On Diff #196367)

Yes, please do so, it's quite distracting/confusing here.

[uweigand]

It looks like this new test case fails on big-endian systems, like this:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/24725

/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/test/tools/llvm-cvtres/duplicate.test:19:7: error: NAME: expected string not found in input
 NAME: duplicate resource: type "TYPEFOO"/name "NAMEBAR"/language 1033, in {{.*}}name1.res and in {{.*}}name2.res
       ^
 <stdin>:1:1: note: scanning from here
 duplicate resource: type "吀夀倀䔀䘀伀伀"/name "一䄀䴀䔀䈀䄀刀"/language 1033, in /home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/test/tools/llvm-cvtres/Output/duplicate.test.tmp.dir/name1.res and in /home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/test/tools/llvm-cvtres/Output/duplicate.test.tmp.dir/name2.res

This seems like an endian issue when converting (Windows) UTF16 strings. I believe those need to be assumed as default little-endian in the absence of a byte-order mark, even on big-endian system.

Code in WindowsResourceParser::TreeNode::addChild seems to handle this explicitly. We may need a similar fix here.

[thakis]

In D61049#1479100, @uweigand wrote:

It looks like this new test case fails on big-endian systems, like this:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/24725

/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/test/tools/llvm-cvtres/duplicate.test:19:7: error: NAME: expected string not found in input
 NAME: duplicate resource: type "TYPEFOO"/name "NAMEBAR"/language 1033, in {{.*}}name1.res and in {{.*}}name2.res
       ^
 <stdin>:1:1: note: scanning from here
 duplicate resource: type "吀夀倀䔀䘀伀伀"/name "一䄀䴀䔀䈀䄀刀"/language 1033, in /home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/test/tools/llvm-cvtres/Output/duplicate.test.tmp.dir/name1.res and in /home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/test/tools/llvm-cvtres/Output/duplicate.test.tmp.dir/name2.res

This seems like an endian issue when converting (Windows) UTF16 strings. I believe those need to be assumed as default little-endian in the absence of a byte-order mark, even on big-endian system.

Code in WindowsResourceParser::TreeNode::addChild seems to handle this explicitly. We may need a similar fix here.

I tried to fix this in r359414. Sorry for the breakage, and sorry for the slow turnaround, I was away on Friday.

[uweigand]

In D61049#1481906, @thakis wrote:

I tried to fix this in r359414. Sorry for the breakage, and sorry for the slow turnaround, I was away on Friday.

Yes, it looks like the build bots are green again. Thanks!

llvm/trunk/include/llvm/Object/WindowsResource.h

private:
 
     static uint32_t StringCount;
     static uint32_t DataCount;
 
     static std::unique_ptr<TreeNode> createStringNode();
     static std::unique_ptr<TreeNode> createIDNode();
     static std::unique_ptr<TreeNode> createDataNode(uint16_t MajorVersion,
                                                     uint16_t MinorVersion,
-                                                    uint32_t Characteristics);
+                                                    uint32_t Characteristics,
+                                                    uint32_t Origin);
 
     explicit TreeNode(bool IsStringNode);
     TreeNode(uint16_t MajorVersion, uint16_t MinorVersion,
-             uint32_t Characteristics);
+             uint32_t Characteristics, uint32_t Origin);
 
-    void addEntry(const ResourceEntryRef &Entry, bool &IsNewTypeString,
-                  bool &IsNewNameString);
+    bool addEntry(const ResourceEntryRef &Entry, uint32_t Origin,
+                  bool &IsNewTypeString, bool &IsNewNameString,
+                  TreeNode *&Result);
     TreeNode &addTypeNode(const ResourceEntryRef &Entry, bool &IsNewTypeString);
     TreeNode &addNameNode(const ResourceEntryRef &Entry, bool &IsNewNameString);
-    TreeNode &addLanguageNode(const ResourceEntryRef &Entry);
-    TreeNode &addDataChild(uint32_t ID, uint16_t MajorVersion,
-                           uint16_t MinorVersion, uint32_t Characteristics);
+    bool addLanguageNode(const ResourceEntryRef &Entry, uint32_t Origin,
+                         TreeNode *&Result);
+    bool addDataChild(uint32_t ID, uint16_t MajorVersion, uint16_t MinorVersion,
+                      uint32_t Characteristics, uint32_t Origin,
+                      TreeNode *&Result);
     TreeNode &addIDChild(uint32_t ID);
     TreeNode &addNameChild(ArrayRef<UTF16> NameRef, bool &IsNewString);
 
     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;
     uint32_t Characteristics = 0;
+
+    // The .res file that defined this TreeNode, for diagnostics.
+    // Index into InputFilenames.
+    uint32_t Origin;
   };
 
 private:
   TreeNode Root;
   std::vector<std::vector<uint8_t>> Data;
   std::vector<std::vector<UTF16>> StringTable;
+
+  std::vector<std::string> InputFilenames;
 };
 
 Expected<std::unique_ptr<MemoryBuffer>>
 writeWindowsResourceCOFF(llvm::COFF::MachineTypes MachineType,
                          const WindowsResourceParser &Parser);
 
 } // namespace object
 } // namespace llvm
 
 #endif

llvm/trunk/lib/Object/WindowsResource.cpp

Error ResourceEntryRef::loadNext() {
 
   RETURN_IF_ERROR(Reader.padToAlignment(WIN_RES_DATA_ALIGNMENT));
 
   return Error::success();
 }
 
 WindowsResourceParser::WindowsResourceParser() : Root(false) {}
 
+static Error duplicateResourceError(const ResourceEntryRef& Entry,
+                            StringRef File1, StringRef File2) {
+  std::string Ret;
+  raw_string_ostream OS(Ret);
+
+  OS << "duplicate resource:";
+
+  OS << " type ";
+  if (Entry.checkTypeString()) {
+    std::string UTF8;
+    if (!convertUTF16ToUTF8String(Entry.getTypeString(), UTF8))
+      UTF8 = "(failed conversion from UTF16)";
+    OS << '\"' << UTF8 << '\"';
+  } else {
+    OS << "ID " << Entry.getTypeID();
+  }
+
+  OS << "/name ";
+  if (Entry.checkNameString()) {
+    std::string UTF8;
+    if (!convertUTF16ToUTF8String(Entry.getNameString(), UTF8))
+      UTF8 = "(failed conversion from UTF16)";
+    OS << '\"' << UTF8 << '\"';
+  } else {
+    OS << "ID " << Entry.getNameID();
+  }
+
+  OS << "/language " << Entry.getLanguage() << ", in " << File1 << " and in "
+     << File2;
+
+  return make_error<GenericBinaryError>(OS.str(), object_error::parse_failed);
+}
+
 Error WindowsResourceParser::parse(WindowsResource *WR) {
   auto EntryOrErr = WR->getHeadEntry();
   if (!EntryOrErr) {
     auto E = EntryOrErr.takeError();
     if (E.isA<EmptyResError>()) {
       // Check if the .res file contains no entries.  In this case we don't have
       // to throw an error but can rather just return without parsing anything.
       // This applies for files which have a valid PE header magic and the
Error WindowsResourceParser::parse(WindowsResource *WR) {
   ResourceEntryRef Entry = EntryOrErr.get();
   bool End = false;
   while (!End) {
     Data.push_back(Entry.getData());
 
     bool IsNewTypeString = false;
     bool IsNewNameString = false;
 
-    Root.addEntry(Entry, IsNewTypeString, IsNewNameString);
+    TreeNode* Node;
+    bool IsNewNode = Root.addEntry(Entry, InputFilenames.size(),
+                                   IsNewTypeString, IsNewNameString, Node);
+    InputFilenames.push_back(WR->getFileName());
+    if (!IsNewNode)
+      return duplicateResourceError(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");
 }
 
-void WindowsResourceParser::TreeNode::addEntry(const ResourceEntryRef &Entry,
+bool WindowsResourceParser::TreeNode::addEntry(const ResourceEntryRef &Entry,
+                                               uint32_t Origin,
                                                bool &IsNewTypeString,
-                                               bool &IsNewNameString) {
+                                               bool &IsNewNameString,
+                                               TreeNode *&Result) {
   TreeNode &TypeNode = addTypeNode(Entry, IsNewTypeString);
   TreeNode &NameNode = TypeNode.addNameNode(Entry, IsNewNameString);
-  NameNode.addLanguageNode(Entry);
+  return NameNode.addLanguageNode(Entry, Origin, Result);
 }
 
 WindowsResourceParser::TreeNode::TreeNode(bool IsStringNode) {
   if (IsStringNode)
     StringIndex = StringCount++;
 }
 
 WindowsResourceParser::TreeNode::TreeNode(uint16_t MajorVersion,
                                           uint16_t MinorVersion,
-                                          uint32_t Characteristics)
+                                          uint32_t Characteristics,
+                                          uint32_t Origin)
     : IsDataNode(true), MajorVersion(MajorVersion), MinorVersion(MinorVersion),
-      Characteristics(Characteristics) {
-    DataIndex = DataCount++;
+      Characteristics(Characteristics), Origin(Origin) {
 }
 
 std::unique_ptr<WindowsResourceParser::TreeNode>
 WindowsResourceParser::TreeNode::createStringNode() {
   return std::unique_ptr<TreeNode>(new TreeNode(true));
 }
 
 std::unique_ptr<WindowsResourceParser::TreeNode>
 WindowsResourceParser::TreeNode::createIDNode() {
   return std::unique_ptr<TreeNode>(new TreeNode(false));
 }
 
 std::unique_ptr<WindowsResourceParser::TreeNode>
 WindowsResourceParser::TreeNode::createDataNode(uint16_t MajorVersion,
                                                 uint16_t MinorVersion,
-                                                uint32_t Characteristics) {
+                                                uint32_t Characteristics,
+                                                uint32_t Origin) {
   return std::unique_ptr<TreeNode>(
-      new TreeNode(MajorVersion, MinorVersion, Characteristics));
+      new TreeNode(MajorVersion, MinorVersion, Characteristics, Origin));
 }
 
 WindowsResourceParser::TreeNode &
 WindowsResourceParser::TreeNode::addTypeNode(const ResourceEntryRef &Entry,
                                              bool &IsNewTypeString) {
   if (Entry.checkTypeString())
     return addNameChild(Entry.getTypeString(), IsNewTypeString);
   else
     return addIDChild(Entry.getTypeID());
 }
 
 WindowsResourceParser::TreeNode &
 WindowsResourceParser::TreeNode::addNameNode(const ResourceEntryRef &Entry,
                                              bool &IsNewNameString) {
   if (Entry.checkNameString())
     return addNameChild(Entry.getNameString(), IsNewNameString);
   else
     return addIDChild(Entry.getNameID());
 }
 
-WindowsResourceParser::TreeNode &
-WindowsResourceParser::TreeNode::addLanguageNode(
-    const ResourceEntryRef &Entry) {
+bool WindowsResourceParser::TreeNode::addLanguageNode(
+    const ResourceEntryRef &Entry, uint32_t Origin, TreeNode *&Result) {
   return addDataChild(Entry.getLanguage(), Entry.getMajorVersion(),
-                      Entry.getMinorVersion(), Entry.getCharacteristics());
+                      Entry.getMinorVersion(), Entry.getCharacteristics(),
+                      Origin, Result);
 }
 
-WindowsResourceParser::TreeNode &WindowsResourceParser::TreeNode::addDataChild(
+bool WindowsResourceParser::TreeNode::addDataChild(
     uint32_t ID, uint16_t MajorVersion, uint16_t MinorVersion,
-    uint32_t Characteristics) {
-  auto Child = IDChildren.find(ID);
-  if (Child == IDChildren.end()) {
-    auto NewChild = createDataNode(MajorVersion, MinorVersion, Characteristics);
-    WindowsResourceParser::TreeNode &Node = *NewChild;
-    IDChildren.emplace(ID, std::move(NewChild));
-    return Node;
-  } else
-    return *(Child->second);
+    uint32_t Characteristics, uint32_t Origin, TreeNode *&Result) {
+  auto NewChild =
+      createDataNode(MajorVersion, MinorVersion, Characteristics, Origin);
+  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;

llvm/trunk/test/tools/llvm-cvtres/Inputs/id.rc

+stringtable begin
+  42, "hi"
+end

llvm/trunk/test/tools/llvm-cvtres/Inputs/name.rc

+namebar typefoo { "data" }

llvm/trunk/test/tools/llvm-cvtres/duplicate.test

+// Check that cvtres rejects duplicate resources.
+// The input was generated with the following command, using the original Windows
+// rc.exe:
+// > rc /fo id.res /nologo id.rc
+// > rc /fo name.res /nologo name.rc
+
+RUN: rm -rf %t.dir
+RUN: mkdir %t.dir
+RUN: cp %S/Inputs/id.res %t.dir/id1.res
+RUN: cp %S/Inputs/id.res %t.dir/id2.res
+RUN: not llvm-cvtres /machine:X86 %t.dir/id1.res %t.dir/id2.res 2>&1 | \
+RUN:     FileCheck -check-prefix=ID %s
+ID: duplicate resource: type ID 6/name ID 3/language 1033, in {{.*}}id1.res and in {{.*}}id2.res
+
+RUN: cp %S/Inputs/name.res %t.dir/name1.res
+RUN: cp %S/Inputs/name.res %t.dir/name2.res
+RUN: not llvm-cvtres /machine:X86 %t.dir/name1.res %t.dir/name2.res 2>&1 | \
+RUN:     FileCheck -check-prefix=NAME %s
+NAME: duplicate resource: type "TYPEFOO"/name "NAMEBAR"/language 1033, in {{.*}}name1.res and in {{.*}}name2.res