Integrate manifest merging library into LLD.

[ecbeckmann]

Summary

Now that the llvm-mt manifest merging libraries are complete, we may use them to merge manifests instead of needing to shell out to mt.exe.

Contingent on D36201.

[ecbeckmann]

Shared return.

[ecbeckmann]

diff to correct revision

[zturner]

lld/COFF/DriverUtils.cpp

336

Can you mark this function static?

386

Indentation is off here. Is this clang-formatted?

398

I wonder if all this should be an implementation detail of WindowsManifestMerger itself. Then this entire function would jsut become:

WindowsManifestMerger Merger;
for (StringRef Filename : Config->ManifestInput)
  Merger.mergeFromFile(Filename);
return Merger.getMergedOutput();

and the decision about whether to use libxml or shell out to mt.exe would be part of the WindowsManifestMerger class.

419

If we're not on Windows we should emit a warning that the manifest file was ignored since mt will not be available.

[ruiu]

lld/COFF/DriverUtils.cpp

337

Can you make it more lld-ish?

DefaultXml -> Ret

363–381

I believe raw_string_ostream is unbuffered, so you don't need this.

398

I agree with Zach. lld shouldn't know whether libxml is available or not. So, #if LLVM_LIBXML2_ENABLED in lld doesn't seem right.

[ecbeckmann]

some format and style changes, also do not check if libxml2 is installed but rather just check if WindowsManifestMerger is successful or not.

[ecbeckmann]

lld/COFF/DriverUtils.cpp

386

hmm seems it wasn't

[ecbeckmann]

Diff with the correction revision.

[ecbeckmann]

Correct format.

[ruiu]

lld/COFF/DriverUtils.cpp

383–384

Separate this function into two functions, createManifestXmlLibxml and createManifestXmlMt, and call them sequentially from createManifestXml.

405–407

Please follow the coding style in this file, so that the new code looks like they existed from beginning. Remove {}.

412

Define OutputBuffer here instead of at the binning of this function, as this is the first line you are using that variable.

[ecbeckmann]

Separate manifest merge functions, style fix.

[ecbeckmann]

lld/COFF/DriverUtils.cpp

412

now defined right before use

[ecbeckmann]

Diff with correct revision.

[ruiu]

lld/COFF/DriverUtils.cpp

363–382

It is more natural to return a value rather than assigning to a variable passed by reference.

428

I'm a little confused. Is this safe?

I mean, you are returning a pointer to a buffer owned by an object, which is in turn managed by a unique_ptr. But the unique_ptr is destructed as soon as you return from this function. What am I missing?

[ecbeckmann]

lld/COFF/DriverUtils.cpp

428

Function returns std::string, not a pointer. std::string copy constructor is implicitly called in the return.

[ecbeckmann]

Return mem buff pointer instead of pass by reference.

[zturner]

lld/test/lit.cfg

270

Can you line-break here? This goes past 80. Also, why are we checking for cvtres instead of mt? The comment makes sense, but why not just check for mt directly?

[ecbeckmann]

lld/test/lit.cfg

270

Checking for mt always seems to succeed, there are too many identically named tools I believe. Note that this was the standard check for finding Microsoft utils even before I started making changes, you could search for rc and cvtres but not for mt.

[ecbeckmann]

Instead of returning nullptr when libxml is not installed, it returns an error
which is more descriptive, and also doesn't require code outside the manifest
library to understand the cases for returning error/nullptr.

[ecbeckmann]

Rebase, diff with correction version.

[ecbeckmann]

Rebase and diff properly.

[ecbeckmann]

Pull out a lot of unnecessary changes.

[ecbeckmann]

lld/COFF/DriverUtils.cpp

419

I actually think we should continue to throw an error, because it is never valid to ignore the manifest when producing the executable.

[ecbeckmann]

Formatting change

[ecbeckmann]

Remove more unnecessary files.

[ruiu]

lld/COFF/DriverUtils.cpp

364

In general, we prefer StringRef over std::string & (or const std::string &). Can you replace?

385

Ditto

386

It is nice to explain why we don't want to execute mt command on non-Windows machines, because this is different from what we usually do (which is to try to execute a command and see if execution succeeded.)

389

std::string() -> ""

lld/test/lit.cfg

270

Yeah, mt is a command to operate magnetic tapes, and even though most Unix machines don't have tape drives anymore, it is still distributed and installed as part of GNU commands by default. So checking existence of cvtres makes sense.

[hans]

lld/trunk/COFF/DriverUtils.cpp

432 ↗

(On Diff #112108)

I'm not sure it makes sense to warn here. The user probably doesn't care which way the manifests were merged?

This breaks Chromium's build with LLD on Windows since libxml2 isn't availalbe and we treat warnings as errors. See https://bugs.chromium.org/p/chromium/issues/detail?id=760385

[hans]

lld/trunk/COFF/DriverUtils.cpp

432 ↗

(On Diff #112108)

I take it back, we don't treat it as an error and that's not why the build is broken.

But I'm still not sure the warning is actually helpful.

[orivej]

This broke building LLD with an installed LLVM: https://bugs.llvm.org/show_bug.cgi?id=34608

lld/COFF/CMakeLists.txt

add_lld_library(lldCOFF
   LTO
   LibDriver
   Object
   MC
   MCDisassembler
   Target
   Option
   Support
+  WindowsManifest
 
   LINK_LIBS
   lldCore
   ${LLVM_PTHREAD_LIB}
 
   DEPENDS
   COFFOptionsTableGen
   ${tablegen_deps}
   )

lld/COFF/DriverUtils.cpp

 
 #include "Config.h"
 #include "Driver.h"
 #include "Error.h"
 #include "Memory.h"
 #include "Symbols.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/WindowsResource.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/WindowsManifest/WindowsManifestMerger.h"
 #include <memory>
 
 using namespace llvm::COFF;
 using namespace llvm;
 using llvm::cl::ExpandResponseFiles;
 using llvm::cl::TokenizeWindowsCommandLine;
 using llvm::sys::Process;
 
return check(MemoryBuffer::getFile(Path, /*FileSize=*/-1,
                                        /*IsVolatileSize=*/true),
                  "could not open " + Path);
   }
 
   std::string Path;
 };
 }
 
-// Create the default manifest file as a temporary file.
-TemporaryFile createDefaultXml() {
-  // Create a temporary file.
-  TemporaryFile File("defaultxml", "manifest");
-
-  // Open the temporary file for writing.
-  std::error_code EC;
-  raw_fd_ostream OS(File.Path, EC, sys::fs::F_Text);
-  if (EC)
-    fatal(EC, "failed to open " + File.Path);
+static std::string createDefaultXml() {
+  std::string Ret;
+  raw_string_ostream OS(Ret);
 
   // Emit the XML. Note that we do *not* verify that the XML attributes are
   // syntactically correct. This is intentional for link.exe compatibility.
   OS << "<?xml version=\"1.0\" standalone=\"yes\"?>\n"
      << "<assembly xmlns=\"urn:schemas-microsoft-com:asm.v1\"\n"
      << "          manifestVersion=\"1.0\">\n";
   if (Config->ManifestUAC) {
     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()) {
     OS << "  <dependency>\n"
        << "    <dependentAssembly>\n"
        << "      <assemblyIdentity " << Config->ManifestDependency << " />\n"
        << "    </dependentAssembly>\n"
        << "  </dependency>\n";
   }
   OS << "</assembly>\n";
-  OS.close();
-  return File;
+  return Ret;
 }
 
-static std::string readFile(StringRef Path) {
-  std::unique_ptr<MemoryBuffer> MB =
-      check(MemoryBuffer::getFile(Path), "could not open " + Path);
-  return MB->getBuffer();
+static Expected<std::unique_ptr<MemoryBuffer>>
+createManifestXmlWithInternalMt(std::string &DefaultXml) {
+  std::unique_ptr<MemoryBuffer> DefaultXmlCopy =
+      MemoryBuffer::getMemBufferCopy(DefaultXml);
+
+  windows_manifest::WindowsManifestMerger Merger;
+  if (auto E = Merger.merge(*DefaultXmlCopy.get()))
+    return std::move(E);
+
+  for (StringRef Filename : Config->ManifestInput) {
+    std::unique_ptr<MemoryBuffer> Manifest =
+        check(MemoryBuffer::getFile(Filename));
+    if (auto E = Merger.merge(*Manifest.get())) {
+      warn("internal manifest tool failed on file " + Filename);
+      return std::move(E);
+    }
   }
 
-static std::string createManifestXml() {
-  // Create the default manifest file.
-  TemporaryFile File1 = createDefaultXml();
-  if (Config->ManifestInput.empty())
-    return readFile(File1.Path);
+  return Merger.getMergedManifest();
+}
 
-  // If manifest files are supplied by the user using /MANIFESTINPUT
-  // option, we need to merge them with the default manifest.
-  TemporaryFile File2("user", "manifest");
+static std::unique_ptr<MemoryBuffer>
+createManifestXmlWithExternalMt(std::string &DefaultXml) {
+  const Triple HostTriple(Triple::normalize(LLVM_HOST_TRIPLE));
+  if (!HostTriple.isOSWindows())
+    fatal("manifest ignored because no external manifest tool available");
+  // Create the default manifest file as a temporary file.
+  TemporaryFile Default("defaultxml", "manifest");
+  std::error_code EC;
+  raw_fd_ostream OS(Default.Path, EC, sys::fs::F_Text);
+  if (EC)
+    fatal(EC, "failed to open " + Default.Path);
+  OS << DefaultXml;
+  OS.close();
+
+  // Merge user-supplied manifests if they are given.  Since libxml2 is not
+  // enabled, we must shell out to Microsoft's mt.exe tool.
+  TemporaryFile User("user", "manifest");
 
   Executor E("mt.exe");
   E.add("/manifest");
-  E.add(File1.Path);
+  E.add(Default.Path);
   for (StringRef Filename : Config->ManifestInput) {
     E.add("/manifest");
     E.add(Filename);
   }
   E.add("/nologo");
-  E.add("/out:" + StringRef(File2.Path));
+  E.add("/out:" + StringRef(User.Path));
   E.run();
-  return readFile(File2.Path);
+
+  return check(MemoryBuffer::getFile(User.Path), "could not open " + User.Path);
+}
+
+static std::string createManifestXml() {
+  std::string DefaultXml = createDefaultXml();
+  if (Config->ManifestInput.empty())
+    return DefaultXml;
+
+  // If manifest files are supplied by the user using /MANIFESTINPUT
+  // option, we need to merge them with the default manifest. If libxml2
+  // is enabled, we may merge them with LLVM's own library.
+  Expected<std::unique_ptr<MemoryBuffer>> OutputBufferOrError =
+      createManifestXmlWithInternalMt(DefaultXml);
+  if (OutputBufferOrError)
+    return OutputBufferOrError.get()->getBuffer();
+  // Using built-in library failed, possibly because libxml2 is not installed.
+  // Shell out to mt.exe instead.
+  handleAllErrors(std::move(OutputBufferOrError.takeError()),
+                  [&](ErrorInfoBase &EIB) {
+                    warn("error with internal manifest tool: " + EIB.message());
+                  });
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
+  OutputBuffer = createManifestXmlWithExternalMt(DefaultXml);
+  return OutputBuffer->getBuffer();
 }
 
 static std::unique_ptr<MemoryBuffer>
 createMemoryBufferForManifestRes(size_t ManifestSize) {
   size_t ResSize = alignTo(
       object::WIN_RES_MAGIC_SIZE + object::WIN_RES_NULL_ENTRY_SIZE +
           sizeof(object::WinResHeaderPrefix) + sizeof(object::WinResIDs) +
           sizeof(object::WinResHeaderSuffix) + ManifestSize,

lld/test/COFF/manifestinput.test

-# REQUIRES: win_mt
+# REQUIRES: manifest_tool
 
 # RUN: yaml2obj %p/Inputs/ret42.yaml > %t.obj
 # RUN: lld-link /out:%t.exe /entry:main \
 # RUN:   /manifest:embed \
 # RUN:   /manifestuac:"level='requireAdministrator'" \
 # RUN:   /manifestinput:%p/Inputs/manifestinput.test %t.obj
 # RUN: llvm-readobj -coff-resources -file-headers %t.exe | FileCheck %s \
 # RUN:   -check-prefix TEST_EMBED

lld/test/lit.cfg

 if re.search(r'X86', archs):
     config.available_features.add('x86')
 llvm_config_cmd.wait()
 
 # Set a fake constant version so that we get consitent output.
 config.environment['LLD_VERSION'] = 'LLD 1.0'
 
 # Indirectly check if the mt.exe Microsoft utility exists by searching for
-# cvtres, which always accompanies it.
-if lit.util.which('cvtres', config.environment['PATH']):
-    config.available_features.add('win_mt')
+# cvtres, which always accompanies it.  Alternatively, check if we can use
+# libxml2 to merge manifests.
+if (lit.util.which('cvtres', config.environment['PATH'])) or \
+ (config.llvm_libxml2_enabled == "1"):
+    config.available_features.add('manifest_tool')

lld/test/lit.site.cfg.in

 @LIT_SITE_CFG_IN_HEADER@
 
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
 config.llvm_libs_dir = "@LLVM_LIBS_DIR@"
+config.llvm_libxml2_enabled = "@LLVM_LIBXML2_ENABLED@"
 config.lit_tools_dir = "@LLVM_LIT_TOOLS_DIR@"
 config.lld_obj_root = "@LLD_BINARY_DIR@"
 config.lld_libs_dir = "@LLVM_LIBRARY_OUTPUT_INTDIR@"
 config.lld_tools_dir = "@LLVM_RUNTIME_OUTPUT_INTDIR@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.have_zlib = @HAVE_LIBZ@
 

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

 
 #else
 
 WindowsManifestMerger::WindowsManifestMergerImpl::~WindowsManifestMergerImpl() {
 }
 
 Error WindowsManifestMerger::WindowsManifestMergerImpl::merge(
     const MemoryBuffer &Manifest) {
-  return Error::success();
+  return make_error<WindowsManifestError>("no libxml2");
 }
 
 std::unique_ptr<MemoryBuffer>
 WindowsManifestMerger::WindowsManifestMergerImpl::getMergedManifest() {
   return nullptr;
 }
 
 #endif

llvm/test/tools/llvm-mt/Inputs/test_manifest.manifest

-<?xml version="1.0"?>
+<?xml version="1.0" encoding="UTF-8"?>
 <assembly xmlns="urn:schemas-microsoft-com:asm.v1">
   <trustInfo>
     <security>
       <requestedPrivileges>
         <requestedExecutionLevel level="3" uiAccess="1"/>
       </requestedPrivileges>
     </security>
   </trustInfo>

llvm/test/tools/llvm-mt/big_merge.test

 REQUIRES: libxml2
 UNSUPPORTED: windows
 
-RUN: llvm-mt /manifest %p/Inputs/trust_info.manifest \
-RUN:   /manifest %p/Inputs/assembly_identity.manifest \
-RUN:   /manifest %p/Inputs/trust_and_identity.manifest \
-RUN:   /manifest %p/Inputs/compatibility.manifest \
-RUN:   /manifest  %p/Inputs/windows_settings.manifest /out:%t
+RUN: llvm-mt /manifest %p/Inputs/trust_info.manifest /manifest \
+RUN:   %p/Inputs/assembly_identity.manifest /manifest \
+RUN:   %p/Inputs/trust_and_identity.manifest /manifest \
+RUN:   %p/Inputs/compatibility.manifest /manifest \
+RUN:   %p/Inputs/windows_settings.manifest /out:%t
 RUN: FileCheck %s -input-file=%t
 
 CHECK:      <?xml version="1.0" encoding="UTF-8"?>
 CHECK-NEXT: <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
 CHECK-NEXT: <ms_asmv1:trustInfo xmlns="urn:schemas-microsoft-com:asm.v3" xmlns:ms_asmv1="urn:schemas-microsoft-com:asm.v1">
 CHECK-NEXT:   <ms_asmv1:security>
 CHECK-NEXT:     <ms_asmv1:requestedPrivileges>
 CHECK-NEXT:       <ms_asmv1:requestedExecutionLevel level="trust1" ms_asmv1:access="false"/>

llvm/tools/llvm-mt/Opts.td

 include "llvm/Option/OptParser.td"
 
+def supported : OptionGroup<"supported">;
 def unsupported : OptionGroup<"unsupported">;
-def manifest : Separate<["/", "-"], "manifest">, HelpText<"Used to specify each manifest that need to be processed">, MetaVarName<"manifest">;
+def manifest : Separate<["/", "-"], "manifest">, HelpText<"Used to specify each manifest that need to be processed">, MetaVarName<"manifest">, Group<supported>;
 def identity : Joined<["/", "-"], "identity:">, HelpText<"Not supported">, MetaVarName<"identity">, Group<unsupported>;
 def rgs : Joined<["/", "-"], "rgs:">, HelpText<"Not supported">, MetaVarName<"script">, Group<unsupported>;
 def tlb : Joined<["/", "-"], "tlb:">, HelpText<"Not supported">, MetaVarName<"file">, Group<unsupported>;
 def dll : Joined<["/", "-"], "dll:">, HelpText<"Not supported">, MetaVarName<"dll">, Group<unsupported>;
 def replacements : Joined<["/", "-"], "replacements:">, HelpText<"Not supported">, MetaVarName<"file">, Group<unsupported>;
 def managed_assembly_name : Joined<["/", "-"], "managedassemblyname:">, HelpText<"Not supported">, MetaVarName<"assembly">, Group<unsupported>;
 def no_dependency : Flag<["/", "-"], "nodependency">, HelpText<"Not supported">, Group<unsupported>;
 def category : Flag<["/", "-"], "category">, HelpText<"Not supported">, Group<unsupported>;
-def no_logo : Flag<["/", "-"], "nologo">, HelpText<"No effect as this tool never writes copyright data.  Included for parity">;
-def out : Joined<["/", "-"], "out:">, HelpText<"Name of the output manifest.  If this is skipped and only one manifest is being operated upon by the tool, that manifest is modified in place">, MetaVarName<"manifest">;
+def no_logo : Flag<["/", "-"], "nologo">, HelpText<"No effect as this tool never writes copyright data.  Included for parity">, Group<supported>;
+def out : Joined<["/", "-"], "out:">, HelpText<"Name of the output manifest.  If this is skipped and only one manifest is being operated upon by the tool, that manifest is modified in place">, MetaVarName<"manifest">, Group<supported>;
 def input_resource : Joined<["/", "-"], "inputresource:">, HelpText<"Not supported">, MetaVarName<"file">, Group<unsupported>;
 def output_resource : Joined<["/", "-"], "outputresource:">, HelpText<"Not supported">, MetaVarName<"file">, Group<unsupported>;
 def output_resource_flag : Flag<["/", "-"], "outputresource">, Alias<output_resource>, HelpText<"Not supported">, Group<unsupported>;
 def update_resource : Joined<["/", "-"], "updateresource:">, HelpText<"Not supported">, MetaVarName<"file">, Group<unsupported>;
 def hash_update : Joined<["/", "-"], "hashupdate:">, HelpText<"Not supported">, MetaVarName<"file">, Group<unsupported>;
 def hash_update_flag : Flag<["/", "-"], "hashupdate">, Alias<hash_update>, HelpText<"Not supported">, Group<unsupported>;
 def validate_manifest : Flag<["/", "-"], "validate_manifest">, HelpText<"Not supported">, Group<unsupported>;
 def validate_file_hashes : Joined<["/", "-"], "validate_file_hashes:">, HelpText<"Not supported">, MetaVarName<"">, Group<unsupported>;
 def canonicalize : Flag<["/", "-"], "canonicalize:">, HelpText<"Not supported">, Group<unsupported>;
 def check_for_duplicates : Flag<["/", "-"], "check_for_duplicates:">, HelpText<"Not supported">, Group<unsupported>;
 def make_cdfs : Flag<["/", "-"], "makecdfs:">, HelpText<"Not supported">, Group<unsupported>;
 def verbose : Flag<["/", "-"], "verbose">, HelpText<"Not supported">, Group<unsupported>;
-def help : Flag<["/", "-"], "?">;
-def help_long : Flag<["/", "-"], "help">, Alias<help>;
-def h : Flag<["/", "-"], "h">, Alias<help>;
+def help : Flag<["/", "-"], "?">, Group<supported>;
+def help_long : Flag<["/", "-"], "help">, Alias<help>, Group<supported>;
+def h : Flag<["/", "-"], "h">, Alias<help>, Group<supported>;

llvm/tools/llvm-mt/llvm-mt.cpp

int main(int argc, const char **argv) {
   unsigned MAI, MAC;
   ArrayRef<const char *> ArgsArr = makeArrayRef(argv + 1, argc);
   opt::InputArgList InputArgs = T.ParseArgs(ArgsArr, MAI, MAC);
 
   for (auto *Arg : InputArgs.filtered(OPT_INPUT))
     reportError(Twine("invalid option ") + Arg->getSpelling());
 
   for (auto &Arg : InputArgs) {
+    if (!(Arg->getOption().matches(OPT_unsupported) ||
+          Arg->getOption().matches(OPT_supported))) {
+      reportError(Twine("invalid option ") + Arg->getSpelling());
+    }
+  }
+
+  for (auto &Arg : InputArgs) {
     if (Arg->getOption().matches(OPT_unsupported)) {
       outs() << "llvm-mt: ignoring unsupported '" << Arg->getOption().getName()
              << "' option\n";
     }
   }
 
   if (InputArgs.hasArg(OPT_help)) {
     T.PrintHelp(outs(), "mt", "Manifest Tool", false);