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

54

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

45

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;

51

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

99

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

78–81

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

65–67

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

62–63

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

490–497

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

670

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

40–47

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.

51

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.

70

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;
}

80–81

namespaceOverrides(StringRef HRef1, StringRef HRef2)

89–93

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

65–67

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

62–63

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.

490–497

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

65–68

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

501–503

if (foo)
  return true;
return false;

is equivalent to

return foo;

511–512

else after return

[rnk]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

511–512

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

517

ditto, you can return the condition.

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

88–90

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

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

110–114

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.

124–130

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");

151

Remove this blank line.

206

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

223

Ditto

235–236

Ditto

[zturner]

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

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

640–641

!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

640–641

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

65–68

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

88–90

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.

110–114

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

88–90

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

88–90

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

88–90

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

88–90

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

88–90

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

88–90

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

88–90

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

88–90

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

80

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;

84

Coding style issue: element -> Element

91

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

element -> Element

94–96

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.

206

It's not done.

235–236

Not done.

[ecbeckmann]

Shorten overriding namespace function.

[ecbeckmann]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

206

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.

235–236

same

[ecbeckmann]

Remove extra line.

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

206

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

206

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

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

209

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

209

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

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

209

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

94

typo on "recursively"

209

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

209

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

[ruiu]

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

262–264

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

 #if LLVM_LIBXML2_ENABLED
 #include <libxml/xmlreader.h>
 #endif
 
 namespace llvm {
 
 class MemoryBuffer;
 
+namespace windows_manifest {
+
 #if LLVM_LIBXML2_ENABLED
 typedef xmlDocPtr XMLDocumentImpl;
 typedef xmlNodePtr XMLNodeImpl;
+typedef xmlNsPtr XMLNsImpl;
+typedef xmlAttrPtr XMLAttrImpl;
 #else
 typedef void *XMLDocumentImpl;
 typedef void *XMLNodeImpl;
+typedef void *XMLNsImpl;
+typedef void *XMLAttrImpl;
 #endif
 
 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();
 
   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;
 };
 
+} // namespace windows_manifest
 } // namespace llvm
 #endif

llvm/lib/WindowsManifest/WindowsManifestMerger.cpp

 #include "llvm/Support/MemoryBuffer.h"
 
 #include <stdarg.h>
 
 #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; }
 
 #if LLVM_LIBXML2_ENABLED
+
+static const std::pair<std::string, std::string>
+    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))
       return true;
   }
   return false;
 }
 
-XMLNodeImpl getChildWithName(XMLNodeImpl Parent,
+static XMLNodeImpl getChildWithName(XMLNodeImpl Parent,
                              const unsigned char *ElementName) {
-#if LLVM_LIBXML2_ENABLED
   for (XMLNodeImpl 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 XMLAttrImpl getAttribute(XMLNodeImpl Node, const unsigned char *AttributeName) {
   for (xmlAttrPtr Attribute = Node->properties; Attribute != nullptr;
        Attribute = Attribute->next) {
     if (xmlStringsEqual(Attribute->name, AttributeName))
-      return Attribute->children->content;
+      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 = std::find_if(
+      std::begin(MtNsHrefsPrefixes), std::end(MtNsHrefsPrefixes),
+      [HRef1](const std::pair<std::string, std::string> &element) {
+        return xmlStringsEqual(HRef1, TO_XML_CHAR(element.first.c_str()));
+      });
+  if (HRef1Position == std::end(MtNsHrefsPrefixes))
+    return false;
+  auto HRef2Position = std::find_if(
+      std::begin(MtNsHrefsPrefixes), std::end(MtNsHrefsPrefixes),
+      [HRef2](const std::pair<std::string, std::string> &element) {
+        return xmlStringsEqual(HRef2, TO_XML_CHAR(element.first.c_str()));
+      });
+  if (HRef2Position == std::end(MtNsHrefsPrefixes))
+    return true;
+  int HRef1Priority = std::distance(HRef1Position, std::end(MtNsHrefsPrefixes));
+  int HRef2Priority = std::distance(HRef2Position, std::end(MtNsHrefsPrefixes));
+  if (HRef1Priority > HRef2Priority)
+    return true;
+  return false;
+}
+
+// Search for prefix-defined namespace specified by HRef, starting on Node and
+// continuing recurisvely upwards.  Returns the namespace or nullptr if not
+// found.
+static XMLNsImpl search(const unsigned char *HRef, XMLNodeImpl Node) {
+  for (XMLNsImpl 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.c_str()))) {
+      return TO_XML_CHAR(Ns.second.c_str());
+    }
+  }
+  return HRef;
+}
+
+// Search for prefix-defined namespace specified by HRef, starting on Node and
+// continuing recurisvely 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<XMLNsImpl> searchOrDefine(const unsigned char *HRef,
+                                   XMLNodeImpl Node) {
+  XMLNsImpl Def = search(HRef, Node);
+  if (Def)
+    return Def;
+  Def = xmlNewNs(Node, HRef, getPrefixForHref(HRef));
+  if (!Def)
+    return make_error<WindowsManifestError>("failed to create new namespace");
+  return Def;
+}
+
+// Set the namespace of OrigionalAttribute on OriginalNode to be that of
+// AdditionalAttribute's.
+static Error copyAttributeNamespace(XMLAttrImpl OriginalAttribute,
+                             XMLNodeImpl OriginalNode,
+                             XMLAttrImpl AdditionalAttribute) {
+
+  Expected<XMLNsImpl> 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 XMLNsImpl getNamespaceWithPrefix(const unsigned char *Prefix,
+                                 XMLNodeImpl Node) {
+
+  if (Node == nullptr)
+    return nullptr;
+  for (XMLNsImpl 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 XMLNsImpl getClosestDefault(XMLNodeImpl Node) {
+  if (XMLNsImpl 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(XMLNodeImpl OriginalNode, XMLNodeImpl AdditionalNode) {
+  XMLNsImpl 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;
+        // 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.
+      } else if (namespaceOverrides(OriginalAttribute->ns->href,
+                                    Attribute->ns->href)) {
+        if (!OriginalAttribute->ns->prefix && !Attribute->ns->prefix &&
+            ClosestDefault &&
+            xmlStringsEqual(Attribute->ns->href, ClosestDefault->href)) {
+          if (auto E = copyAttributeNamespace(OriginalAttribute, OriginalNode,
+                                              Attribute))
+            return E;
+        }
+        // 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.
+      } else if (Attribute->ns->prefix || OriginalAttribute->ns->prefix ||
+                 (ClosestDefault &&
+                  !xmlStringsEqual(OriginalAttribute->ns->href,
+                                   ClosestDefault->href))) {
+        if (auto E = copyAttributeNamespace(OriginalAttribute, OriginalNode,
+                                            Attribute))
+          return E;
+      }
+      // 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.
     } 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));
+      XMLAttrImpl NewProp = xmlNewProp(OriginalNode, Attribute->name,
+                                       Attribute->children->content);
+      Expected<XMLNsImpl> ExplicitOrError =
+          searchOrDefine(Attribute->ns->href, OriginalNode);
+      if (!ExplicitOrError)
+        return ExplicitOrError.takeError();
+      NewProp->ns = std::move(ExplicitOrError.get());
     }
   }
-#endif
   return Error::success();
 }
 
-Error treeMerge(XMLNodeImpl OriginalRoot, XMLNodeImpl AdditionalRoot) {
-#if LLVM_LIBXML2_ENABLED
+// Given two nodes, return the one with the higher priority namespace.
+static XMLNodeImpl getDominantNode(XMLNodeImpl Node1, XMLNodeImpl Node2) {
+
+  if (!Node1 || !Node1->ns)
+    return Node2;
+  else if (!Node2 || !Node2->ns)
+    return Node1;
+  else if (namespaceOverrides(Node1->ns->href, Node2->ns->href))
+    return Node1;
+  else
+    return Node2;
+}
+
+// Checks if this Node's namespace is inherited or one it defined itself.
+static bool hasInheritedNs(XMLNodeImpl Node) {
+  if (Node->ns && Node->ns != getNamespaceWithPrefix(Node->ns->prefix, Node))
+    return true;
+  else
+    return false;
+}
+
+// Check if this Node's namespace is a default namespace that it inherited, as
+// opposed to defining itself.
+static bool hasInheritedDefaultNs(XMLNodeImpl Node) {
+  if (hasInheritedNs(Node) && Node->ns->prefix == nullptr)
+    return true;
+  else
+    return false;
+}
+
+// Check if this Node's namespace is a default namespace it defined itself.
+static bool hasDefinedDefaultNamespace(XMLNodeImpl Node) {
+  if (Node->ns && (Node->ns == getNamespaceWithPrefix(nullptr, Node)))
+    return true;
+  else
+    return false;
+}
+
+// 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(XMLNsImpl PrefixDef, XMLNodeImpl 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 (XMLNodeImpl Child = Node->children; Child; Child = Child->next) {
+    explicateNamespace(PrefixDef, Child);
+  }
+}
+
+// Perform the namespace merge between two nodes.
+static Error mergeNamespaces(XMLNodeImpl OriginalNode, XMLNodeImpl AdditionalNode) {
+  // Save the original default namespace definition in case the incoming node
+  // overrides it.
+  const unsigned char *OriginalDefinedDefaultHref = nullptr;
+  if (XMLNsImpl 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 (XMLNsImpl Def = AdditionalNode->nsDef; Def; Def = Def->next) {
+    if (XMLNsImpl 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 {
+      XMLNsImpl NewDef = xmlCopyNamespace(Def);
+      NewDef->next = OriginalNode->nsDef;
+      OriginalNode->nsDef = NewDef;
+    }
+  }
+
+  // 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.
+  XMLNodeImpl DominantNode = getDominantNode(OriginalNode, AdditionalNode);
+  XMLNodeImpl NonDominantNode =
+      DominantNode == OriginalNode ? AdditionalNode : OriginalNode;
+  if (DominantNode == OriginalNode) {
+    if (OriginalDefinedDefaultHref) {
+      XMLNsImpl 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<XMLNsImpl> EC =
+            searchOrDefine(OriginalDefinedDefaultHref, DominantNode);
+        if (!EC)
+          return EC.takeError();
+        XMLNsImpl 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) {
+        XMLNsImpl ClosestDefault = getClosestDefault(DominantNode->parent);
+        Expected<XMLNsImpl> EC =
+            searchOrDefine(ClosestDefault->href, DominantNode);
+        if (!EC)
+          return EC.takeError();
+        XMLNsImpl 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<XMLNsImpl> EC =
+          searchOrDefine(DominantNode->ns->href, NonDominantNode);
+      if (!EC)
+        return EC.takeError();
+      XMLNsImpl 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 (XMLNsImpl 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<XMLNsImpl> EC =
+              searchOrDefine(OriginalDefinedDefaultHref, NonDominantNode);
+          if (!EC)
+            return EC.takeError();
+          XMLNsImpl 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.
+        XMLNsImpl ClosestDefault = getClosestDefault(NonDominantNode);
+        Expected<XMLNsImpl> EC =
+            searchOrDefine(ClosestDefault->href, NonDominantNode);
+        if (!EC)
+          return EC.takeError();
+        XMLNsImpl ExplicitDefault = std::move(EC.get());
+        explicateNamespace(ExplicitDefault, NonDominantNode);
+      }
+    }
+  }
+  if (NewDefinedDefaultHref) {
+    XMLNsImpl OriginalNsDef = getNamespaceWithPrefix(nullptr, OriginalNode);
+    xmlFree(const_cast<unsigned char *>(OriginalNsDef->href));
+    OriginalNsDef->href = NewDefinedDefaultHref;
+  }
+  xmlFree(const_cast<unsigned char *>(OriginalDefinedDefaultHref));
+  return Error::success();
+}
+
+static bool isRecognizedNamespace(const unsigned char *NsHref) {
+  for (auto &Ns : MtNsHrefsPrefixes) {
+    if (xmlStringsEqual(NsHref, TO_XML_CHAR(Ns.first.c_str())))
+      return true;
+  }
+  return false;
+}
+
+static bool hasRecognizedNamespace(XMLNodeImpl Node) {
+  return isRecognizedNamespace(Node->ns->href);
+}
+
+// Ensure a node's inherited namespace is actually defined in the tree it
+// resides in.
+static Error reconcileNamespaces(XMLNodeImpl Node) {
+  if (Node == nullptr)
+    return Error::success();
+  if (hasInheritedNs(Node)) {
+    Expected<XMLNsImpl> ExplicitOrError = searchOrDefine(Node->ns->href, Node);
+    if (!ExplicitOrError)
+      return ExplicitOrError.takeError();
+    XMLNsImpl Explicit = std::move(ExplicitOrError.get());
+    Node->ns = Explicit;
+  }
+  for (XMLNodeImpl 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(XMLNodeImpl OriginalRoot, XMLNodeImpl AdditionalRoot) {
+  if (auto E = mergeAttributes(OriginalRoot, AdditionalRoot))
+    return E;
+  if (auto E = mergeNamespaces(OriginalRoot, AdditionalRoot))
+    return E;
   XMLNodeImpl AdditionalFirstChild = AdditionalRoot->children;
   xmlNode StoreNext;
   for (XMLNodeImpl Child = AdditionalFirstChild; Child; Child = Child->next) {
     XMLNodeImpl 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))
         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(XMLNodeImpl 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);
+    if (!xmlStringsEqual(Child->name, TO_XML_CHAR("comment"))) {
+      stripComments(Child);
     } else {
       StoreNext.next = Child->next;
       XMLNodeImpl Remove = Child;
       Child = &StoreNext;
       xmlUnlinkNode(Remove);
       xmlFreeNode(Remove);
     }
   }
-#endif
+}
+
+// The 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(XMLNodeImpl Node) {
+  for (XMLAttrImpl Attribute = Node->properties; Attribute;
+       Attribute = Attribute->next) {
+    if (!Attribute->ns) {
+      Attribute->ns = getClosestDefault(Node);
+    }
+  }
+  for (XMLNodeImpl Child = Node->children; Child; Child = Child->next) {
+    setAttributeNamespaces(Child);
+  }
+}
+
+// The merging process may create too many prefix defined namespaces.  This
+// function removes all unnecessary ones from the tree.
+static void checkAndStripPrefixes(XMLNodeImpl Node,
+                           std::vector<XMLNsImpl> &RequiredPrefixes) {
+  for (XMLNodeImpl Child = Node->children; Child; Child = Child->next) {
+    checkAndStripPrefixes(Child, RequiredPrefixes);
+  }
+  if (Node->ns && Node->ns->prefix != nullptr) {
+    XMLNsImpl ClosestDefault = getClosestDefault(Node);
+    if (ClosestDefault &&
+        xmlStringsEqual(ClosestDefault->href, Node->ns->href)) {
+      Node->ns = ClosestDefault;
+    } else if (std::find(RequiredPrefixes.begin(), RequiredPrefixes.end(),
+                         Node->ns) == RequiredPrefixes.end()) {
+      RequiredPrefixes.push_back(Node->ns);
+    }
+  }
+  for (XMLAttrImpl Attribute = Node->properties; Attribute;
+       Attribute = Attribute->next) {
+    if (Attribute->ns && Attribute->ns->prefix != nullptr) {
+      XMLNsImpl ClosestDefault = getClosestDefault(Node);
+      if (ClosestDefault &&
+          xmlStringsEqual(ClosestDefault->href, Attribute->ns->href)) {
+        Attribute->ns = ClosestDefault;
+      } else if (std::find(RequiredPrefixes.begin(), RequiredPrefixes.end(),
+                           Node->ns) == RequiredPrefixes.end()) {
+        RequiredPrefixes.push_back(Attribute->ns);
+      }
+    }
+  }
+  XMLNsImpl Prev;
+  xmlNs Temp;
+  for (XMLNsImpl Def = Node->nsDef; Def; Def = Def->next) {
+    if (Def->prefix &&
+        (std::find(RequiredPrefixes.begin(), RequiredPrefixes.end(), Def) ==
+         RequiredPrefixes.end())) {
+      if (Def == Node->nsDef)
+        Node->nsDef = Def->next;
+      else
+        Prev->next = Def->next;
+      Temp.next = Def->next;
+      xmlFreeNs(Def);
+      Def = &Temp;
+    } else {
+      Prev = Def;
+    }
+  }
 }
 
 WindowsManifestMerger::~WindowsManifestMerger() {
-#if LLVM_LIBXML2_ENABLED
   for (auto &Doc : MergedDocs)
     xmlFreeDoc(Doc);
-#endif
 }
 
 Error WindowsManifestMerger::merge(const MemoryBuffer &Manifest) {
-#if LLVM_LIBXML2_ENABLED
   if (Manifest.getBufferSize() == 0)
     return make_error<WindowsManifestError>(
         "attempted to merge empty manifest");
   xmlSetGenericErrorFunc((void *)this, WindowsManifestMerger::errorCallback);
   XMLDocumentImpl 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);
+  stripComments(AdditionalRoot);
+  setAttributeNamespaces(AdditionalRoot);
   if (CombinedDoc == nullptr) {
     CombinedDoc = ManifestXML;
   } else {
     XMLNodeImpl CombinedRoot = xmlDocGetRootElement(CombinedDoc);
     if (xmlStringsEqual(CombinedRoot->name, AdditionalRoot->name) &&
-        isMergeableElement(AdditionalRoot->name)) {
+        isMergeableElement(AdditionalRoot->name) &&
+        hasRecognizedNamespace(AdditionalRoot)) {
       if (auto E = treeMerge(CombinedRoot, AdditionalRoot)) {
         return E;
       }
     } 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
   unsigned char *XmlBuff;
   int BufferSize = 0;
   if (CombinedDoc) {
+    XMLNodeImpl CombinedRoot = xmlDocGetRootElement(CombinedDoc);
+    std::vector<XMLNsImpl> 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));
+  return nullptr;
+}
+
 #else
+
+WindowsManifestMerger::~WindowsManifestMerger() {}
+
+Error WindowsManifestMerger::merge(const MemoryBuffer &Manifest) {
+  return Error::success();
+}
+
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
   return nullptr;
-#endif
 }
 
+#endif 
+
 void WindowsManifestMerger::errorCallback(void *Ctx, const char *Format, ...) {
   auto *Merger = (WindowsManifestMerger *)Ctx;
   Merger->ParseErrorOccurred = true;
 }
 
 Error WindowsManifestMerger::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/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>
 No newline at end of file

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="urn:schemas-microsoft-com:asm.v1" 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));