Merge manifest namespaces.

[ecbeckmann]

Summary

mt.exe performs a tree merge where certain element nodes are combined into one. This introduces the possibility of xml namespaces conflicting with each other. The original mt.exe has a hierarchy whereby certain namespace names can override others, and nodes that would then end up in ambigious namespaces have their namespaces explicitly defined. This namespace handles this merging process.

[ecbeckmann]

Added comments.

[ruiu]

Before looking into details, there are a few things I want to say.

• This is a large patch but the commit message is too terse. Please write it to describe what this patch does.
• Now you have a lot of functions that are individually guarded by #ifdef's, which is somewhat error-prone. It is probably better to have one larger #ifdef and put all functions that are called only when libxml is available into it.

[ecbeckmann]

Updated commit message.

[ecbeckmann]

Put all libxml-only functions in a single ifdef.

[rnk]

llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h

38

This will probably have internal linkage and get duplicated into every TU that includes this header. Can you sink this down into the .cpp file?

This is also just static data, so consider making this an array like this:

static const std::pair<const char *, const char *> MtNsHrefsPrefixes[] = { .... };

That should avoid some dynamic initialization, while still letting you loop over it and make StringRefs: https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

64

I feel like this would be simpler as:

// If either pointer is null, use pointer equality. If both are null, they are equal, otherwise they are unequal.
if (!A || !B)
  return A == B;

69

Right now, these functions are externally visible symbols in the global namespace. We should take some steps to hide them from users of LLVM. Any free function that you don't need outside this file should be static: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

121

Echoing what @ruiu said, I was imagining that we'd have one large ifdef between stub implementations of WindowsManifest::merge etc, and the real implementations and all of the helpers you wrote here that call libxml2.

[ecbeckmann]

Fix formatting errors caused by removing ifdefs.

[ecbeckmann]

Avoid static constructor, remove symbols from global namespace, separate real and stub implementations, simplify string compare.

[rnk]

llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h

58–59

Perhaps we should hide the implementation details here with the pimpl pattern: http://en.cppreference.com/w/cpp/language/pimpl

Then we don't need to include xmlreader.h above behind ifdefs in the header, and clients will not have to worry about possible name or macro conflicts.

The "Impl" class would store the vector of merged XML documents.

[ecbeckmann]

Use PImpl for WindowsManifestMerger.

[ecbeckmann]

Added newline to manifest

[ruiu]

llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h

49–51

Usually we write private members after public members. Consistency matter, so please make new code consistent with existing code in general.

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

79–80

Please run clang-format every time you upload a new patch to phab.

468–475

No else after return (apply this rule to other places in this file.)

701

clang-format.

[zturner]

Mostly style issues. I'd like to see all the char pointers go away in favor of StringRef, unless there's a strong reason why this won't work.

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

53–60

Why not have this be a vector of pairs of StringRef? We're not modifying anything, and these will be in constant storage, so I think StringRef is fine? Maybe even a std::map<StringRef, StringRef>, which can still use initializer list syntax.

69

It would be nice if we could deal entirely with StringRef and do conversions at the highest level possible so we never have to deal with this stuff.

StringRef toStringRef(const unsigned char *Str) {
  return StringRef(FROM_XML_CHAR(Str));
}

static bool isMergeableElement(StringRef S) {
  constexpr StringRef KnownList[] = { ... };
  return llvm::is_contained(KnownList, S);
}

then the above xmlStringsEqual() also just becomes toStringRef(X) == toStringRef(Y), which would most likely just turn into X == Y because you'll be getting rid of the pointer-ness at the highest level and everything will actually be dealing with StringRef behind the scenes.

90–91

For a concrete example of the previous suggestion:

static xmlAttrPtr getAttribute(xmlNodePtr Node, StringRef AttributeName) {
  for (auto Attribute = Node->properties; Attribute; Attribute = Attribute->next) {
    if (toStringRef(Attribute->Name) == AttributeName)
      return true;
  }
  return false;
}

102–103

namespaceOverrides(StringRef HRef1, StringRef HRef2)

111–115

You can use the range-based llvm::find_if(MtNsHrefsPrefixes, [HRef2]...); Alternatively, if my initial suggestion of making it a map works, you could just write: auto HRef2Position = MtNsHrefsPrefixes.find(HRef2);

[ecbeckmann]

In D36201#830843, @zturner wrote:

Mostly style issues. I'd like to see all the char pointers go away in favor of StringRef, unless there's a strong reason why this won't work.

I agree changing to StringRef would be good for consistency. However there are a couple reasons I see not to. One is just simply the string type libxml2 uses is unsigned char *, so keeping as such whenever possible reduces the number of conversions. Second, my merging implementation allows nullptr to be a valid string value, and is used to designate the prefix of a default xml namespace, and libxml also uses this representation. Therefore we have things like making nullptr comparisons, which StringRef explicitly disallows.

llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h

49–51

Sorry, this was leftover from a previous version where I had inline forwarding calls in the header, so I needed the implementation class defined prior.

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

79–80

I had run clang-format before uploading, however I believe the problem is I was diffing against the wrong previous revision so not all my changes were covered.

468–475

also changed 2 other places

[ecbeckmann]

Fix formatting, remove else after return, switched array for map

[ruiu]

Publishing comments about coding style first. I'll do real code review later.

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

82–85

I think I wrote this before, but I believe we usually avoid this style

for (...)
  if (...) {
  }

You want to either use this style

for (...) {
  if (...) {
  }
}

or

for (...)
  if (...)
    ....

479–481

if (foo)
  return true;
return false;

is equivalent to

return foo;

489–490

else after return

[rnk]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

489–490

This can be just return hasInheritedNs(Node) && Node->ns->prefix == nullptr;

495

ditto, you can return the condition.

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

Since HRef1Position and HRef2Position are iterators for the same map, you can compare them directly, can't you?

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

132–136

Sorry for picking up styles, but even in the same patch, you are using two styles. Here, you are doing

for (...) {
  if (...) {
  }
}

but in the above function you did

for (...) {
  if (...)
    ...
}

Consistency matters because it helps people who read your code, so please pay a bit more attention to consistency. It is also a good practice to read your code yourself several times after you think you "finish" your patch, as there's always something you can improve as a finishing touch. It will shorten the time for code review as (1) you will unlikely to get nitpicking comments on style and (2) consistent coding style makes code reviewers more confident that you know what you are doing.

146–152

Maybe a better way of writing this is to use assignments in ifs.

if (xmlNsPtr Def = search(HRef, Node))
  return Def;
if (xmlNsPtr  Def = xmlNewNs(Node, HRef, getPrefixForHref(HRef)))
  return Def;
return make_error<WindowsManifestError>("failed to create new namespace");

173

Remove this blank line.

218

It's good to do continue here, as early continues are preferred.

235

Ditto

349–350

Ditto

[zturner]

Only 1 nitpick, I think reid and rui covered the rest pretty well.

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

659–660

!llvm::is_contained(RequiredPrefixes, Def) will allow you to get this all on a single line. Similar applies to other places in the file.

[zturner]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

659–660

Also, invert the conditional first and then put a continue.

if (!Def->prefix || llvm::is_contained(RequiredPrefixes, Def)) {
  Prev = Def;
  continue;
}

...

This keeps the indentation down and makes it easier to read.

[ecbeckmann]

Style consitency updates.

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

82–85

Ah okay....I think I've heard other variations of a similar principle, but not this exact style rule. I'll change this everywhere.

110–112

the only way to directly compare iterators is using std::distance. However, behavior is undefined if the 2nd iterator is not reachable by incrementing the first iterator, which will often be the case for our usage. Therefore, the only way to correctly compare is to first find each one's absolute position in the map, which I do here.

132–136

I understand, consistency is important. I have been reading the code myself before sending it out for review, but I guess there are certain areas I need to focus on more.

[zturner]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

Maps are ordered, so why not just use the ordering function directly?

return FROM_XML_CHAR(HRef1) > FROM_XML_CHAR(HRef2)?

[ecbeckmann]

use llvm::is_contained

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

Unfortunately HRef1 and HRef2 do not point within the table, but rather xmlChar arrays allocated by libxml2's parser, so we can't compare those.

[zturner]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

I think I'm still mis-understanding something.

The code here is looking both entries up by key, then after determining that both keys are present, it then computes each iterator's distance from the end. And if the first one is farther from the end (e.g. closer to the beginning) than the second one, it returns true, otherwise it returns false.

But when you iterate a map, it iterates in key order. So saying "the first one is closer to the beginning" is identical to saying "the first one is less than the second one". So as long as you use the same comparison function that the map uses internally to order keys, then it should be the same. And in this case, the map seems to be using a string comparison.

So it seems like all you have to do is string compare the two arguments and return true if the first one is less than the second one?

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

I'm no longer using a map, but rather a vector of pairs, therefore the keys stay in the same order as they were put in the definition. No comparison operator is used on the StringRefs. We purely want to find the index in the vector and compare them. So using StringRef / string comparison will not work here.

[zturner]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

Ahh, I see. I didn't realize we were no longer using a map. In that case, why doesn't return Iter1 < Iter2; work? vector::iterator is a random accessor iterator, which supports relational operators.

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

find() in this case returns type "std::_Rb_tree_const_iterator" which does not support comparison. I think it is because it is a vector of pairs, so this iterator is used? I feel like a complicated template specification is required to get it to return vector::iterator

[zturner]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

Something doesn't seem right. If it's std::_Rb_tree_const_iterator, then this can't possibly be a vector of pairs, it's a std::map<StringRef, StringRef>.

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–112

Ah shoot my apologies, I was looking at the wrong revision. It indeed uses a map. Actually, this seems to be a bug I inadvertantly created, because I assume the sort will put it in alphabetical order, when really it should stay in the same order. I will change to a vector and use position comparison on iterators.

[ecbeckmann]

Changed to vector of StringRefs so that no reordering happens.

[ecbeckmann]

Add test case to cover incorrect namespace prioritization.

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

102

Overall, you can simplify this function as

auto It1 = llvm::find_if(
    MtNsHrefsPrefixes,
    [=](const std::pair<StringRef, StringRef> &element) {
      return xmlStringsEqual(HRef1, TO_XML_CHAR(element.first.data()));
    });
auto It2 = llvm::find_if(
    MtNsHrefsPrefixes,
    [=](const std::pair<StringRef, StringRef> &element) {
      return xmlStringsEqual(HRef2, TO_XML_CHAR(element.first.data()));
    });
return It1 < It2;

106

Coding style issue: element -> Element

113

Instead of explicitly capturing variables, use [=] or [&].

element -> Element

116–118

I believe you can just remove

if (HRef2Position == std::end(MtNsHrefsPrefixes))
  return true;

because if HRef2Position is std::end(MtNsHrefsPrefixes), expression HRef1Position < HRef2Position is always true in this context.

218

It's not done.

349–350

Not done.

[ecbeckmann]

Shorten overriding namespace function.

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

218

are you referring to the if block or the else if block? Because on this comment's revision it refers to the else if and I have added a continue since then.

349–350

same

[ecbeckmann]

Remove extra line.

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

218

What I was saying is that, if you have this code

for (...) {
  if (...) {
    if (...) {
      ...
    } else {
      ...
    }
  } else if (...) {
    ...
  } else if (...) {
    ...
  }
}

then you can rewrite it with early continues like this:

for (...) {
  if (...) {
    if (...) {
      ...
      continue;
    }
    ...
    continue;
  }

  if (...) {
    ...
    continue;
  }

  ...
}

[ecbeckmann]

Add last early continue.

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

218

ah okay. As of the most recent revision this criteria should be fulfilled.

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

221

Well, you want to remove continue, as I described in the previous comment. (Please fix not only this place but other places as well.)

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

221

wait I'm confused, I thought we want to add the early continues?

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

221

Ah sorry, I meant not continue but else.

[ecbeckmann]

finish early continue

[rnk]

I think this is pretty close to ready. How do other reviewers feel?

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

116

typo on "recursively"

221

Most of these if / else branches are about equivalent in terms of complexity. I'd actually suggest removing all the continues that got added (except the !Attribute->ns check). Most of the continues in the current code are redundant, because they'd hit the end of the loop naturally anyway.

[ecbeckmann]

Fix comment typo.

[ecbeckmann]

This comment has been deleted.

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

221

I think there's a small benefit in terms of readability

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

274–276

No else after return

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

5–8

Maybe it is better to write

/manifest <path1> \
/manifest <path2> \
...

instead of

<path1> /manifest \
<path2> /manifest \
 ...

because paths are arguments for /manifest.

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

106–107

!A && !B is preferred over !(A || B).

But do you have to do this way? I think usually we handle OPT_UNKNOWN to report unknown options instead of putting all known options to some groups. Look at https://github.com/llvm-project/llvm-project-20170507/blob/dd24779ef3e3e592cc0c7561a2fb1fbe3309fa1c/lld/ELF/DriverUtils.cpp#L112 for example.

[ecbeckmann]

Removed else after return, also switched to early return in 2 other places.

[ecbeckmann]

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

106–107

I have tried this....however for some reason OPT_UNKNOWN never filters any arguments for me. It seems for some reason the invalid flags are recognized as inputs, not flags. I'm not sure if I'm setting up the options incorrectly or if this is a bug in the option library.....also not sure if this is worth the effort to fix.

[ruiu]

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

106–107

Ah, okay, I think I know why it behaves like that. Unlike Unix, Windows command line options start with '/', and the character is ambiguous as to whether it is a path (e.g. /bin/ls) or an option (e.g. /manifest). LLVM's option parser's behavior is that, if an option "/foo" is defined, "/foo" is handled as an option. Otherwise, "/foo" is handled as a path.

This is an unavoidable consequence if you allow both Unix-style paths and Windows-style command line options. Since that's what we want, it is okay.

I think Group<supported> does not do anything. I believe your llvm-mt command still does not warn on unknown options. Please try llvm-mt /foo, for example. My guess is that it will warn that it cannot find file "/foo". You may want to just remove Group<supported> from Options.td and also remove this piece of new code.

[ecbeckmann]

Reformat test.

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

106–107

I think you're right. It also seems to be somewhat of a bug with the option parsing library, OPT_UNKNOWN never seems to do anything. I just tried with lld-link and it seems unable to detect invalid flags, complaining rather that it can't open the given file. So that line in DriverUtils.cpp doesn't actually do anything.

My Group<supported> example does work however. For example, running llvm-mt /foo does indeed detect the invalid option given, instead of interpreting it as an input file.

[ruiu]

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

106–107

OPT_UNKNOWN does work if you pass an argument with - prefix. Try -foo for example. I think it'll be handled as an OPT_UNKNOWN option.

I believe that your Group<supported> doesn't really work. Well, it's working in some sense, but this is the same as iterating over OPT_INPUT, no? I mean, I believe your code is the same as

for (auto *Arg : Args.filtered(OPT_INPUT))
  reportError(Twine("invalid option ") + Arg->getSpelling());

[ecbeckmann]

Remove supported group option

[ecbeckmann]

remove unnecessary encoding change.

[ruiu]

LGTM

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

105–106

Please add a test for an unknown option.

[ecbeckmann]

Added test to handle unknown option

llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h

 //===---------------------------------------------------------------------===//
 
 #ifndef LLVM_INCLUDE_LLVM_SUPPORT_WINDOWS_MANIFEST_MERGER_H
 #define LLVM_INCLUDE_LLVM_SUPPORT_WINDOWS_MANIFEST_MERGER_H
 
 #include "llvm/Config/config.h"
 #include "llvm/Support/Error.h"
 
-#if LLVM_LIBXML2_ENABLED
-#include <libxml/xmlreader.h>
-#endif
-
 namespace llvm {
 
 class MemoryBuffer;
 
-#if LLVM_LIBXML2_ENABLED
-typedef xmlDocPtr XMLDocumentImpl;
-typedef xmlNodePtr XMLNodeImpl;
-#else
-typedef void *XMLDocumentImpl;
-typedef void *XMLNodeImpl;
-#endif
+namespace windows_manifest {
 
 class WindowsManifestError : public ErrorInfo<WindowsManifestError, ECError> {
 public:
   static char ID;
   WindowsManifestError(const Twine &Msg);
   void log(raw_ostream &OS) const override;
 
 private:
   std::string Msg;
 };
 
 class WindowsManifestMerger {
 public:
+  WindowsManifestMerger();
   ~WindowsManifestMerger();
-
   Error merge(const MemoryBuffer &Manifest);
 
   // Returns vector containing merged xml manifest, or uninitialized vector for
   // empty manifest.
   std::unique_ptr<MemoryBuffer> getMergedManifest();
 
 private:
-  static void errorCallback(void *Ctx, const char *Format, ...);
-  Error getParseError();
-
-#if LLVM_LIBXML2_ENABLED
-  XMLDocumentImpl CombinedDoc = nullptr;
-  std::vector<XMLDocumentImpl> MergedDocs;
-#endif
-  bool ParseErrorOccurred = false;
+  class WindowsManifestMergerImpl;
+  std::unique_ptr<WindowsManifestMergerImpl> Impl;
 };
 
+} // namespace windows_manifest
 } // namespace llvm
 #endif

llvm/lib/WindowsManifest/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/WindowsManifest/WindowsManifestMerger.h"
 #include "llvm/Support/MemoryBuffer.h"
 
-#include <stdarg.h>
+#include <map>
+
+#if LLVM_LIBXML2_ENABLED
+#include <libxml/xmlreader.h>
+#endif
 
 #define TO_XML_CHAR(X) reinterpret_cast<const unsigned char *>(X)
 #define FROM_XML_CHAR(X) reinterpret_cast<const char *>(X)
 
 using namespace llvm;
-
-namespace llvm {
+using namespace windows_manifest;
 
 char WindowsManifestError::ID = 0;
 
 WindowsManifestError::WindowsManifestError(const Twine &Msg) : Msg(Msg.str()) {}
 
 void WindowsManifestError::log(raw_ostream &OS) const { OS << Msg; }
 
+class WindowsManifestMerger::WindowsManifestMergerImpl {
+public:
+  ~WindowsManifestMergerImpl();
+  Error merge(const MemoryBuffer &Manifest);
+  std::unique_ptr<MemoryBuffer> getMergedManifest();
+
+private:
+  static void errorCallback(void *Ctx, const char *Format, ...);
+  Error getParseError();
+#if LLVM_LIBXML2_ENABLED
+  xmlDocPtr CombinedDoc = nullptr;
+  std::vector<xmlDocPtr> MergedDocs;
+#endif
+  bool ParseErrorOccurred = false;
+};
+
 #if LLVM_LIBXML2_ENABLED
+
+static const std::pair<StringRef, StringRef> MtNsHrefsPrefixes[] = {
+    {"urn:schemas-microsoft-com:asm.v1", "ms_asmv1"},
+    {"urn:schemas-microsoft-com:asm.v2", "ms_asmv2"},
+    {"urn:schemas-microsoft-com:asm.v3", "ms_asmv3"},
+    {"http://schemas.microsoft.com/SMI/2005/WindowsSettings",
+     "ms_windowsSettings"},
+    {"urn:schemas-microsoft-com:compatibility.v1", "ms_compatibilityv1"}};
+
 static bool xmlStringsEqual(const unsigned char *A, const unsigned char *B) {
+  // Handle null pointers.  Comparison of 2 null pointers returns true because
+  // this indicates the prefix of a default namespace.
+  if (!A || !B)
+    return A == B;
   return strcmp(FROM_XML_CHAR(A), FROM_XML_CHAR(B)) == 0;
 }
-#endif
 
-bool isMergeableElement(const unsigned char *ElementName) {
+static bool isMergeableElement(const unsigned char *ElementName) {
   for (StringRef S : {"application", "assembly", "assemblyIdentity",
                       "compatibility", "noInherit", "requestedExecutionLevel",
                       "requestedPrivileges", "security", "trustInfo"}) {
-    if (S == FROM_XML_CHAR(ElementName))
+    if (S == FROM_XML_CHAR(ElementName)) {
       return true;
     }
+  }
   return false;
 }
 
-XMLNodeImpl getChildWithName(XMLNodeImpl Parent,
+static xmlNodePtr getChildWithName(xmlNodePtr Parent,
                                    const unsigned char *ElementName) {
-#if LLVM_LIBXML2_ENABLED
-  for (XMLNodeImpl Child = Parent->children; Child; Child = Child->next)
+  for (xmlNodePtr Child = Parent->children; Child; Child = Child->next) {
     if (xmlStringsEqual(Child->name, ElementName)) {
       return Child;
     }
-#endif
+  }
   return nullptr;
 }
 
-const unsigned char *getAttribute(XMLNodeImpl Node,
-                                  const unsigned char *AttributeName) {
-#if LLVM_LIBXML2_ENABLED
+static xmlAttrPtr getAttribute(xmlNodePtr Node,
   for (xmlAttrPtr Attribute = Node->properties; Attribute != nullptr;
        Attribute = Attribute->next) {
-    if (xmlStringsEqual(Attribute->name, AttributeName))
-      return Attribute->children->content;
+    if (xmlStringsEqual(Attribute->name, AttributeName)) {
+      return Attribute;
+    }
   }
-#endif
   return nullptr;
 }
 
-Error mergeAttributes(XMLNodeImpl OriginalNode, XMLNodeImpl AdditionalNode) {
-#if LLVM_LIBXML2_ENABLED
-  for (xmlAttrPtr Attribute = AdditionalNode->properties; Attribute != nullptr;
+// Check if namespace specified by HRef1 overrides that of HRef2.
+static bool namespaceOverrides(const unsigned char *HRef1,
+                               const unsigned char *HRef2) {
+  auto HRef1Position = llvm::find_if(
+      MtNsHrefsPrefixes, [=](const std::pair<StringRef, StringRef> &Element) {
+        return xmlStringsEqual(HRef1, TO_XML_CHAR(Element.first.data()));
+      });
+  auto HRef2Position = llvm::find_if(
+      MtNsHrefsPrefixes, [=](const std::pair<StringRef, StringRef> &Element) {
+        return xmlStringsEqual(HRef2, TO_XML_CHAR(Element.first.data()));
+      });
+  return HRef1Position < HRef2Position;
+}
+
+// Search for prefix-defined namespace specified by HRef, starting on Node and
+// continuing recursively upwards.  Returns the namespace or nullptr if not
+// found.
+static xmlNsPtr search(const unsigned char *HRef, xmlNodePtr Node) {
+  for (xmlNsPtr Def = Node->nsDef; Def; Def = Def->next) {
+    if (Def->prefix && xmlStringsEqual(Def->href, HRef)) {
+      return Def;
+    }
+  }
+  if (Node->parent) {
+    return search(HRef, Node->parent);
+  }
+  return nullptr;
+}
+
+// Return the prefix that corresponds to the HRef.  If HRef is not a recognized
+// URI, then just return the HRef itself to use as the prefix.
+static const unsigned char *getPrefixForHref(const unsigned char *HRef) {
+  for (auto &Ns : MtNsHrefsPrefixes) {
+    if (xmlStringsEqual(HRef, TO_XML_CHAR(Ns.first.data()))) {
+      return TO_XML_CHAR(Ns.second.data());
+    }
+  }
+  return HRef;
+}
+
+// Search for prefix-defined namespace specified by HRef, starting on Node and
+// continuing recursively upwards.  If it is found, then return it.  If it is
+// not found, then prefix-define that namespace on the node and return a
+// reference to it.
+static Expected<xmlNsPtr> searchOrDefine(const unsigned char *HRef,
+                                         xmlNodePtr Node) {
+  if (xmlNsPtr Def = search(HRef, Node))
+    return Def;
+  if (xmlNsPtr Def = xmlNewNs(Node, HRef, getPrefixForHref(HRef)))
+    return Def;
+  return make_error<WindowsManifestError>("failed to create new namespace");
+}
+
+// Set the namespace of OrigionalAttribute on OriginalNode to be that of
+// AdditionalAttribute's.
+static Error copyAttributeNamespace(xmlAttrPtr OriginalAttribute,
+                                    xmlNodePtr OriginalNode,
+                                    xmlAttrPtr AdditionalAttribute) {
+
+  Expected<xmlNsPtr> ExplicitOrError =
+      searchOrDefine(AdditionalAttribute->ns->href, OriginalNode);
+  if (!ExplicitOrError)
+    return ExplicitOrError.takeError();
+  OriginalAttribute->ns = std::move(ExplicitOrError.get());
+  return Error::success();
+}
+
+// Return the corresponding namespace definition for the prefix, defined on the
+// given Node.  Returns nullptr if there is no such definition.
+static xmlNsPtr getNamespaceWithPrefix(const unsigned char *Prefix,
+                                       xmlNodePtr Node) {
+  if (Node == nullptr)
+    return nullptr;
+  for (xmlNsPtr Def = Node->nsDef; Def; Def = Def->next) {
+    if (xmlStringsEqual(Def->prefix, Prefix)) {
+      return Def;
+    }
+  }
+  return nullptr;
+}
+
+// Search for the closest inheritable default namespace, starting on (and
+// including) the Node and traveling upwards through parent nodes.  Returns
+// nullptr if there are no inheritable default namespaces.
+static xmlNsPtr getClosestDefault(xmlNodePtr Node) {
+  if (xmlNsPtr Ret = getNamespaceWithPrefix(nullptr, Node))
+    return Ret;
+  if (Node->parent == nullptr)
+    return nullptr;
+  return getClosestDefault(Node->parent);
+}
+
+// Merge the attributes of AdditionalNode into OriginalNode.  If attributes
+// with identical types are present, they are not duplicated but rather if
+// their values are not consistent and error is thrown.  In addition, the
+// higher priority namespace is used for each attribute, EXCEPT in the case
+// of merging two default namespaces and the lower priority namespace
+// definition occurs closer than the higher priority one.
+static Error mergeAttributes(xmlNodePtr OriginalNode,
+                             xmlNodePtr AdditionalNode) {
+  xmlNsPtr ClosestDefault = getClosestDefault(OriginalNode);
+  for (xmlAttrPtr Attribute = AdditionalNode->properties; Attribute;
        Attribute = Attribute->next) {
-    if (const unsigned char *OriginalValue =
+    if (xmlAttrPtr OriginalAttribute =
             getAttribute(OriginalNode, Attribute->name)) {
-      // Attributes of the same name must also have the same value.  Otherwise
-      // an error is thrown.
-      if (!xmlStringsEqual(OriginalValue, Attribute->children->content))
+      if (!xmlStringsEqual(OriginalAttribute->children->content,
+                           Attribute->children->content)) {
         return make_error<WindowsManifestError>(
             Twine("conflicting attributes for ") +
             FROM_XML_CHAR(OriginalNode->name));
+      }
+      if (!Attribute->ns) {
+        continue;
+      }
+      if (!OriginalAttribute->ns) {
+        if (auto E = copyAttributeNamespace(OriginalAttribute, OriginalNode,
+                                            Attribute)) {
+          return E;
+        }
+        continue;
+      }
+      if (namespaceOverrides(OriginalAttribute->ns->href,
+                             Attribute->ns->href)) {
+        // In this case, the original attribute has a higher priority namespace
+        // than the incomiing attribute, however the namespace definition of
+        // the lower priority namespace occurs first traveling upwards in the
+        // tree.  Therefore the lower priority namespace is applied.
+        if (!OriginalAttribute->ns->prefix && !Attribute->ns->prefix &&
+            ClosestDefault &&
+            xmlStringsEqual(Attribute->ns->href, ClosestDefault->href)) {
+          if (auto E = copyAttributeNamespace(OriginalAttribute, OriginalNode,
+                                              Attribute)) {
+            return E;
+          }
+          continue;
+        }
+        continue;
+        // This covers the case where the incoming attribute has the higher
+        // priority.  The higher priority namespace is applied in all cases
+        // EXCEPT when both of the namespaces are default inherited, and the
+        // closest inherited default is the lower priority one.
+      }
+      if (Attribute->ns->prefix || OriginalAttribute->ns->prefix ||
+          (ClosestDefault && !xmlStringsEqual(OriginalAttribute->ns->href,
+                                              ClosestDefault->href))) {
+        if (auto E = copyAttributeNamespace(OriginalAttribute, OriginalNode,
+                                            Attribute)) {
+          return E;
+        }
+        continue;
+      }
+      continue;
+    }
+    // If the incoming attribute is not already found on the node, append it
+    // to the end of the properties list.  Also explicitly apply its
+    // namespace as a prefix because it might be contained in a separate
+    // namespace that doesn't use the attribute.
+    xmlAttrPtr NewProp =
+        xmlNewProp(OriginalNode, Attribute->name, Attribute->children->content);
+    Expected<xmlNsPtr> ExplicitOrError =
+        searchOrDefine(Attribute->ns->href, OriginalNode);
+    if (!ExplicitOrError)
+      return ExplicitOrError.takeError();
+    NewProp->ns = std::move(ExplicitOrError.get());
+  }
+  return Error::success();
+}
+
+// Given two nodes, return the one with the higher priority namespace.
+static xmlNodePtr getDominantNode(xmlNodePtr Node1, xmlNodePtr Node2) {
+
+  if (!Node1 || !Node1->ns)
+    return Node2;
+  if (!Node2 || !Node2->ns)
+    return Node1;
+  if (namespaceOverrides(Node1->ns->href, Node2->ns->href))
+    return Node1;
+  return Node2;
+}
+
+// Checks if this Node's namespace is inherited or one it defined itself.
+static bool hasInheritedNs(xmlNodePtr Node) {
+  return Node->ns && Node->ns != getNamespaceWithPrefix(Node->ns->prefix, Node);
+}
+
+// Check if this Node's namespace is a default namespace that it inherited, as
+// opposed to defining itself.
+static bool hasInheritedDefaultNs(xmlNodePtr Node) {
+  return hasInheritedNs(Node) && Node->ns->prefix == nullptr;
+}
+
+// Check if this Node's namespace is a default namespace it defined itself.
+static bool hasDefinedDefaultNamespace(xmlNodePtr Node) {
+  return Node->ns && (Node->ns == getNamespaceWithPrefix(nullptr, Node));
+}
+
+// For the given explicit prefix-definition of a namespace, travel downwards
+// from a node recursively, and for every implicit, inherited default usage of
+// that namespace replace it with that explicit prefix use.  This is important
+// when namespace overriding occurs when merging, so that elements unique to a
+// namespace will still stay in that namespace.
+static void explicateNamespace(xmlNsPtr PrefixDef, xmlNodePtr Node) {
+  // If a node as its own default namespace definition it clearly cannot have
+  // inherited the given default namespace, and neither will any of its
+  // children.
+  if (hasDefinedDefaultNamespace(Node))
+    return;
+  if (Node->ns && xmlStringsEqual(Node->ns->href, PrefixDef->href) &&
+      hasInheritedDefaultNs(Node))
+    Node->ns = PrefixDef;
+  for (xmlAttrPtr Attribute = Node->properties; Attribute;
+       Attribute = Attribute->next) {
+    if (Attribute->ns &&
+        xmlStringsEqual(Attribute->ns->href, PrefixDef->href)) {
+      Attribute->ns = PrefixDef;
+    }
+  }
+  for (xmlNodePtr Child = Node->children; Child; Child = Child->next) {
+    explicateNamespace(PrefixDef, Child);
+  }
+}
+
+// Perform the namespace merge between two nodes.
+static Error mergeNamespaces(xmlNodePtr OriginalNode,
+                             xmlNodePtr AdditionalNode) {
+  // Save the original default namespace definition in case the incoming node
+  // overrides it.
+  const unsigned char *OriginalDefinedDefaultHref = nullptr;
+  if (xmlNsPtr OriginalDefinedDefaultNs =
+          getNamespaceWithPrefix(nullptr, OriginalNode)) {
+    OriginalDefinedDefaultHref = xmlStrdup(OriginalDefinedDefaultNs->href);
+  }
+  const unsigned char *NewDefinedDefaultHref = nullptr;
+  // Copy all namespace definitions.  There can only be one default namespace
+  // definition per node, so the higher priority one takes precedence in the
+  // case of collision.
+  for (xmlNsPtr Def = AdditionalNode->nsDef; Def; Def = Def->next) {
+    if (xmlNsPtr OriginalNsDef =
+            getNamespaceWithPrefix(Def->prefix, OriginalNode)) {
+      if (!Def->prefix) {
+        if (namespaceOverrides(Def->href, OriginalNsDef->href)) {
+          NewDefinedDefaultHref = TO_XML_CHAR(strdup(FROM_XML_CHAR(Def->href)));
+        }
+      } else if (!xmlStringsEqual(OriginalNsDef->href, Def->href)) {
+        return make_error<WindowsManifestError>(
+            Twine("conflicting namespace definitions for ") +
+            FROM_XML_CHAR(Def->prefix));
+      }
     } else {
-      char *NameCopy = strdup(FROM_XML_CHAR(Attribute->name));
-      char *ContentCopy = strdup(FROM_XML_CHAR(Attribute->children->content));
-      xmlNewProp(OriginalNode, TO_XML_CHAR(NameCopy), TO_XML_CHAR(ContentCopy));
+      xmlNsPtr NewDef = xmlCopyNamespace(Def);
+      NewDef->next = OriginalNode->nsDef;
+      OriginalNode->nsDef = NewDef;
     }
   }
-#endif
+
+  // Check whether the original node or the incoming node has the higher
+  // priority namespace.  Depending on which one is dominant, we will have
+  // to recursively apply namespace changes down to children of the original
+  // node.
+  xmlNodePtr DominantNode = getDominantNode(OriginalNode, AdditionalNode);
+  xmlNodePtr NonDominantNode =
+      DominantNode == OriginalNode ? AdditionalNode : OriginalNode;
+  if (DominantNode == OriginalNode) {
+    if (OriginalDefinedDefaultHref) {
+      xmlNsPtr NonDominantDefinedDefault =
+          getNamespaceWithPrefix(nullptr, NonDominantNode);
+      // In this case, both the nodes defined a default namespace.  However
+      // the lower priority node ended up having a higher priority default
+      // definition.  This can occur if the higher priority node is prefix
+      // namespace defined.  In this case we have to define an explicit
+      // prefix for the overridden definition and apply it to all children
+      // who relied on that definition.
+      if (NonDominantDefinedDefault &&
+          namespaceOverrides(NonDominantDefinedDefault->href,
+                             OriginalDefinedDefaultHref)) {
+        Expected<xmlNsPtr> EC =
+            searchOrDefine(OriginalDefinedDefaultHref, DominantNode);
+        if (!EC) {
+          return EC.takeError();
+        }
+        xmlNsPtr PrefixDominantDefinedDefault = std::move(EC.get());
+        explicateNamespace(PrefixDominantDefinedDefault, DominantNode);
+      }
+      // In this case the node with a higher priority namespace did not have a
+      // default namespace definition, but the lower priority node did.  In this
+      // case the new default namespace definition is copied.  A side effect of
+      // this is that all children will suddenly find themselves in a different
+      // default namespace.  To maintain correctness we need to ensure that all
+      // children now explicitly refer to the namespace that they had previously
+      // implicitly inherited.
+    } else if (getNamespaceWithPrefix(nullptr, NonDominantNode)) {
+      if (DominantNode->parent) {
+        xmlNsPtr ClosestDefault = getClosestDefault(DominantNode->parent);
+        Expected<xmlNsPtr> EC =
+            searchOrDefine(ClosestDefault->href, DominantNode);
+        if (!EC) {
+          return EC.takeError();
+        }
+        xmlNsPtr ExplicitDefault = std::move(EC.get());
+        explicateNamespace(ExplicitDefault, DominantNode);
+      }
+    }
+  } else {
+    // Covers case where the incoming node has a default namespace definition
+    // that overrides the original node's namespace.  This always leads to
+    // the original node receiving that new default namespace.
+    if (hasDefinedDefaultNamespace(DominantNode)) {
+      NonDominantNode->ns = getNamespaceWithPrefix(nullptr, NonDominantNode);
+    } else {
+      // This covers the case where the incoming node either has a prefix
+      // namespace, or an inherited default namespace.  Since the namespace
+      // may not yet be defined in the original tree we do a searchOrDefine
+      // for it, and then set the namespace equal to it.
+      Expected<xmlNsPtr> EC =
+          searchOrDefine(DominantNode->ns->href, NonDominantNode);
+      if (!EC) {
+        return EC.takeError();
+      }
+      xmlNsPtr Explicit = std::move(EC.get());
+      NonDominantNode->ns = Explicit;
+    }
+    // This covers cases where the incoming dominant node HAS a default
+    // namespace definition, but MIGHT NOT NECESSARILY be in that namespace.
+    if (xmlNsPtr DominantDefaultDefined =
+            getNamespaceWithPrefix(nullptr, DominantNode)) {
+      if (OriginalDefinedDefaultHref) {
+        if (namespaceOverrides(DominantDefaultDefined->href,
+                               OriginalDefinedDefaultHref)) {
+          // In this case, the incoming node's default definition overrides
+          // the original default definition, all children who relied on that
+          // definition must be updated accordingly.
+          Expected<xmlNsPtr> EC =
+              searchOrDefine(OriginalDefinedDefaultHref, NonDominantNode);
+          if (!EC) {
+            return EC.takeError();
+          }
+          xmlNsPtr ExplicitDefault = std::move(EC.get());
+          explicateNamespace(ExplicitDefault, NonDominantNode);
+        }
+      } else {
+        // The original did not define a default definition, however the new
+        // default definition still applies to all children, so they must be
+        // updated to explicitly refer to the namespace they had previously
+        // been inheriting implicitly.
+        xmlNsPtr ClosestDefault = getClosestDefault(NonDominantNode);
+        Expected<xmlNsPtr> EC =
+            searchOrDefine(ClosestDefault->href, NonDominantNode);
+        if (!EC) {
+          return EC.takeError();
+        }
+        xmlNsPtr ExplicitDefault = std::move(EC.get());
+        explicateNamespace(ExplicitDefault, NonDominantNode);
+      }
+    }
+  }
+  if (NewDefinedDefaultHref) {
+    xmlNsPtr OriginalNsDef = getNamespaceWithPrefix(nullptr, OriginalNode);
+    xmlFree(const_cast<unsigned char *>(OriginalNsDef->href));
+    OriginalNsDef->href = NewDefinedDefaultHref;
+  }
+  xmlFree(const_cast<unsigned char *>(OriginalDefinedDefaultHref));
   return Error::success();
 }
 
-Error treeMerge(XMLNodeImpl OriginalRoot, XMLNodeImpl AdditionalRoot) {
-#if LLVM_LIBXML2_ENABLED
-  XMLNodeImpl AdditionalFirstChild = AdditionalRoot->children;
+static bool isRecognizedNamespace(const unsigned char *NsHref) {
+  for (auto &Ns : MtNsHrefsPrefixes) {
+    if (xmlStringsEqual(NsHref, TO_XML_CHAR(Ns.first.data()))) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static bool hasRecognizedNamespace(xmlNodePtr Node) {
+  return isRecognizedNamespace(Node->ns->href);
+}
+
+// Ensure a node's inherited namespace is actually defined in the tree it
+// resides in.
+static Error reconcileNamespaces(xmlNodePtr Node) {
+  if (!Node) {
+    return Error::success();
+  }
+  if (hasInheritedNs(Node)) {
+    Expected<xmlNsPtr> ExplicitOrError = searchOrDefine(Node->ns->href, Node);
+    if (!ExplicitOrError) {
+      return ExplicitOrError.takeError();
+    }
+    xmlNsPtr Explicit = std::move(ExplicitOrError.get());
+    Node->ns = Explicit;
+  }
+  for (xmlNodePtr Child = Node->children; Child; Child = Child->next) {
+    if (auto E = reconcileNamespaces(Child)) {
+      return E;
+    }
+  }
+  return Error::success();
+}
+
+// Recursively merge the two given manifest trees, depending on which elements
+// are of a mergeable type, and choose namespaces according to which have
+// higher priority.
+static Error treeMerge(xmlNodePtr OriginalRoot, xmlNodePtr AdditionalRoot) {
+  if (auto E = mergeAttributes(OriginalRoot, AdditionalRoot))
+    return E;
+  if (auto E = mergeNamespaces(OriginalRoot, AdditionalRoot))
+    return E;
+  xmlNodePtr AdditionalFirstChild = AdditionalRoot->children;
   xmlNode StoreNext;
-  for (XMLNodeImpl Child = AdditionalFirstChild; Child; Child = Child->next) {
-    XMLNodeImpl OriginalChildWithName;
+  for (xmlNodePtr Child = AdditionalFirstChild; Child; Child = Child->next) {
+    xmlNodePtr OriginalChildWithName;
     if (!isMergeableElement(Child->name) ||
         !(OriginalChildWithName =
-              getChildWithName(OriginalRoot, Child->name))) {
+              getChildWithName(OriginalRoot, Child->name)) ||
+        !hasRecognizedNamespace(Child)) {
       StoreNext.next = Child->next;
       xmlUnlinkNode(Child);
-      if (!xmlAddChild(OriginalRoot, Child))
+      if (!xmlAddChild(OriginalRoot, Child)) {
         return make_error<WindowsManifestError>(Twine("could not merge ") +
                                                 FROM_XML_CHAR(Child->name));
+      }
+      if (auto E = reconcileNamespaces(Child)) {
+        return E;
+      }
       Child = &StoreNext;
     } else if (auto E = treeMerge(OriginalChildWithName, Child)) {
       return E;
     }
   }
-  if (auto E = mergeAttributes(OriginalRoot, AdditionalRoot))
-    return E;
-#endif
   return Error::success();
 }
 
-void stripCommentsAndText(XMLNodeImpl Root) {
-#if LLVM_LIBXML2_ENABLED
+static void stripComments(xmlNodePtr Root) {
   xmlNode StoreNext;
-  for (XMLNodeImpl Child = Root->children; Child; Child = Child->next) {
-    if (!xmlStringsEqual(Child->name, TO_XML_CHAR("text")) &&
-        !xmlStringsEqual(Child->name, TO_XML_CHAR("comment"))) {
-      stripCommentsAndText(Child);
-    } else {
-      StoreNext.next = Child->next;
-      XMLNodeImpl Remove = Child;
-      Child = &StoreNext;
-      xmlUnlinkNode(Remove);
-      xmlFreeNode(Remove);
-    }
-  }
-#endif
+  for (xmlNodePtr Child = Root->children; Child; Child = Child->next) {
+    if (!xmlStringsEqual(Child->name, TO_XML_CHAR("comment"))) {
+      stripComments(Child);
+      continue;
+    }
+    xmlNodePtr Remove = Child;
+
+// libxml2 assumes that attributes do not inherit default namespaces, whereas
+// the original  mt.exe does make this assumption.  This function reconciles
+// this by setting all attributes to have the inherited default namespace.
+static void setAttributeNamespaces(xmlNodePtr Node) {
+  for (xmlAttrPtr Attribute = Node->properties; Attribute;
+       Attribute = Attribute->next) {
+    if (!Attribute->ns) {
+      Attribute->ns = getClosestDefault(Node);
+    }
+  }
+  for (xmlNodePtr Child = Node->children; Child; Child = Child->next) {
+    setAttributeNamespaces(Child);
+  }
 }
 
-WindowsManifestMerger::~WindowsManifestMerger() {
-#if LLVM_LIBXML2_ENABLED
+// The merging process may create too many prefix defined namespaces.  This
+// function removes all unnecessary ones from the tree.
+static void checkAndStripPrefixes(xmlNodePtr Node,
+                                  std::vector<xmlNsPtr> &RequiredPrefixes) {
+  for (xmlNodePtr Child = Node->children; Child; Child = Child->next) {
+    checkAndStripPrefixes(Child, RequiredPrefixes);
+  }
+  if (Node->ns && Node->ns->prefix != nullptr) {
+    xmlNsPtr ClosestDefault = getClosestDefault(Node);
+    if (ClosestDefault &&
+        xmlStringsEqual(ClosestDefault->href, Node->ns->href)) {
+      Node->ns = ClosestDefault;
+    } else if (!llvm::is_contained(RequiredPrefixes, Node->ns)) {
+      RequiredPrefixes.push_back(Node->ns);
+    }
+  }
+  for (xmlAttrPtr Attribute = Node->properties; Attribute;
+       Attribute = Attribute->next) {
+    if (Attribute->ns && Attribute->ns->prefix != nullptr) {
+      xmlNsPtr ClosestDefault = getClosestDefault(Node);
+      if (ClosestDefault &&
+          xmlStringsEqual(ClosestDefault->href, Attribute->ns->href)) {
+        Attribute->ns = ClosestDefault;
+      } else if (!llvm::is_contained(RequiredPrefixes, Node->ns)) {
+        RequiredPrefixes.push_back(Attribute->ns);
+      }
+    }
+  }
+  xmlNsPtr Prev;
+  xmlNs Temp;
+  for (xmlNsPtr Def = Node->nsDef; Def; Def = Def->next) {
+    if (!Def->prefix || llvm::is_contained(RequiredPrefixes, Def)) {
+      Prev = Def;
+      continue;
+    }
+    if (Def == Node->nsDef) {
+      Node->nsDef = Def->next;
+    } else {
+      Prev->next = Def->next;
+    }
+    Temp.next = Def->next;
+    xmlFreeNs(Def);
+    Def = &Temp;
+  }
+}
+
+WindowsManifestMerger::WindowsManifestMergerImpl::~WindowsManifestMergerImpl() {
   for (auto &Doc : MergedDocs)
     xmlFreeDoc(Doc);
-#endif
 }
 
-Error WindowsManifestMerger::merge(const MemoryBuffer &Manifest) {
-#if LLVM_LIBXML2_ENABLED
+Error WindowsManifestMerger::WindowsManifestMergerImpl::merge(
+    const MemoryBuffer &Manifest) {
   if (Manifest.getBufferSize() == 0)
     return make_error<WindowsManifestError>(
         "attempted to merge empty manifest");
-  xmlSetGenericErrorFunc((void *)this, WindowsManifestMerger::errorCallback);
-  XMLDocumentImpl ManifestXML =
+  xmlSetGenericErrorFunc((void *)this,
+                         WindowsManifestMergerImpl::errorCallback);
+  xmlDocPtr ManifestXML =
       xmlReadMemory(Manifest.getBufferStart(), Manifest.getBufferSize(),
                     "manifest.xml", nullptr, XML_PARSE_NOBLANKS);
   xmlSetGenericErrorFunc(nullptr, nullptr);
   if (auto E = getParseError())
     return E;
-  XMLNodeImpl AdditionalRoot = xmlDocGetRootElement(ManifestXML);
-  stripCommentsAndText(AdditionalRoot);
+  xmlNodePtr AdditionalRoot = xmlDocGetRootElement(ManifestXML);
+  stripComments(AdditionalRoot);
+  setAttributeNamespaces(AdditionalRoot);
   if (CombinedDoc == nullptr) {
     CombinedDoc = ManifestXML;
   } else {
-    XMLNodeImpl CombinedRoot = xmlDocGetRootElement(CombinedDoc);
-    if (xmlStringsEqual(CombinedRoot->name, AdditionalRoot->name) &&
-        isMergeableElement(AdditionalRoot->name)) {
-      if (auto E = treeMerge(CombinedRoot, AdditionalRoot)) {
-        return E;
+    xmlNodePtr CombinedRoot = xmlDocGetRootElement(CombinedDoc);
+    if (!xmlStringsEqual(CombinedRoot->name, AdditionalRoot->name) ||
+        !isMergeableElement(AdditionalRoot->name) ||
+        !hasRecognizedNamespace(AdditionalRoot)) {
+      return make_error<WindowsManifestError>("multiple root nodes");
+    }
       }
-    } else {
-      return make_error<WindowsManifestError>("multiple root nodes");
-    }
   }
   MergedDocs.push_back(ManifestXML);
-#endif
   return Error::success();
 }
 
-std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
-#if LLVM_LIBXML2_ENABLED
+std::unique_ptr<MemoryBuffer>
+WindowsManifestMerger::WindowsManifestMergerImpl::getMergedManifest() {
   unsigned char *XmlBuff;
   int BufferSize = 0;
   if (CombinedDoc) {
+    xmlNodePtr CombinedRoot = xmlDocGetRootElement(CombinedDoc);
+    std::vector<xmlNsPtr> RequiredPrefixes;
+    checkAndStripPrefixes(CombinedRoot, RequiredPrefixes);
     std::unique_ptr<xmlDoc> OutputDoc(xmlNewDoc((const unsigned char *)"1.0"));
-    xmlDocSetRootElement(OutputDoc.get(), xmlDocGetRootElement(CombinedDoc));
+    xmlDocSetRootElement(OutputDoc.get(), CombinedRoot);
     xmlKeepBlanksDefault(0);
-    xmlDocDumpFormatMemory(OutputDoc.get(), &XmlBuff, &BufferSize, 1);
+    xmlDocDumpFormatMemoryEnc(OutputDoc.get(), &XmlBuff, &BufferSize, "UTF-8",
+                              1);
   }
   if (BufferSize == 0)
     return nullptr;
   return MemoryBuffer::getMemBuffer(
       StringRef(FROM_XML_CHAR(XmlBuff), (size_t)BufferSize));
+}
+
 #else
+
+WindowsManifestMerger::WindowsManifestMergerImpl::~WindowsManifestMergerImpl() {
+}
+
+Error WindowsManifestMerger::WindowsManifestMergerImpl::merge(
+    const MemoryBuffer &Manifest) {
+  return Error::success();
+}
+
+std::unique_ptr<MemoryBuffer>
+WindowsManifestMerger::WindowsManifestMergerImpl::getMergedManifest() {
   return nullptr;
+}
+
 #endif
+
+WindowsManifestMerger::WindowsManifestMerger()
+    : Impl(make_unique<WindowsManifestMergerImpl>()) {}
+
+WindowsManifestMerger::~WindowsManifestMerger() {}
+
+Error WindowsManifestMerger::merge(const MemoryBuffer &Manifest) {
+  return Impl->merge(Manifest);
 }
 
-void WindowsManifestMerger::errorCallback(void *Ctx, const char *Format, ...) {
-  auto *Merger = (WindowsManifestMerger *)Ctx;
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
+  return Impl->getMergedManifest();
+}
+
+void WindowsManifestMerger::WindowsManifestMergerImpl::errorCallback(
+    void *Ctx, const char *Format, ...) {
+  auto *Merger = (WindowsManifestMergerImpl *)Ctx;
   Merger->ParseErrorOccurred = true;
 }
 
-Error WindowsManifestMerger::getParseError() {
+Error WindowsManifestMerger::WindowsManifestMergerImpl::getParseError() {
   if (!ParseErrorOccurred)
     return Error::success();
   return make_error<WindowsManifestError>("invalid xml document");
 }
-
-} // namespace llvm

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

+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v2" manifestVersion="1.0">
+  <dependency>
+    <dependentAssembly>
+      <assemblyIdentity foo="bar" name="identity1" version="1.2.3.4"></assemblyIdentity>
+    </dependentAssembly>
+  </dependency>
+</assembly>

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

+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
+  <application>
+    <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
+      <supportedOS Id="FooOS"/>
+      <supportedOS Id="BarOS"/>
+    </compatibility>
+  </application>
+</assembly>

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

+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+<ms_asmv1:trustInfo xmlns="urn:schemas-microsoft-com:asm.v3" xmlns:ms_asmv1="urn:schemas-microsoft-com:asm.v1">
+  <ms_asmv1:security>
+    <ms_asmv1:requestedPrivileges>
+      <ms_asmv1:requestedExecutionLevel level="trust1" ms_asmv1:access="false"></ms_asmv1:requestedExecutionLevel>
+    </ms_asmv1:requestedPrivileges>
+  </ms_asmv1:security>
+</ms_asmv1:trustInfo>
+<ms_asmv2:dependency xmlns:ms_asmv2="urn:schemas-microsoft-com:asm.v2">
+  <ms_asmv2:dependentAssembly>
+    <ms_asmv2:assemblyIdentity foo="bar" name="identity1" version="1.2.3.4"></assemblyIdentity>
+  </ms_asmv2:dependentAssembly>
+</ms_asmv2:dependency>
+<dependency>
+  <dependentAssembly>
+    <assemblyIdentity bar="foo" name="my assembly" version="5"></assemblyIdentity>
+  </dependentAssembly>
+</dependency>
+<application xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
+  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
+    <supportedOS Id="FooOS"/>
+    <supportedOS Id="BarOS"/>
+  </compatibility>
+  <asmv3:windowsSettings xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">
+    <dpiAware>true/pm</dpiAware>
+  </asmv3:windowsSettings>
+</application>
+</assembly>

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/Inputs/trust_and_identity.manifest

+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+
+  <!--random comment -->
+
+  <dependency>
+    <dependentAssembly>
+      <assemblyIdentity bar="foo" name="my assembly" version="5"></assemblyIdentity>
+    </dependentAssembly>
+  </dependency>
+
+  <trustInfo>
+    <security>
+      <requestedPrivileges>
+        <requestedExecutionLevel level="trust1" access="false"/>
+      </requestedPrivileges>
+    </security>
+  </trustInfo>
+
+</assembly>

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

+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
+  <security>
+    <requestedPrivileges>
+      <requestedExecutionLevel level="trust1"></requestedExecutionLevel>
+    </requestedPrivileges>
+  </security>
+</trustInfo></assembly>

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

+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings" manifestVersion="1.0">
+ <asmv3:application xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
+    <asmv3:windowsSettings xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">
+      <dpiAware>true/pm</dpiAware>
+    </asmv3:windowsSettings>
+  </asmv3:application>
+</assembly>

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

+REQUIRES: libxml2
+UNSUPPORTED: windows
+
+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"/>
+CHECK-NEXT:     </ms_asmv1:requestedPrivileges>
+CHECK-NEXT:   </ms_asmv1:security>
+CHECK-NEXT: </ms_asmv1:trustInfo>
+CHECK-NEXT: <ms_asmv2:dependency xmlns:ms_asmv2="urn:schemas-microsoft-com:asm.v2">
+CHECK-NEXT:   <ms_asmv2:dependentAssembly>
+CHECK-NEXT:     <ms_asmv2:assemblyIdentity foo="bar" name="identity1" version="1.2.3.4"/>
+CHECK-NEXT:   </ms_asmv2:dependentAssembly>
+CHECK-NEXT: </ms_asmv2:dependency>
+CHECK-NEXT: <dependency>
+CHECK-NEXT:   <dependentAssembly>
+CHECK-NEXT:     <assemblyIdentity bar="foo" name="my assembly" version="5"/>
+CHECK-NEXT:   </dependentAssembly>
+CHECK-NEXT: </dependency>
+CHECK-NEXT: <application xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
+CHECK-NEXT:   <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
+CHECK-NEXT:     <supportedOS Id="FooOS"/>
+CHECK-NEXT:     <supportedOS Id="BarOS"/>
+CHECK-NEXT:   </compatibility>
+CHECK-NEXT:   <asmv3:windowsSettings xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">
+CHECK-NEXT:     <dpiAware>true/pm</dpiAware>
+CHECK-NEXT:   </asmv3:windowsSettings>
+CHECK-NEXT: </application>
+CHECK-NEXT: </assembly>

llvm/test/tools/llvm-mt/simple_merge.test

 REQUIRES: libxml2
 UNSUPPORTED: windows
 
 RUN: llvm-mt /manifest %p/Inputs/test_manifest.manifest /manifest \
 RUN:   %p/Inputs/additional.manifest /out:%t
 RUN: FileCheck %s -input-file=%t
 
-CHECK:      <?xml version="1.0"?>
+CHECK:      <?xml version="1.0" encoding="UTF-8"?>
 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" admin="false"/>
 CHECK-NEXT:       </requestedPrivileges>
 CHECK-NEXT:     </security>
 CHECK-NEXT:   </trustInfo>

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

 REQUIRES: libxml2
 UNSUPPORTED: windows
 
 RUN: llvm-mt /manifest %p/Inputs/test_manifest.manifest /out:%t
 RUN: FileCheck %s --input-file=%t
 
-CHECK:      <?xml version="1.0"?>
+CHECK:      <?xml version="1.0" encoding="UTF-8"?>
 CHECK-NEXT: <assembly xmlns="urn:schemas-microsoft-com:asm.v1">
 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>

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

ExitOnErr(errorCodeToError(sys::Process::GetArgumentVector(
       argv_buf, 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) ||
+          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);
int main(int argc, const char **argv) {
   if (InputArgs.hasArg(OPT_out)) {
     OutputFile = InputArgs.getLastArgValue(OPT_out);
   } else if (InputFiles.size() == 1) {
     OutputFile = InputFiles[0];
   } else {
     reportError("no output file specified");
   }
 
-  WindowsManifestMerger Merger;
+  windows_manifest::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));