[lld/COFF] Improve handling of the /manifestdependency: flag

[thakis]

Summary

If multiple /manifestdependency: flags are passed, they are
naively deduped, but after that each of them should have an
effect, instead of just the last one.

Also, /manifestdependency: flags are allowed in .drectve sections
(from #pragma comment(linker, ...). To make the interaction between
/manifestdependency: flags enabling manifest by default but
/manifest:no overriding this work, add an explict ManifestKind::Default
state to represent no explicit /manifest flag being passed.
To make /manifestdependency: flags from input file .drectve sections
work with /manifest:embed, delay embedded manifest emission until
after input files have been read.

This in turn requires undoing D38975: The .res file is now created only
sometimes, and only after the repro response file has been created.
Moving the response file creation is tricky (see review for details), so
move back to having the manifest inputs in the repro file instead. Now
that we don't rely on cvt.exe, that's less bad than it used to be.

[thakis]

lld/test/COFF/manifest.test

82

(This matches link.exe: Even just an additional space right after the : causes the flags to not be uniq'd.)

[thakis]

git add new input file

[thakis]

I didn't see the test failure because linkrepro-manifest.test requires gnutar, which on macOS apparently isn't present (only BSD tar is). The test runs fine without this patch on macOS though; maybe that REQUIRES is overly strict.

Anyways, that test failure is real and kind of annoying. The problem is that my patch moves addBuffer(createManifestRes(), false, false); after the response file has been written, and so the .res file that is internally created by that is added to filePaths only after filePaths has been copied to the response file.

However, /manifestdependency: in input files means we can't create the inputs to the .res file much earlier. So we have to call createResponseFile() later I suppose, but there are several early exits (for writing import .lib files, or for writing thin indexes only) that then also have to call it, which is a bit messy.

Another option, which seems maybe a bit nicer and more consistent with the change here, is to undo D38975 and instead of putting the generated .res file in the response file, put the input flags in it instead. We have llvm-cvtres now, so we no longer need cvtres.exe to link repro files that bundle the inputs.

I like that last idea most.

[thakis]

@pcc too, in case he has opinions on the last comment, given that D38975 was his change :)

[thakis]

Another option is to extract a method that computes the name of the generated .res file and adds it to the response file eagerly, and only do so if we don't create an import lib or thin lto indices only. That'd work too, but it's also a bit brittle since it needs to match early exit patterns.

Yet another option is to call createResponseFile() from the destructor of some RAII class so that it's always created as late as possible. That sounds like an ok option too.

[thakis]

In D108628#2963094, @thakis wrote:

Yet another option is to call createResponseFile() from the destructor of some RAII class so that it's always created as late as possible. That sounds like an ok option too.

One drawback of that is that that'd also add lib files referenced by e.g. /defaultlib: flags in .drectve entries from obj files to the response file, so that when replaying the link from the response file, we'd now see those libs twice (once from the response file, and another time from the .drectve).

It's probably best if the repro is as close as possible to the input, which suggests undoing D38975.

[thakis]

undo D38975

[thakis]

try to fix test on windows where we still call external cvtres

[thakis]

clang-format

[rnk]

lgtm

lld/COFF/Config.h

184

I worry that MSVC may do more things to deduplicate manifest files. They could canonicalize the path first and ignore case, for example. However, I really don't think it matters. The main thing is that repeated, identical directives should not add the same file twice, and this handles that well enough.

[thakis]

Thanks!

lld/COFF/Config.h

184

This flag doesn't contain a path, but raw text that's copied into the manifest xml. As far as I can tell (link main.obj /manifestdependency:foo=bar /manifestdependency:Foo=bar ; type foo.exe.manifest) both an additional space or a difference just in upper/lowercase makes link.exe emit ore than one dependentAssembly element.

lld/COFF/Config.h

 //===- Config.h -------------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_COFF_CONFIG_H
 #define LLD_COFF_CONFIG_H
 
 #include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Support/CachePruning.h"
 #include <cstdint>
 #include <map>
 #include <set>
 #include <string>
enum class ICFLevel {
   None,
   Safe, // Safe ICF for all sections.
   All,  // Aggressive ICF for code, but safe ICF for data, similar to MSVC's
         // behavior.
 };
 
 // Global configuration.
 struct Configuration {
-  enum ManifestKind { SideBySide, Embed, No };
+  enum ManifestKind { Default, SideBySide, Embed, No };
   bool is64() { return machine == AMD64 || machine == ARM64; }
 
   llvm::COFF::MachineTypes machine = IMAGE_FILE_MACHINE_UNKNOWN;
   size_t wordsize;
   bool verbose = false;
   WindowsSubsystem subsystem = llvm::COFF::IMAGE_SUBSYSTEM_UNKNOWN;
   Symbol *entry = nullptr;
   bool noEntry = false;
struct Configuration {
 
   // Used for /merge:from=to (e.g. /merge:.rdata=.text)
   std::map<StringRef, StringRef> merge;
 
   // Used for /section=.name,{DEKPRSW} to set section attributes.
   std::map<StringRef, uint32_t> section;
 
   // Options for manifest files.
-  ManifestKind manifest = No;
+  ManifestKind manifest = Default;
   int manifestID = 1;
-  StringRef manifestDependency;
+  llvm::SetVector<StringRef> manifestDependencies;
   bool manifestUAC = true;
   std::vector<std::string> manifestInput;
   StringRef manifestLevel = "'asInvoker'";
   StringRef manifestUIAccess = "'false'";
   StringRef manifestFile;
 
   // Used for /aligncomm.
   std::map<std::string, int> alignComm;

lld/COFF/Driver.cpp

for (StringRef e : directives.exports) {
     exp.directives = true;
     config->exports.push_back(exp);
   }
 
   // Handle /include: in bulk.
   for (StringRef inc : directives.includes)
     addUndefined(inc);
 
+  // https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=msvc-160
   for (auto *arg : directives.args) {
     switch (arg->getOption().getID()) {
     case OPT_aligncomm:
       parseAligncomm(arg->getValue());
       break;
     case OPT_alternatename:
       parseAlternateName(arg->getValue());
       break;
     case OPT_defaultlib:
       if (Optional<StringRef> path = findLib(arg->getValue()))
         enqueuePath(*path, false, false);
       break;
     case OPT_entry:
       config->entry = addUndefined(mangle(arg->getValue()));
       break;
     case OPT_failifmismatch:
       checkFailIfMismatch(arg->getValue(), file);
       break;
     case OPT_incl:
       addUndefined(arg->getValue());
       break;
+    case OPT_manifestdependency:
+      config->manifestDependencies.insert(arg->getValue());
+      break;
     case OPT_merge:
       parseMerge(arg->getValue());
       break;
     case OPT_nodefaultlib:
       config->noDefaultLibs.insert(doFindLib(arg->getValue()).lower());
       break;
     case OPT_section:
       parseSection(arg->getValue());
static std::string createResponseFile(const opt::InputArgList &args,
 
   for (auto *arg : args) {
     switch (arg->getOption().getID()) {
     case OPT_linkrepro:
     case OPT_reproduce:
     case OPT_INPUT:
     case OPT_defaultlib:
     case OPT_libpath:
-    case OPT_manifest:
-    case OPT_manifest_colon:
-    case OPT_manifestdependency:
-    case OPT_manifestfile:
-    case OPT_manifestinput:
-    case OPT_manifestuac:
       break;
     case OPT_call_graph_ordering_file:
     case OPT_deffile:
+    case OPT_manifestinput:
     case OPT_natvis:
       os << arg->getSpelling() << quote(rewritePath(arg->getValue())) << '\n';
       break;
     case OPT_order: {
       StringRef orderFile = arg->getValue();
       orderFile.consume_front("@");
       os << arg->getSpelling() << '@' << quote(rewritePath(orderFile)) << '\n';
       break;
     }
     case OPT_pdbstream: {
       const std::pair<StringRef, StringRef> nameFile =
           StringRef(arg->getValue()).split("=");
       os << arg->getSpelling() << nameFile.first << '='
          << quote(rewritePath(nameFile.second)) << '\n';
       break;
     }
     case OPT_implib:
+    case OPT_manifestfile:
     case OPT_pdb:
     case OPT_pdbstripped:
     case OPT_out:
       os << arg->getSpelling() << sys::path::filename(arg->getValue()) << "\n";
       break;
     default:
       os << toString(*arg) << "\n";
     }
if (auto *arg = args.getLastArg(OPT_align)) {
     if (!args.hasArg(OPT_driver))
       warn("/align specified without /driver; image may not run");
   }
 
   // Handle /aligncomm
   for (auto *arg : args.filtered(OPT_aligncomm))
     parseAligncomm(arg->getValue());
 
-  // Handle /manifestdependency. This enables /manifest unless /manifest:no is
-  // also passed.
-  if (auto *arg = args.getLastArg(OPT_manifestdependency)) {
-    config->manifestDependency = arg->getValue();
-    config->manifest = Configuration::SideBySide;
-  }
+  // Handle /manifestdependency.
+  for (auto *arg : args.filtered(OPT_manifestdependency))
+    config->manifestDependencies.insert(arg->getValue());
 
   // Handle /manifest and /manifest:
   if (auto *arg = args.getLastArg(OPT_manifest, OPT_manifest_colon)) {
     if (arg->getOption().getID() == OPT_manifest)
       config->manifest = Configuration::SideBySide;
     else
       parseManifest(arg->getValue());
   }
void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   }
 
   // Process files specified as /defaultlib. These should be enequeued after
   // other files, which is why they are in a separate loop.
   for (auto *arg : args.filtered(OPT_defaultlib))
     if (Optional<StringRef> path = findLib(arg->getValue()))
       enqueuePath(*path, false, false);
 
-  // Windows specific -- Create a resource file containing a manifest file.
-  if (config->manifest == Configuration::Embed)
-    addBuffer(createManifestRes(), false, false);
-
   // Read all input files given via the command line.
   run();
 
   if (errorCount())
     return;
 
   // We should have inferred a machine type by now from the input files, but if
   // not we assume x64.
for (auto pair : config->alignComm) {
     auto *dc = dyn_cast<DefinedCommon>(sym);
     if (!dc)
       continue;
 
     CommonChunk *c = dc->getChunk();
     c->setAlignment(std::max(c->getAlignment(), alignment));
   }
 
-  // Windows specific -- Create a side-by-side manifest file.
-  if (config->manifest == Configuration::SideBySide)
+  // Windows specific -- Create an embedded or side-by-side manifest.
+  // /manifestdependency: enables /manifest unless an explicit /manifest:no is
+  // also passed.
+  if (config->manifest == Configuration::Embed)
+    addBuffer(createManifestRes(), false, false);
+  else if (config->manifest == Configuration::SideBySide ||
+           (config->manifest == Configuration::Default &&
+            !config->manifestDependencies.empty()))
     createSideBySideManifest();
 
   // Handle /order. We want to do this at this moment because we
   // need a complete list of comdat sections to warn on nonexistent
   // functions.
   if (auto *arg = args.getLastArg(OPT_order)) {
     if (args.hasArg(OPT_call_graph_ordering_file))
       error("/order and /call-graph-order-file may not be used together");

lld/COFF/DriverUtils.cpp

os << "  <trustInfo>\n"
        << "    <security>\n"
        << "      <requestedPrivileges>\n"
        << "         <requestedExecutionLevel level=" << config->manifestLevel
        << " uiAccess=" << config->manifestUIAccess << "/>\n"
        << "      </requestedPrivileges>\n"
        << "    </security>\n"
        << "  </trustInfo>\n";
   }
-  if (!config->manifestDependency.empty()) {
+  for (auto manifestDependency : config->manifestDependencies) {
     os << "  <dependency>\n"
        << "    <dependentAssembly>\n"
-       << "      <assemblyIdentity " << config->manifestDependency << " />\n"
+       << "      <assemblyIdentity " << manifestDependency << " />\n"
        << "    </dependentAssembly>\n"
        << "  </dependency>\n";
   }
   os << "</assembly>\n";
   return os.str();
 }
 
 static std::string createManifestXmlWithInternalMt(StringRef defaultXml) {
   std::unique_ptr<MemoryBuffer> defaultXmlCopy =
       MemoryBuffer::getMemBufferCopy(defaultXml);
 
   windows_manifest::WindowsManifestMerger merger;
   if (auto e = merger.merge(*defaultXmlCopy.get()))
     fatal("internal manifest tool failed on default xml: " +
           toString(std::move(e)));
 
   for (StringRef filename : config->manifestInput) {
     std::unique_ptr<MemoryBuffer> manifest =
         check(MemoryBuffer::getFile(filename));
-    if (auto e = merger.merge(*manifest.get()))
+    // Call takeBuffer to include in /reproduce: output if applicable.
+    if (auto e = merger.merge(driver->takeBuffer(std::move(manifest))))
       fatal("internal manifest tool failed on file " + filename + ": " +
             toString(std::move(e)));
   }
 
   return std::string(merger.getMergedManifest().get()->getBuffer());
 }
 
 static std::string createManifestXmlWithExternalMt(StringRef defaultXml) {
static std::string createManifestXmlWithExternalMt(StringRef defaultXml) {
   TemporaryFile user("user", "manifest");
 
   Executor e("mt.exe");
   e.add("/manifest");
   e.add(Default.path);
   for (StringRef filename : config->manifestInput) {
     e.add("/manifest");
     e.add(filename);
+
+    // Manually add the file to the /reproduce: tar if needed.
+    if (driver->tar)
+      if (auto mbOrErr = MemoryBuffer::getFile(filename))
+        driver->takeBuffer(std::move(*mbOrErr));
   }
   e.add("/nologo");
   e.add("/out:" + StringRef(user.path));
   e.run();
 
   return std::string(
       CHECK(MemoryBuffer::getFile(user.path), "could not open " + user.path)
           .get()

lld/test/COFF/Inputs/manifestdependency-drectve.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: []
+sections:
+  - Name:            .drectve
+    Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
+    Alignment:       1
+    # "/manifestdependency:foo='bar'" "/manifestdependency:baz='quux'"
+    # (pipe into `xxd -p -r` to recover raw contents; pipe into `xxd -p` to put
+    # something new here.)
+    SectionData: 222f6d616e6966657374646570656e64656e63793a666f6f3d27626172272220222f6d616e6966657374646570656e64656e63793a62617a3d27717575782722
+symbols:
+...

lld/test/COFF/linkrepro-manifest.test

 REQUIRES: x86, gnutar, manifest_tool
 
-manifest-related files are compiled to a .res file and the .res file is
-added to the repro archive, instead of adding the inputs.
-
 RUN: rm -rf %t && mkdir %t && cd %t
 RUN: lld-link -entry:__ImageBase -nodefaultlib -linkrepro:%t \
 RUN:     -manifest:embed %p/Inputs/std32.lib -subsystem:console \
 RUN:     -manifestinput:%p/Inputs/manifestinput.test
 
 RUN: tar tf repro.tar | FileCheck --check-prefix=LIST %s
-RUN: tar xOf repro.tar repro/response.txt | FileCheck %s
+RUN: tar xOf repro.tar repro/response.txt \
+RUN:     | FileCheck --implicit-check-not=.manifest.res %s
 
-LIST: manifest.res
+LIST: {{.*}}manifestinput.test
 
-CHECK-NOT: -manifest:
-CHECK: .manifest.res
-CHECK-NOT: -manifest:
+CHECK-DAG: -manifest:embed
+CHECK-DAG: -manifestinput:{{.*}}manifestinput.test

lld/test/COFF/manifest.test

 
 # RUN: lld-link /manifest /out:%t.exe /entry:main /manifestuac:no %t.obj
 # RUN: FileCheck -check-prefix=NOUACNODEP %s < %t.exe.manifest
 
 NOUACNODEP: <?xml version="1.0" standalone="yes"?>
 NOUACNODEP: <assembly xmlns="urn:schemas-microsoft-com:asm.v1"
 NOUACNODEP:           manifestVersion="1.0">
 NOUACNODEP: </assembly>
+
+# Several /manifestdependency: flags are naively dedup'd.
+# RUN: lld-link /out:%t.exe /entry:main \
+# RUN:   /manifestdependency:"foo='bar'" \
+# RUN:   /manifestdependency:"foo='bar'" \
+# RUN:   /manifestdependency:"baz='quux'" \
+# RUN:   %t.obj
+# RUN: FileCheck -check-prefix=SEVERALDEPS %s < %t.exe.manifest
+
+SEVERALDEPS: <?xml version="1.0" standalone="yes"?>
+SEVERALDEPS: <assembly xmlns="urn:schemas-microsoft-com:asm.v1"
+SEVERALDEPS:           manifestVersion="1.0">
+SEVERALDEPS:   <trustInfo>
+SEVERALDEPS:     <security>
+SEVERALDEPS:       <requestedPrivileges>
+SEVERALDEPS:          <requestedExecutionLevel level='asInvoker' uiAccess='false'/>
+SEVERALDEPS:       </requestedPrivileges>
+SEVERALDEPS:     </security>
+SEVERALDEPS:   </trustInfo>
+SEVERALDEPS:   <dependency>
+SEVERALDEPS:     <dependentAssembly>
+SEVERALDEPS:       <assemblyIdentity foo='bar' />
+SEVERALDEPS:     </dependentAssembly>
+SEVERALDEPS:   <dependency>
+SEVERALDEPS:     <dependentAssembly>
+SEVERALDEPS:       <assemblyIdentity baz='quux' />
+SEVERALDEPS:     </dependentAssembly>
+SEVERALDEPS:   </dependency>
+SEVERALDEPS: </assembly>
+
+# /manifestdependency: flags can be in .drectve sections.
+# RUN: yaml2obj %p/Inputs/manifestdependency-drectve.yaml -o %t.dir.obj
+# RUN: rm %t.exe.manifest
+# RUN: lld-link /out:%t.exe /entry:main \
+# RUN:   %t.obj %t.dir.obj
+# RUN: FileCheck -check-prefix=SEVERALDEPS %s < %t.exe.manifest
+
+# /manifestdependency: flags in .drectve sections are ignored with an
+# explicit /manifest:no.
+# RUN: rm %t.exe.manifest
+# RUN: lld-link /out:%t.exe /entry:main /manifest:no \
+# RUN:   %t.obj %t.dir.obj
+# RUN: test ! -e %t.exe.manifest
+
+# Test that /manifestdependency: flags in .drectve sections work
+# with /manifest:embed too.
+# RUN: lld-link /out:%t.exe /entry:main /manifest:embed \
+# RUN:   %t.obj %t.dir.obj
+# RUN: test ! -e %t.exe.manifest
+# RUN: llvm-readobj --coff-resources %t.exe \
+# RUN:     | FileCheck --check-prefix EMBED %s
+
+EMBED: Data (
+EMBED:   0000: 3C3F786D 6C207665 7273696F 6E3D2231  |<?xml version="1|
+EMBED:   0010: 2E302220 7374616E 64616C6F 6E653D22  |.0" standalone="|
+EMBED:   0020: 79657322 3F3E0A3C 61737365 6D626C79  |yes"?>.<assembly|
+EMBED:   0030: 20786D6C 6E733D22 75726E3A 73636865  | xmlns="urn:sche|
+EMBED:   0040: 6D61732D 6D696372 6F736F66 742D636F  |mas-microsoft-co|
+EMBED:   0050: 6D3A6173 6D2E7631 220A2020 20202020  |m:asm.v1".      |
+EMBED:   0060: 20202020 6D616E69 66657374 56657273  |    manifestVers|
+EMBED:   0070: 696F6E3D 22312E30 223E0A20 203C7472  |ion="1.0">.  <tr|
+EMBED:   0080: 75737449 6E666F3E 0A202020 203C7365  |ustInfo>.    <se|
+EMBED:   0090: 63757269 74793E0A 20202020 20203C72  |curity>.      <r|
+EMBED:   00A0: 65717565 73746564 50726976 696C6567  |equestedPrivileg|
+EMBED:   00B0: 65733E0A 20202020 20202020 203C7265  |es>.         <re|
+EMBED:   00C0: 71756573 74656445 78656375 74696F6E  |questedExecution|
+EMBED:   00D0: 4C657665 6C206C65 76656C3D 27617349  |Level level='asI|
+EMBED:   00E0: 6E766F6B 65722720 75694163 63657373  |nvoker' uiAccess|
+EMBED:   00F0: 3D276661 6C736527 2F3E0A20 20202020  |='false'/>.     |
+EMBED:   0100: 203C2F72 65717565 73746564 50726976  | </requestedPriv|
+EMBED:   0110: 696C6567 65733E0A 20202020 3C2F7365  |ileges>.    </se|
+EMBED:   0120: 63757269 74793E0A 20203C2F 74727573  |curity>.  </trus|
+EMBED:   0130: 74496E66 6F3E0A20 203C6465 70656E64  |tInfo>.  <depend|
+EMBED:   0140: 656E6379 3E0A2020 20203C64 6570656E  |ency>.    <depen|
+EMBED:   0150: 64656E74 41737365 6D626C79 3E0A2020  |dentAssembly>.  |
+EMBED:   0160: 20202020 3C617373 656D626C 79496465  |    <assemblyIde|
+EMBED:   0170: 6E746974 7920666F 6F3D2762 61722720  |ntity foo='bar' |
+EMBED:   0180: 2F3E0A20 2020203C 2F646570 656E6465  |/>.    </depende|
+EMBED:   0190: 6E744173 73656D62 6C793E0A 20203C2F  |ntAssembly>.  </|
+EMBED:   01A0: 64657065 6E64656E 63793E0A 20203C64  |dependency>.  <d|
+EMBED:   01B0: 6570656E 64656E63 793E0A20 2020203C  |ependency>.    <|
+EMBED:   01C0: 64657065 6E64656E 74417373 656D626C  |dependentAssembl|
+EMBED:   01D0: 793E0A20 20202020 203C6173 73656D62  |y>.      <assemb|
+EMBED:   01E0: 6C794964 656E7469 74792062 617A3D27  |lyIdentity baz='|
+EMBED:   01F0: 71757578 27202F3E 0A202020 203C2F64  |quux' />.    </d|
+EMBED:   0200: 6570656E 64656E74 41737365 6D626C79  |ependentAssembly|
+EMBED:   0210: 3E0A2020 3C2F6465 70656E64 656E6379  |>.  </dependency|
+EMBED:   0220: 3E0A3C2F 61737365 6D626C79 3E0A      |>.</assembly>.|
+EMBED: )