Implement parsing and writing of a single xml manifest file.

[ecbeckmann]

Summary

Implement parsing and writing of a single xml manifest file.

[ecbeckmann]

llvm/lib/Support/WindowsManifestMerger.cpp

46

remove

llvm/test/CMakeLists.txt

141

remove

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

147

remove

153

remove

156

remove

[ecbeckmann]

Removed logging.

[ecbeckmann]

Tests will not be run without libxml2.

[ecbeckmann]

Also ensure that the libary actually exists...

[ecbeckmann]

a couple test fixes.

[ecbeckmann]

Added newline

[ruiu]

llvm/cmake/config-ix.cmake

604

Does it make sense to not do this on Windows?

llvm/include/llvm/Support/WindowsManifestMerger.h

33

You can use #ifdef

52

const Twine &Msg?

llvm/lib/Support/WindowsManifestMerger.cpp

29

Use #ifdef throughout this patch.

42

This function returns something non-empty even if you dont' have libxml2, but I don't think that makes sense. Just like above function which just returns Error::success(), I think it is better to return {} from this function if libxml is not available.

42

Looks like it is more straightforward to return an std::vector<uint8_t> from this function instead of a MemoryBuffer. MemoryBuffer is usually used to represent something you've read from files. In this case, this function just construct an in-memory data structure which can easily be represented as std::vector.

llvm/test/CMakeLists.txt

61

Is this intentional?

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

68

Remove a full stop.

77–79

nit: since the real function body is just one line, this is simpler

if (EC)
  reportError

83–87

Ditto

110

Remove a full stop and \n.

Error messages should start with lowercase letter.

122

Ditto

129

Ditto

132

Ditto

148

Ditto

[ecbeckmann]

Some syntax and return value changes.

[ruiu]

Is this ready for another round of review?

[ecbeckmann]

Rebase

llvm/cmake/config-ix.cmake

604

Yes, apparently the version of libxml2 on Windows uses different headers than what we expect and linking it in would cause a problem. Besides, if we are on windows we can just continue to shell out to the original mt.exe tool.

llvm/lib/Support/WindowsManifestMerger.cpp

42

In this case I feel it is more convenient to use MemoryBuffer because it directly takes ownership of the underlying data allocated by xmlDocDumpMemory(). For using a vector we would need an additional copy, and then explicitly delete the allocated memory. Though perhaps this is okay for clarity?

llvm/test/CMakeLists.txt

61

Yes down below I only add the test depends if libxml2 is found.

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

68

When I rebase the previous patch these mistakes will go away.

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

29

This include guard is not correct. #endif should be at end of the file.

61–62

Do you need this? Since you didn't define any other ctors, this is defined as a default ctor. Isn't it enough?

llvm/lib/Support/WindowsManifestMerger.cpp

43–46

I think you'll get a compiler warning if you compile it in an environment that doesn't have libxml, as BufferSize and XmlBuff will be unused.

A better way of doing it is to completely separate two conditions:

std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
#ifdef LIBXML2_ENABLED
  ...
  return OutputBuffer;
#else
  return nullptr;
#endif
}

69

Remove else after return.

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

78

Indentation.

83

Indentation.

[ecbeckmann]

update

[ecbeckmann]

More elegant linking of the xml2 library in the build system.

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

75

Fix this. (It doesn't say that you need a blank line at end of a file, but it says that your last line doesn't end with '\n'.)

llvm/lib/Support/WindowsManifestMerger.cpp

52–55

You can eliminate OutputBuffer by doing this:

if (BufferSize == 0)
  return nullptr;  
return MemoryBuffer::getMemBuffer(
      StringRef(reinterpret_cast<const char *>(XmlBuff), (size_t)BufferSize));

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

148

Isn't this an error? Looks like this program exits with 0 in this case.

[ecbeckmann]

some reformats

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

44

This is dead now.

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

148

How about this?

[ecbeckmann]

Remove vestigal variable.

[ecbeckmann]

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

148

check most recent patch.

[ruiu]

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

149

Since reportError is a noreturn function, remove else.

[ecbeckmann]

Remove else block after error.

[ruiu]

LGTM

Please make sure that this patch works with/without libxml2 before committing.

[ecbeckmann]

Final changes to the build system.

[orivej]

WindowsManifestMerger.h should not include llvm/Config/config.h: https://bugs.llvm.org/show_bug.cgi?id=34608

llvm/cmake/config-ix.cmake

else()
       else()
         message(STATUS "OCaml bindings disabled, need ctypes >=0.4.")
       endif()
     endif()
   endif()
 endif()
 
 string(REPLACE " " ";" LLVM_BINDINGS_LIST "${LLVM_BINDINGS}")
+
+message ( STATUS "find libxml" )
+
+if (NOT LIBXML2_FOUND AND NOT (CMAKE_SYSTEM_NAME MATCHES "Windows"))
+  find_package(LibXml2)
+  message( STATUS "Ran find_package lixml2" )
+endif()
+
+if (LIBXML2_FOUND AND NOT LLVM_DISABLE_LIBXML2)
+  message( STATUS "found libxml2")
+  add_definitions( -DLIBXML2_ENABLED )
+  list(APPEND CMAKE_REQUIRED_LIBRARIES ${LIBXML2_LIBRARIES})
+  include_directories(${LIBXML2_INCLUDE_DIR})
+endif()

llvm/include/llvm/Support/WindowsManifestMerger.h

+//===-- WindowsManifestMerger.h ---------------------------------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+//
+// This file provides a utility for merging Microsoft .manifest files.  These
+// files are xml documents which contain meta-information about applications,
+// such as whether or not admin access is required, system compatibility,
+// versions, etc.  Part of the linking process of an executable may require
+// merging several of these .manifest files using a tree-merge following
+// specific rules.  Unfortunately, these rules are not documented well
+// anywhere.  However, a careful investigation of the behavior of the original
+// Microsoft Manifest Tool (mt.exe) revealed the rules of this merge.  As the
+// saying goes, code is the best documentation, so please look below if you are
+// interested in the exact merging requirements.
+//
+// Ref: https://msdn.microsoft.com/en-us/library/windows/desktop/aa374191(v=vs.85).aspx
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_INCLUDE_LLVM_SUPPORT_WINDOWS_MANIFEST_MERGER_H
+#define LLVM_INCLUDE_LLVM_SUPPORT_WINDOWS_MANIFEST_MERGER_H
+#endif
+
+#include "llvm/Support/Error.h"
+
+#if defined(LIBXML2_ENABLED)
+#include <libxml/xmlreader.h>
+#endif
+
+namespace llvm {
+
+class MemoryBuffer;
+
+#if defined(LIBXML2_ENABLED)
+typedef xmlDocPtr XMLDocumentImpl;
+typedef xmlNodePtr XMLNodeImpl;
+#else
+typedef void *XMLDocumentImpl;
+typedef void *XMLNodeImpl;
+#endif
+
+class WindowsManifestError : public ErrorInfo<WindowsManifestError, ECError> {
+public:
+  static char ID;
+  WindowsManifestError(Twine Msg);
+  void log(raw_ostream &OS) const override;
+private:
+  std::string Msg;
+};
+
+
+class WindowsManifestMerger {
+public:
+  WindowsManifestMerger() = default;
+
+  Error merge(MemoryBuffer& Manifest);
+
+  std::unique_ptr<MemoryBuffer> getMergedManifest();
+
+private:
+  static void errorCallback(void *Ctx, const char *Format, ...);
+  Error getParseError();
+
+  XMLNodeImpl CombinedRoot = nullptr;
+  SmallString<256> ParseErrorMsg;
+  bool ParseErrorOccurred = false;
+};
+
+}

llvm/lib/Support/CMakeLists.txt

 set(system_libs)
+
+message(STATUS "hello")
+
 if( MSVC OR MINGW )
   # libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
   set(system_libs ${system_libs} psapi shell32 ole32 uuid)
 elseif( CMAKE_HOST_UNIX )
   if( HAVE_LIBRT )
     set(system_libs ${system_libs} rt)
   endif()
   if( HAVE_LIBDL )
elseif( CMAKE_HOST_UNIX )
   if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
     set(system_libs ${system_libs} z)
   endif()
   if( UNIX AND NOT (BEOS OR HAIKU) )
     set(system_libs ${system_libs} m)
   endif()
 endif( MSVC OR MINGW )
 
+message (STATUS "checking libxml2 enabled in support")
+message( STATUS "adding libxml2 include")
+
+include_directories(${LIBXML2_INCLUDE_DIR})
+set(system_libs ${system_libs} ${LIBXML2_LIBRARIES})
+message (STATUS ${LIBXML2_INCLUDE_DIR})
+
 add_llvm_library(LLVMSupport
   AMDGPUCodeObjectMetadata.cpp
   APFloat.cpp
   APInt.cpp
   APSInt.cpp
   ARMBuildAttrs.cpp
   ARMAttributeParser.cpp
   ARMWinEH.cpp
add_llvm_library(LLVMSupport
   TargetParser.cpp
   ThreadPool.cpp
   Timer.cpp
   ToolOutputFile.cpp
   TrigramIndex.cpp
   Triple.cpp
   Twine.cpp
   Unicode.cpp
+  WindowsManifestMerger.cpp
   YAMLParser.cpp
   YAMLTraits.cpp
   raw_os_ostream.cpp
   raw_ostream.cpp
   regcomp.c
   regerror.c
   regexec.c
   regfree.c

llvm/lib/Support/WindowsManifestMerger.cpp

+//===-- WindowsManifestMerger.cpp ------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+//
+// This file implements the .manifest merger class.
+//
+//===---------------------------------------------------------------------===//
+
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/WindowsManifestMerger.h"
+
+#include <stdarg.h>
+
+namespace llvm {
+
+char WindowsManifestError::ID = 0;
+
+WindowsManifestError::WindowsManifestError(Twine Msg) : Msg(Msg.str()) {}
+
+void WindowsManifestError::log(raw_ostream &OS) const {
+  OS << Msg;
+}
+
+Error WindowsManifestMerger::merge(MemoryBuffer& Manifest) {
+  #if defined(LIBXML2_ENABLED)
+  xmlSetGenericErrorFunc((void *)this, WindowsManifestMerger::errorCallback);
+  XMLDocumentImpl ManifestXML = xmlReadMemory(Manifest.getBufferStart(), Manifest.getBufferSize(), "manifest.xml", nullptr, 0);
+  xmlSetGenericErrorFunc(nullptr, nullptr);
+  if (auto E = getParseError())
+    return E;
+  CombinedRoot = xmlDocGetRootElement(ManifestXML);
+  #endif
+  return Error::success();
+}
+
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
+  unsigned char *XmlBuff;
+  int BufferSize = 0;
+  #if defined(LIBXML2_ENABLED)
+  outs() << "LIBXML2_ENABLED\n";
+  if (CombinedRoot)
+  {
+    std::unique_ptr<xmlDoc> OutputDoc(xmlNewDoc((const unsigned char *) "1.0"));
+    xmlDocSetRootElement(OutputDoc.get(), CombinedRoot);
+    xmlDocDumpMemory(OutputDoc.get(), &XmlBuff, &BufferSize);
+  }
+  #endif
+
+  if (BufferSize > 0)
+    OutputBuffer = MemoryBuffer::getMemBuffer(StringRef(reinterpret_cast<const char*>(XmlBuff), (size_t) BufferSize));
+  else
+    OutputBuffer = MemoryBuffer::getNewMemBuffer(0);
+
+  return OutputBuffer;
+}
+
+void WindowsManifestMerger::errorCallback(void *Ctx, const char *Format, ...) {
+  WindowsManifestMerger *Merger = (WindowsManifestMerger *)Ctx;
+  Merger->ParseErrorOccurred = true;
+  va_list Args;
+  va_start(Args, Format);
+  vsnprintf(Merger->ParseErrorMsg.data(), Merger->ParseErrorMsg.size(), Format, Args);
+  va_end(Args);
+}
+
+Error WindowsManifestMerger::getParseError() {
+  if (!ParseErrorOccurred)
+    return Error::success();
+  else
+    return make_error<WindowsManifestError>(ParseErrorMsg);
+}
+
+}

llvm/test/CMakeLists.txt

set(LLVM_TEST_DEPENDS
           llvm-dwp
           llvm-extract
           llvm-lib
           llvm-link
           llvm-lto2
           llvm-mc
           llvm-mcmarkup
           llvm-modextract
-          llvm-mt
           llvm-nm
           llvm-objdump
           llvm-opt-report
           llvm-pdbutil
           llvm-profdata
           llvm-ranlib
           llvm-readobj
           llvm-rtdyld
set(LLVM_TEST_DEPENDS ${LLVM_TEST_DEPENDS}
           ocaml_llvm_ipo
           ocaml_llvm_passmgr_builder
           ocaml_llvm_scalar_opts
           ocaml_llvm_transform_utils
           ocaml_llvm_vectorize
         )
 endif()
 
+if (LIBXML2_FOUND AND NOT LLVM_DISABLE_LIBXML2)
+  message (STATUS "enabling llvm-mt tests")
+  set(LLVM_TEST_DEPENDS ${LLVM_TEST_DEPENDS}
+          llvm-mt
+        )
+endif()
+
 add_custom_target(llvm-test-depends DEPENDS ${LLVM_TEST_DEPENDS})
 set_target_properties(llvm-test-depends PROPERTIES FOLDER "Tests")
 
 add_lit_testsuite(check-llvm "Running the LLVM regression tests"
   ${CMAKE_CURRENT_BINARY_DIR}
   PARAMS llvm_site_config=${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
          llvm_unit_site_config=${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg
   DEPENDS ${LLVM_TEST_DEPENDS}

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

+<?xml version="1.0"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+  <trustInfo>
+    <security>
+      <requestedPrivileges>
+         <requestedExecutionLevel level="3" uiAccess="1"/>
+      </requestedPrivileges>
+    </security>
+  </trustInfo>
+  <dependency>
+    <dependentAssembly>
+      <assemblyIdentity program="displayDriver"/>
+    </dependentAssembly>
+  </dependency>
+</assembly>

llvm/test/tools/llvm-mt/help.test

 RUN: llvm-mt /? | FileCheck %s -check-prefix=HELP
 
-RUN: llvm-mt /inputresource:foo.res /manifest foo.manifest | FileCheck %s -check-prefix=NOT_SUPPORTED
+RUN: llvm-mt /inputresource:foo.res /manifest \
+RUN:   %p/Inputs/test_manifest.manifest | FileCheck %s -check-prefix=NOT_SUPPORTED
 
 HELP:      OVERVIEW: Manifest Tool
 
 NOT_SUPPORTED: Ignoring INPUTRESOURCE: option; not supported.

llvm/test/tools/llvm-mt/single_file.test

+RUN: llvm-mt /manifest %p/Inputs/test_manifest.manifest /out:%t
+RUN: FileCheck --input-file %t %s
+
+CHECK:      <?xml version="1.0"?>
+CHECK-NEXT: <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+CHECK-NEXT: <trustInfo>
+CHECK-NEXT: <security>
+CHECK-NEXT: <requestedPrivileges>
+CHECK-NEXT: <requestedExecutionLevel level="3" uiAccess="1"/>
+CHECK-NEXT: </requestedPrivileges>
+CHECK-NEXT: </security>
+CHECK-NEXT: </trustInfo>
+CHECK-NEXT: <dependency>
+CHECK-NEXT: <dependentAssembly>
+CHECK-NEXT: <assemblyIdentity program="displayDriver"/>
+CHECK-NEXT: </dependentAssembly>
+CHECK-NEXT: </dependency>
+CHECK-NEXT: </assembly>

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

 // of Microsoft's mt.exe.
 //
 //===---------------------------------------------------------------------===//
 
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/WindowsManifestMerger.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include <system_error>
 
 using namespace llvm;
 
 namespace {
 
 enum ID {
-  OPT_INVALID = 0, // This is not an option ID.
+  OPT_INVALID = 0, // This is not an option ID.
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   OPT_##ID,
 #include "Opts.inc"
 #undef OPTION
 };
 
 #define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;

 };
 
 class CvtResOptTable : public opt::OptTable {
 public:
   CvtResOptTable() : OptTable(InfoTable, true) {}
 };
 
 static ExitOnError ExitOnErr;
-}
+} // namespace
 
 LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
-  errs() << "llvm-mt error: " << Msg;
+  errs() << "llvm-mt error: " << Msg << ".\n";
   exit(1);
 }
 
+static void reportError(StringRef Input, std::error_code EC) {
+  reportError(Twine(Input) + ": " + EC.message());
+}
+
+void error(std::error_code EC) {
+  if (!EC)
+    return;
+  reportError(EC.message());
+}
+
+void error(Error EC) {
+  if (!EC)
+    return;
+  handleAllErrors(std::move(EC),
+                  [&](const ErrorInfoBase &EI) { reportError(EI.message()); });
+}
 
 int main(int argc_, const char *argv_[]) {
   sys::PrintStackTraceOnErrorSignal(argv_[0]);
   PrettyStackTraceProgram X(argc_, argv_);
 
   ExitOnErr.setBanner("llvm-mt: ");
 
   SmallVector<const char *, 256> argv;
   SpecificBumpPtrAllocator<char> ArgAllocator;
   ExitOnErr(errorCodeToError(sys::Process::GetArgumentVector(
       argv, makeArrayRef(argv_, argc_), ArgAllocator)));
 
   llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.
 
   CvtResOptTable T;
   unsigned MAI, MAC;
   ArrayRef<const char *> ArgsArr = makeArrayRef(argv_ + 1, argc_);
   opt::InputArgList InputArgs = T.ParseArgs(ArgsArr, MAI, MAC);
 
   for (auto &Arg : InputArgs) {
-    if (Arg->getOption().matches(OPT_UNSUPPORTED)) {
-      outs() << "Ignoring " << Arg->getOption().getName() << " option; not supported.\n";
-    }
+    if (Arg->getOption().matches(OPT_UNSUPPORTED)) {
+      outs() << "Ignoring " << Arg->getOption().getName()
+             << " option; not supported.\n";
+    }
   }
 
   if (InputArgs.hasArg(OPT_HELP)) {
     T.PrintHelp(outs(), "mt", "Manifest Tool", false);
     return 0;
   }
 
   std::vector<std::string> InputFiles = InputArgs.getAllArgValues(OPT_MANIFEST);
 
   if (InputFiles.size() == 0) {
-    reportError("No input file specified.\n");
+    reportError("No input file specified");
   }
 
   StringRef OutputFile;
 
   if (InputArgs.hasArg(OPT_OUT)) {
     OutputFile = InputArgs.getLastArgValue(OPT_OUT);
   } else if (InputFiles.size() == 1) {
-    OutputFile = InputFiles[0];
+    OutputFile = InputFiles[0];
   } else {
-    reportError("No output file specified.\n");
+    reportError("No output file specified");
   }
 
-  return 0;
+  WindowsManifestMerger Merger;
+
+  for (const auto &File : InputFiles) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> ManifestOrErr =
+        MemoryBuffer::getFile(File);
+    if (!ManifestOrErr)
+      reportError(File, ManifestOrErr.getError());
+    MemoryBuffer &Manifest = *ManifestOrErr.get();
+    error(Merger.merge(Manifest));
+  }
 
+  std::unique_ptr<MemoryBuffer> OutputBuffer = Merger.getMergedManifest();
+  outs() << "Buffer size " << OutputBuffer->getBufferSize() << "\n";
+  if (OutputBuffer->getBufferSize() == 0) {
+    outs() << "Empty manifest not written.\n";
+  } else {
+    ErrorOr<std::unique_ptr<FileOutputBuffer>> FileOrErr =
+        FileOutputBuffer::create(OutputFile, OutputBuffer->getBufferSize());
+    outs() << "about to check file or err\n";
+    if (!FileOrErr)
+      reportError(OutputFile, FileOrErr.getError());
+    outs() << "created good\n";
+    std::unique_ptr<FileOutputBuffer> FileBuffer = std::move(*FileOrErr);
+    std::copy(OutputBuffer->getBufferStart(), OutputBuffer->getBufferEnd(),
+              FileBuffer->getBufferStart());
+    error(FileBuffer->commit());
+  }
+
+  return 0;
 }