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

320

Can you mark this function static?

352

Indentation is off here. Is this clang-formatted?

354

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.

376

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

321

Can you make it more lld-ish?

DefaultXml -> Ret

344

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

354

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

352

hmm seems it wasn't

[ecbeckmann]

Diff with the correction revision.

[ecbeckmann]

Correct format.

[ruiu]

lld/COFF/DriverUtils.cpp

347–348

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

362–364

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

406

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

406

now defined right before use

[ecbeckmann]

Diff with correct revision.

[ruiu]

lld/COFF/DriverUtils.cpp

344–345

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

422

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

422

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

376

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

345

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

351

Ditto

352

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.)

355

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;
-}
-
-static std::string readFile(StringRef Path) {
-  std::unique_ptr<MemoryBuffer> MB =
-      check(MemoryBuffer::getFile(Path), "could not open " + Path);
-  return MB->getBuffer();
+  return Ret;
 }
 
 static std::string createManifestXml() {
-  // Create the default manifest file.
-  TemporaryFile File1 = createDefaultXml();
+  std::string DefaultXml = createDefaultXml();
   if (Config->ManifestInput.empty())
-    return readFile(File1.Path);
+    return DefaultXml;
+
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
 
   // If manifest files are supplied by the user using /MANIFESTINPUT
-  // option, we need to merge them with the default manifest.
-  TemporaryFile File2("user", "manifest");
+  // option, we need to merge them with the default manifest. If libxml2
+  // is enabled, we may merge them with LLVM's own library.
+
+  std::unique_ptr<MemoryBuffer> DefaultXmlCopy =
+      MemoryBuffer::getMemBufferCopy(DefaultXml);
+
+  windows_manifest::WindowsManifestMerger Merger;
+  if (auto E = Merger.merge(*DefaultXmlCopy.get())) {
+    fatal(E, "failed to merge default manifest");
+  }
+
+  for (StringRef Filename : Config->ManifestInput) {
+    std::unique_ptr<MemoryBuffer> Manifest =
+        check(MemoryBuffer::getFile(Filename));
+    if (auto E = Merger.merge(*Manifest.get()))
+      fatal(E, "failed to merge " + Filename);
+  }
+
+  OutputBuffer = Merger.getMergedManifest();
+
+  // Using built-in library failed, possibly because libxml2 is not installed.
+  // Shell out to mt.exe instead.
+  if (!OutputBuffer) {
+    const Triple HostTriple(Triple::normalize(LLVM_HOST_TRIPLE));
+    if (!HostTriple.isOSWindows()) {
+      warn("manifest ignored because no manifest merging tool available");
+      return std::string();
+    }
+  // 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);
+
+  OutputBuffer =
+      check(MemoryBuffer::getFile(User.Path), "could not open " + User.Path);
+    }
+  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,
       object::WIN_RES_DATA_ALIGNMENT);
   return MemoryBuffer::getNewMemBuffer(ResSize);
 }
 
 static void writeResFileHeader(char *&Buf) {
   memcpy(Buf, COFF::WinResMagic, sizeof(COFF::WinResMagic));
   Buf += sizeof(COFF::WinResMagic);
   memset(Buf, 0, object::WIN_RES_NULL_ENTRY_SIZE);
   Buf += object::WIN_RES_NULL_ENTRY_SIZE;
 }
 
 static void writeResEntryHeader(char *&Buf, size_t 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@