[10/10] [LLD] [COFF] Implement MinGW default manifest handling

[mstorsjo]

Summary

In mingw environments, resources are normally compiled to resource object files directly, instead of letting the linker convert them to COFF format.

Since some time, GCC supports the notion of a default manifest object. When invoking the linker, GCC looks for the default manifest object file, and if found in the expected path, it is added to linker commands.

The default manifest is one that indicates support for the latest known versions of windows, to implicitly unlock the modern behaviours of certain APIs. (No stance taken on whether doing that implicitly is a good thing.)

Not all mingw/gcc distributions include this file, but e.g. in msys2, the default manifest object is distributed in a separate package (which can be but might not always be installed).

This means that even if user projects only use one single resource object file, the linker can end up with two resource object files, and thus needs to support merging them.

The default manifest has a language id of zero, and GNU ld has got logic for dropping a manifest with a zero language id, if there's another manifest present with a nonzero language id. If there are multiple manifests with a nonzero language id, the merging process errors out.

Replicate the same logic from GNU ld in llvm's WindowsResourceParser class. (Should we limit this behaviour to object files, and/or when the mingw flag is used?)

[ruiu]

I think it is fine to make this logic run unconditionally if the logic doesn't harm non-mingw use cases.

llvm/lib/Object/WindowsResource.cpp

254

Can you add a function comment?

266

nit: no else after return.

351

Detecting duplicate manifest objects having the same ID and skipping duplicate zero language IDs while reading an object file seems a bit tricky. Do you think you can do after reading the entire manifest file? If you can, you can separate the logic to parse a file from the logic that you want to add in this patch.

[mstorsjo]

In D66825#1648507, @ruiu wrote:

I think it is fine to make this logic run unconditionally if the logic doesn't harm non-mingw use cases.

Well, I guess it shouldn't harm other use cases no, unless someone tries to link .res files containing multiple manifests. As that is invalid anyway (as far as I've understood from the GNU ld source) it shouldn't really matter though, but it's possible to do right now with lld-link and MS tools

llvm/lib/Object/WindowsResource.cpp

254

Sure

351

I presume you mean "after reading the entire object file"? Yes it's a bit messy to have it integrated in the parsing/adding, but I chose doing it that way for a couple reasons.

One can almost implement most of the same logic by cleaning up the tree after parsing/adding, but there's two details that work less well that way:

1. The current code silently ignores two zero-language manifests, while that would trigger a duplicate if the handling was moved to afterwards. This isn't something that should normally happen (the main usecase is user code having a non-zero language manifest and the default manifest object has a zero-language one) though so it's probably not a big issue.
2. If we first add a zero-language manifest to the resource tree, and later remove it, we'd still have the data for it in the Data vector, which would end up in the binary, unreferenced. But I guess we could remove that entry and iterate over the tree, decrementing DataIndex for any node where it is >= the removed entry.

[mstorsjo]

llvm/lib/Object/WindowsResource.cpp

351

I tried doing this and it probably looks more straightforward than this original version. But I kept a check to allow duplicate entries of manifests with language zero, in case that actually happens, but I tried making it a bit more readable.

[mstorsjo]

Moved the manifest duplicate cleanup to a separate method which is called after parsing and adding nodes to the tree, allowing sharing most of the code for both input file types. I still kept one check for ignoring duplicates of manifests with language zero in the parse phase though (and added a testcase for it) even though I'm unsure if that's a real issue that would arise.

By restructuring the code, it's much easier to limit most of this manifest handling to mingw mode.

[ruiu]

It looks better!

llvm/lib/Object/WindowsResource.cpp

254

Sorry to be a bit too picky, but I'd write more comments as to what the default manifest, why we may have duplicate default manifests, and why we can safely ignore them if there's any other manifest. I'd also explain that this is for mingw.

282

This function doesn't have any blank line, so it looks too dense. I'd insert blank lines at the places where it makes sense.

585

I'd explain what this function is supposed to do -- this function shifts DataIndex of tree nodes to fill a gap created by removing a default language node.

[MaskRay]

lld/test/COFF/merge-resource-manifest.test

13

Does yaml2obj lack any features that prevents it from generating manifest-lang0.o and other object files?

[mstorsjo]

lld/test/COFF/merge-resource-manifest.test

13

Hmm, no, yaml2obj should work fine.

In principle we could also produce them with llvm-rc and llvm-cvtres, but we currently don't have those as test-depends for lld. (And for patch 09/10 I have object files generated with other tools, to test interoperability with them. But a yaml serialization of it should preserve all the necessary detail.

llvm/lib/Object/WindowsResource.cpp

254

Ok, will expand the comment. Currently the explanation is mostly in the commit message, but I'll try to make the individual comments also understandable standalone.

282

Sure, will do.

585

Ah yes, will add a comment here as well.

[mstorsjo]

lld/test/COFF/merge-resource-manifest.test

13

yaml2obj can't synthesize the .res files though, but if you want to, we could add llvm-rc to the list of test dependencies and create it with that. (We do have a number of existing tests with hardcoded .res and resource object files though.)

[mstorsjo]

Changed accoring to review feedback

[thakis]

There are enough highly capable reviewers actively looking at this; resigning from this one :)

[mstorsjo]

@ruiu Anything else on this one?

[ruiu]

No, but I actually wanted Nico to take a look, as Nico is probably more familiar with this code than me.

[mstorsjo]

In D66825#1655204, @ruiu wrote:

No, but I actually wanted Nico to take a look, as Nico is probably more familiar with this code than me.

Ok, fair enough.

@thakis - Your review is (still) wanted here :-)

[thakis]

llvm/include/llvm/Object/WindowsResource.h

160

nit: cleanUpManifests – the verb is "to clean up" (with "cleanup" is the corresponding noun)

llvm/lib/Object/WindowsResource.cpp

310

nit: call "shouldIgnoreDuplicate" and make const; as-is the function sounds like it modifies state

319

same

353

Do we have to add code for this to the merging logic? Or is the implicit manifest always res obj file 0 and we can skip that if there are more than 1 res obj files?

[mstorsjo]

Fixed method naming, made helper const.

[mstorsjo]

llvm/lib/Object/WindowsResource.cpp

353

Yes, we need to handle it on a per-resource basis during/after merging; as GNU ld does support merging resource objs, the user could legitimately be passing multiple resource objects, and we don't know if one of them is user provided or the default manifest (as we don't know if the compiler driver did inject that or not). Additionally, even if there's one user provided resource object and one default manifest, they need to be merged as we don't know if the user provided resource object actually contained a manifest or not.

[thakis]

Looks horrible, please commit :)

llvm/lib/Object/WindowsResource.cpp

316

Should these check for mingw? A normal user-provided resource could probably conceivably have language id 0 as well?

[mstorsjo]

llvm/lib/Object/WindowsResource.cpp

316

Ideally yes, but we don't have the flag available here. We'd need to add it as parameter to the parse functions, passed down recursively. As the harm from it here is pretty small (only one missed warning/error in an obscure corner case) I thought it wasn't worth the churn. But I can add a parameter if you want to.

[thakis]

llvm/lib/Object/WindowsResource.cpp

316

Can't you make it a member variable on WindowsResourceParser?

[mstorsjo]

llvm/lib/Object/WindowsResource.cpp

316

Yes, that works and is simpler. I think I'll push it with a flag named mingw; otherwise the correct name would be "ignoreManifestLangZeroDuplicates", which is more than a mouthful...

lld/COFF/DriverUtils.cpp

for (ObjFile *f : objs) {
     object::ResourceSectionRef rsf;
     if (auto ec = rsf.load(f->getCOFFObj()))
       fatal(toString(f) + ": " + toString(std::move(ec)));
 
     if (auto ec = parser.parse(rsf, f->getName(), duplicates))
       fatal(toString(std::move(ec)));
   }
 
+  if (config->mingw)
+    parser.cleanupManifests(duplicates);
 
   for (const auto &dupeDiag : duplicates)
     if (config->forceMultipleRes)
       warn(dupeDiag);
     else
       error(dupeDiag);
 
   Expected<std::unique_ptr<MemoryBuffer>> e =

lld/test/COFF/Inputs/manifest-lang0.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_LINE_NUMS_STRIPPED ]
+sections:
+  - Name:            .rsrc
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     000000000000000000000000000001001800000018000080000000000000000000000000000001000100000030000080000000000000000000000000000001000000000048000000580000000E00000000000000000000006D616E69666573742D6C616E67300000
+    Relocations:
+      - VirtualAddress:  72
+        SymbolName:      .rsrc
+        Type:            IMAGE_REL_AMD64_ADDR32NB
+symbols:
+  - Name:            .rsrc
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...

lld/test/COFF/Inputs/manifest-lang1.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_LINE_NUMS_STRIPPED ]
+sections:
+  - Name:            .rsrc
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     000000000000000000000000000001001800000018000080000000000000000000000000000001000100000030000080000000000000000000000000000001000100000048000000580000000E00000000000000000000006D616E69666573742D6C616E67310000
+    Relocations:
+      - VirtualAddress:  72
+        SymbolName:      .rsrc
+        Type:            IMAGE_REL_AMD64_ADDR32NB
+symbols:
+  - Name:            .rsrc
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...

lld/test/COFF/Inputs/manifest-lang2.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_LINE_NUMS_STRIPPED ]
+sections:
+  - Name:            .rsrc
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     000000000000000000000000000001001800000018000080000000000000000000000000000001000100000030000080000000000000000000000000000001000200000048000000580000000E00000000000000000000006D616E69666573742D6C616E67320000
+    Relocations:
+      - VirtualAddress:  72
+        SymbolName:      .rsrc
+        Type:            IMAGE_REL_AMD64_ADDR32NB
+symbols:
+  - Name:            .rsrc
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...

lld/test/COFF/merge-resource-manifest.test

+# RUN: yaml2obj < %p/Inputs/ret42.yaml > %t.o
+
+# RUN: yaml2obj < %p/Inputs/manifest-lang0.yaml > %t-manifest-lang0.o
+# RUN: yaml2obj < %p/Inputs/manifest-lang1.yaml > %t-manifest-lang1.o
+# RUN: yaml2obj < %p/Inputs/manifest-lang2.yaml > %t-manifest-lang2.o
+
+# RUN: lld-link /lldmingw /out:%t.exe /entry:main %t.o %p/Inputs/manifest-lang0.res %p/Inputs/manifest-lang1.res
+# RUN: llvm-readobj --coff-resources %t.exe | FileCheck %s
+
+# RUN: lld-link /lldmingw /out:%t.exe /entry:main %t.o %t-manifest-lang0.o %t-manifest-lang1.o
+# RUN: llvm-readobj --coff-resources %t.exe | FileCheck %s
+
+# RUN: lld-link /lldmingw /out:%t.exe /entry:main %t.o %p/Inputs/manifest-lang1.res %p/Inputs/manifest-lang0.res
+# RUN: llvm-readobj --coff-resources %t.exe | FileCheck %s
+
+# RUN: lld-link /lldmingw /out:%t.exe /entry:main %t.o %t-manifest-lang1.o %t-manifest-lang0.o
+# RUN: llvm-readobj --coff-resources %t.exe | FileCheck %s
+
+# RUN: cp %t-manifest-lang0.o %t-manifest-lang0-copy.o
+# RUN: lld-link /lldmingw /out:%t.exe /entry:main %t.o %t-manifest-lang0.o %t-manifest-lang0-copy.o %t-manifest-lang1.o
+# RUN: llvm-readobj --coff-resources %t.exe | FileCheck %s
+
+# RUN: not lld-link /lldmingw /out:%t.exe /entry:main %t.o %p/Inputs/manifest-lang1.res %p/Inputs/manifest-lang2.res 2>&1 | FileCheck --check-prefix=ERROR %s
+
+# RUN: not lld-link /lldmingw /out:%t.exe /entry:main %t.o %t-manifest-lang1.o %t-manifest-lang2.o 2>&1 | FileCheck --check-prefix=ERROR %s
+
+# CHECK:      Resources [
+# CHECK-NEXT:  Total Number of Resources: 1
+# CHECK-NEXT:  Base Table Address: 0x
+# CHECK-NEXT:  {{ }}
+# CHECK-NEXT:   Number of String Entries: 0
+# CHECK-NEXT:   Number of ID Entries: 1
+# CHECK-NEXT:   Type: MANIFEST (ID 24) [
+# CHECK-NEXT:     Table Offset: 0x18
+# CHECK-NEXT:     Number of String Entries: 0
+# CHECK-NEXT:     Number of ID Entries: 1
+# CHECK-NEXT:     Name: (ID 1) [
+# CHECK-NEXT:       Table Offset: 0x30
+# CHECK-NEXT:       Number of String Entries: 0
+# CHECK-NEXT:       Number of ID Entries: 1
+# CHECK-NEXT:       Language: (ID 1) [
+# CHECK-NEXT:         Entry Offset: 0x48
+# CHECK-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+# CHECK-NEXT:         Major Version: 0
+# CHECK-NEXT:         Minor Version: 0
+# CHECK-NEXT:         Characteristics: 0
+# CHECK-NEXT:         Data [
+# CHECK-NEXT:           DataRVA:
+# CHECK-NEXT:           DataSize: 14
+# CHECK-NEXT:           Codepage: 0
+# CHECK-NEXT:           Reserved: 0
+# CHECK-NEXT:           Data (
+# CHECK-NEXT:             0000: 6D616E69 66657374 2D6C616E 6731      |manifest-lang1|
+# CHECK-NEXT:           )
+# CHECK-NEXT:         ]
+# CHECK-NEXT:       ]
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   ]
+# CHECK-NEXT: ]
+
+# ERROR: error: duplicate non-default manifests with languages 1 in {{.*}}manifest-lang1.{{res|o}} and 2 in {{.*}}manifest-lang2.{{res|o}}

llvm/include/llvm/Object/WindowsResource.h

 
 class WindowsResourceParser {
 public:
   class TreeNode;
   WindowsResourceParser();
   Error parse(WindowsResource *WR, std::vector<std::string> &Duplicates);
   Error parse(ResourceSectionRef &RSR, StringRef Filename,
               std::vector<std::string> &Duplicates);
+  void cleanupManifests(std::vector<std::string> &Duplicates);
   void printTree(raw_ostream &OS) const;
   const TreeNode &getTree() const { return Root; }
   const ArrayRef<std::vector<uint8_t>> getData() const { return Data; }
   const ArrayRef<std::vector<UTF16>> getStringTable() const {
     return StringTable;
   }
 
   class TreeNode {
bool addLanguageNode(const ResourceEntryRef &Entry, uint32_t Origin,
                          std::vector<std::vector<uint8_t>> &Data,
                          TreeNode *&Result);
     bool addDataChild(uint32_t ID, uint16_t MajorVersion, uint16_t MinorVersion,
                       uint32_t Characteristics, uint32_t Origin,
                       uint32_t DataIndex, TreeNode *&Result);
     TreeNode &addIDChild(uint32_t ID);
     TreeNode &addNameChild(ArrayRef<UTF16> NameRef,
                            std::vector<std::vector<UTF16>> &StringTable);
+    void shiftDataIndexDown(uint32_t Index);
 
     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;
struct StringOrID {
     StringOrID(ArrayRef<UTF16> String) : IsString(true), String(String) {}
   };
 
 private:
   Error addChildren(TreeNode &Node, ResourceSectionRef &RSR,
                     const coff_resource_dir_table &Table, uint32_t Origin,
                     std::vector<StringOrID> &Context,
                     std::vector<std::string> &Duplicates);
+  bool ignoreDuplicate(const ResourceEntryRef &Entry);
+  bool ignoreDuplicate(const std::vector<StringOrID> &Context);
 
   TreeNode Root;
   std::vector<std::vector<uint8_t>> Data;
   std::vector<std::vector<UTF16>> StringTable;
 
   std::vector<std::string> InputFilenames;
 };
 

llvm/lib/Object/WindowsResource.cpp

if (Context.size() >= 3) {
     OS << "/language ";
     printStringOrID(Context[2], OS, /* IsType */ false, /* IsID */ false);
   }
   OS << ", in " << File1 << " and in " << File2;
 
   return OS.str();
 }
 
+// MinGW specific. Remove default manifests (with language zero) if there are
+// other manifests present, and report an error if there are more than one
+// manifest with a non-zero language code.
+// GCC has the concept of a default manifest resource object, which gets
+// linked in implicitly if present. This default manifest has got language
+// id zero, and should be dropped silently if there's another manifest present.
+// If the user resources surprisignly had a manifest with language id zero,
+// we should also ignore the duplicate default manifest.
+void WindowsResourceParser::cleanupManifests(
+    std::vector<std::string> &Duplicates) {
+  auto TypeIt = Root.IDChildren.find(/* RT_MANIFEST */ 24);
+  if (TypeIt == Root.IDChildren.end())
+    return;
+
+  TreeNode *TypeNode = TypeIt->second.get();
+  auto NameIt =
+      TypeNode->IDChildren.find(/* CREATEPROCESS_MANIFEST_RESOURCE_ID */ 1);
+  if (NameIt == TypeNode->IDChildren.end())
+    return;
+
+  TreeNode *NameNode = NameIt->second.get();
+  if (NameNode->IDChildren.size() <= 1)
+    return; // None or one manifest present, all good.
+
+  // If we hve more than one manifest, drop the language zero one if present,
+  // and check again.
+  auto LangZeroIt = NameNode->IDChildren.find(0);
+  if (LangZeroIt != NameNode->IDChildren.end() &&
+      LangZeroIt->second->IsDataNode) {
+    uint32_t RemovedIndex = LangZeroIt->second->DataIndex;
+    NameNode->IDChildren.erase(LangZeroIt);
+    Data.erase(Data.begin() + RemovedIndex);
+    Root.shiftDataIndexDown(RemovedIndex);
+
+    // If we're now down to one manifest, all is good.
+    if (NameNode->IDChildren.size() <= 1)
+      return;
+  }
+
+  // More than one non-language-zero manifest
+  auto FirstIt = NameNode->IDChildren.begin();
+  uint32_t FirstLang = FirstIt->first;
+  TreeNode *FirstNode = FirstIt->second.get();
+  auto LastIt = NameNode->IDChildren.rbegin();
+  uint32_t LastLang = LastIt->first;
+  TreeNode *LastNode = LastIt->second.get();
+  Duplicates.push_back(
+      ("duplicate non-default manifests with languages " + Twine(FirstLang) +
+       " in " + InputFilenames[FirstNode->Origin] + " and " + Twine(LastLang) +
+       " in " + InputFilenames[LastNode->Origin])
+          .str());
+}
+
+// Ignore duplicates of manifests with language zero (the default manifest),
+// in case the user has provided a manifest with that language id. See
+// the function comment above for context.
+bool WindowsResourceParser::ignoreDuplicate(const ResourceEntryRef &Entry) {
+  return !Entry.checkTypeString() &&
+         Entry.getTypeID() == /* RT_MANIFEST */ 24 &&
+         !Entry.checkNameString() &&
+         Entry.getNameID() == /* CREATEPROCESS_MANIFEST_RESOURCE_ID */ 1 &&
+         Entry.getLanguage() == 0;
+}
+
+bool WindowsResourceParser::ignoreDuplicate(
+    const std::vector<StringOrID> &Context) {
+  return Context.size() == 3 && !Context[0].IsString &&
+         Context[0].ID == /* RT_MANIFEST */ 24 && !Context[1].IsString &&
+         Context[1].ID == /* CREATEPROCESS_MANIFEST_RESOURCE_ID */ 1 &&
+         !Context[2].IsString && Context[2].ID == 0;
+}
+
 Error WindowsResourceParser::parse(WindowsResource *WR,
                                    std::vector<std::string> &Duplicates) {
   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.
Error WindowsResourceParser::parse(WindowsResource *WR,
 
   ResourceEntryRef Entry = EntryOrErr.get();
   uint32_t Origin = InputFilenames.size();
   InputFilenames.push_back(WR->getFileName());
   bool End = false;
   while (!End) {
 
     TreeNode *Node;
     bool IsNewNode = Root.addEntry(Entry, Origin, Data, StringTable, Node);
     if (!IsNewNode) {
+      if (!ignoreDuplicate(Entry))
         Duplicates.push_back(makeDuplicateResourceError(
             Entry, InputFilenames[Node->Origin], WR->getFileName()));
     }
 
     RETURN_IF_ERROR(Entry.moveNext(End));
   }
 
   return Error::success();
 }
 
if (Entry.Offset.isSubDir()) {
                                      Table.MinorVersion, Table.Characteristics,
                                      Origin, Data.size(), Child);
       if (Added) {
         UNWRAP_OR_RETURN(Contents, RSR.getContents(DataEntry));
         Data.push_back(ArrayRef<uint8_t>(
             reinterpret_cast<const uint8_t *>(Contents.data()),
             Contents.size()));
       } else {
+        if (!ignoreDuplicate(Context))
           Duplicates.push_back(makeDuplicateResourceError(
               Context, InputFilenames[Child->Origin], InputFilenames.back()));
       }
       Context.pop_back();
 
     }
   }
   return Error::success();
 }
 
for (auto const &Child : StringChildren) {
     Size += Child.second->getTreeSize();
   }
   for (auto const &Child : IDChildren) {
     Size += Child.second->getTreeSize();
   }
   return Size;
 }
 
+// Shift DataIndex of all data children with an Index greater or equal to the
+// given one, to fill a gap from removing an entry from the Data vector.
+void WindowsResourceParser::TreeNode::shiftDataIndexDown(uint32_t Index) {
+  if (IsDataNode && DataIndex >= Index) {
+    DataIndex--;
+  } else {
+    for (auto &Child : IDChildren)
+      Child.second->shiftDataIndexDown(Index);
+    for (auto &Child : StringChildren)
+      Child.second->shiftDataIndexDown(Index);
+  }
+}
+
 class WindowsResourceCOFFWriter {
 public:
   WindowsResourceCOFFWriter(COFF::MachineTypes MachineType,
                             const WindowsResourceParser &Parser, Error &E);
   std::unique_ptr<MemoryBuffer> write(uint32_t TimeDateStamp);
 
 private:
   void performFileLayout();