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

316 ↗

(On Diff #110091)

Can you mark this function static?

352 ↗

(On Diff #110091)

Indentation is off here. Is this clang-formatted?

354 ↗

(On Diff #110091)

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.

377 ↗

(On Diff #110091)

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

317 ↗

(On Diff #110091)

Can you make it more lld-ish?

DefaultXml -> Ret

343 ↗

(On Diff #110091)

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

354 ↗

(On Diff #110091)

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 ↗

(On Diff #110091)

hmm seems it wasn't

[ecbeckmann]

Diff with the correction revision.

[ecbeckmann]

Correct format.

[ruiu]

lld/COFF/DriverUtils.cpp

347 ↗

(On Diff #111281)

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

362–364 ↗

(On Diff #111281)

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

373 ↗

(On Diff #111281)

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

373 ↗

(On Diff #111281)

now defined right before use

[ecbeckmann]

Diff with correct revision.

[ruiu]

lld/COFF/DriverUtils.cpp

347–348 ↗

(On Diff #111447)

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

414 ↗

(On Diff #111447)

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

414 ↗

(On Diff #111447)

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 ↗

(On Diff #111452)

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 ↗

(On Diff #111452)

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

377 ↗

(On Diff #110091)

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

348 ↗

(On Diff #111452)

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

367 ↗

(On Diff #111452)

Ditto

368 ↗

(On Diff #111452)

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

371 ↗

(On Diff #111452)

std::string() -> ""

lld/test/lit.cfg

270 ↗

(On Diff #111452)

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

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

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/trunk/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/trunk/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/trunk/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/trunk/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/trunk/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/trunk/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