llvm-mt: implement simple merging of manifests, not factoring namespaces.

[ecbeckmann]

Summary

Does a simple merge, where mergeable elements are combined, all others
are appended. Does not apply trickly namespace rules.

[ecbeckmann]

added additional test.

[ecbeckmann]

Format.

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

38

Even if C++11 allows this style, I think inserting = like this is more common.

MT_String[] = {

60

Conditionally defining a dtor seems tricky. It is probably better to define this regardless of the presence of libxml and do nothing if libxml is not available.

72

Likewise, always define this variable and don't use it if libxml is not available.

llvm/lib/Support/WindowsManifestMerger.cpp

29

Prefix + Message?

30

I think a better way of writing this without a global variable is this.

for (StringRef S : {"application",         "assembly",  "assemblyIdentity",
                    "compatibility",       "noInherit", "requestedExecutionLevel",
                    "requestedPrivileges", "security",  "trustInfo"}) {
  if (S == ElementName)
    return true;
}
return false;

[ecbeckmann]

Don't conditionally define destructor, remove global.

llvm/include/llvm/Support/WindowsManifestMerger.h

72

Unfortunately this will cause the sanitizer to complain that "CombinedDoc is never used".

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

52

Why do you need this? Looks like you can just concatenate a string before passing it to the ctor.

llvm/lib/Support/WindowsManifestMerger.cpp

30

Format

95

Do you need this reinterpret_cast? It looks like Child->name is of xmlChar * which is a typedef to unsigned char *.

95

Reverse the condition so that you can continue early.

106–107

else if instead of else { if ... }.

122

Format

[ecbeckmann]

Take away extraneous error constructor. Some format changes.

[ecbeckmann]

llvm/lib/Support/WindowsManifestMerger.cpp

95

unfortunately const char * and const unsigned char * are technically not related types, and must be casted back and forth to each other.

122

where is format error here?

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

33

I think you can remove explicit instantiation of StringRef.

95

Is it done?

122

There wasn't a space between for and (. Now you have it. It is probably worth reviewing your patch yourself on Phabricator after you upload a new one, as I could easily find multiple trivial errors such as indentation errors, and it is easy to find errors on Phab rather than on an editor for some reason. :)

[ecbeckmann]

Early return, remove explicit StringRef

llvm/lib/Support/WindowsManifestMerger.cpp

95

Oh sorry I assumed you meant the isMergeableElement should be checked first.

[ecbeckmann]

Remove logging.

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

39

has -> get

51

Since this function returns not a boolean value but a value itself, get is a better prefix than has.

77–79

The memory you allocate using strdup needs to be freed somewhere using free(). Does your patch do that?

106

Remove Twine from the second expression

113

I don't think you need an explicit instantiation of Twine for the second expression.

114

Add {}.

Usual convention is, if one statement is a block (e.g. enclosed in {})) all statements in the same if ~ else if ~ else ~ should be enclosed with {} as well. E.g. this is fine

if (foo)
  bar;
else
  baz;

and this is also fine

if (foo) {
  bar
} else if (bar) {
  baz
}

but this isn't okay.

if (foo) {
  bar
} else
  baz

146–166

Fix style

151–152

I think we have too many occurrences of this pattern. Can you add a function that compares two xmlStrings?

[ecbeckmann]

Readability improvements.

[ecbeckmann]

Readability improvements.

llvm/lib/Support/WindowsManifestMerger.cpp

77–79

Yes, these strings become part of the node tree structure of the xml document. When xmlFreeDoc() is called by the class destructor these strings should also be freed.

151–152

All the function would do is abstract away the reinterpret_cast. I think with the macro I added for cast it isn't that bad.

[ecbeckmann]

Strip all comment and text nodes, not just in elements being merged.

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

19–20

In C++ it is not a very good practice to use a C macro where you can avoid it. I'd probably keep the original reinterpret_casts, but if you like this, I think I'm fine with it.

151–152

What I was suggesting is adding something like this

static bool streq(const unsigned char *A, const unsigned char B) {
  return strcmp((char *)A, (char *)B) == 0;
}

[ecbeckmann]

add string compare helper function.

llvm/lib/Support/WindowsManifestMerger.cpp

19–20

I really prefer this, having the entire reinterpret_cast written out every time clutters the code IMO

[ruiu]

LGTM

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

9–40

Please indent the expected result for the sake of readability.

[ecbeckmann]

Indent in test.

llvm/include/llvm/Support/WindowsManifestMerger.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
 
 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
-  XMLNodeImpl CombinedRoot = nullptr;
+  XMLDocumentImpl CombinedDoc = nullptr;
+  std::vector<XMLDocumentImpl> MergedDocs;
 #endif
   bool ParseErrorOccurred = false;
 };
 
 } // namespace llvm
 #endif

llvm/lib/Support/WindowsManifestMerger.cpp

 //
 //===---------------------------------------------------------------------===//
 
 #include "llvm/Support/WindowsManifestMerger.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 #include <stdarg.h>
 
+using namespace llvm;
+
 namespace llvm {
 
 char WindowsManifestError::ID = 0;
 
 WindowsManifestError::WindowsManifestError(const Twine &Msg) : Msg(Msg.str()) {}
 
 void WindowsManifestError::log(raw_ostream &OS) const { OS << Msg; }
 
+bool isMergeableElement(const unsigned char *ElementName) {
+  for (StringRef S : {"application", "assembly", "assemblyIdentity",
+                      "compatibility", "noInherit", "requestedExecutionLevel",
+                      "requestedPrivileges", "security", "trustInfo"}) {
+    if (S == reinterpret_cast<const char *>(ElementName))
+      return true;
+  }
+  return false;
+}
+
+XMLNodeImpl hasChildWithName(XMLNodeImpl Parent,
+                             const unsigned char *ElementName) {
+#if LLVM_LIBXML2_ENABLED
+  for (XMLNodeImpl Child = Parent->children; Child; Child = Child->next)
+    if (strcmp(reinterpret_cast<const char *>(Child->name),
+               reinterpret_cast<const char *>(ElementName)) == 0) {
+      return Child;
+    }
+#endif
+  return nullptr;
+}
+
+const unsigned char *hasAttribute(XMLNodeImpl Node,
+                                  const unsigned char *AttributeName) {
+#if LLVM_LIBXML2_ENABLED
+  for (xmlAttrPtr Attribute = Node->properties; Attribute != nullptr;
+       Attribute = Attribute->next) {
+    if (strcmp(reinterpret_cast<const char *>(Attribute->name),
+               reinterpret_cast<const char *>(AttributeName)) == 0)
+      return Attribute->children->content;
+  }
+#endif
+  return nullptr;
+}
+
+Error mergeAttributes(XMLNodeImpl OriginalNode, XMLNodeImpl AdditionalNode) {
+#if LLVM_LIBXML2_ENABLED
+  outs() << "Namespace "
+         << std::string(reinterpret_cast<const char *>(OriginalNode->ns->href))
+         << "\n";
+  for (xmlAttrPtr Attribute = AdditionalNode->properties; Attribute != nullptr;
+       Attribute = Attribute->next) {
+    if (const unsigned char *OriginalValue =
+            hasAttribute(OriginalNode, Attribute->name)) {
+      if (strcmp(reinterpret_cast<const char *>(OriginalValue),
+                 reinterpret_cast<const char *>(
+                     Attribute->children->content)) != 0)
+        return make_error<WindowsManifestError>(
+            Twine("conflicting attributes for ") +
+            Twine(reinterpret_cast<const char *>(OriginalNode->name)));
+    } else {
+      char *NameCopy = strdup(reinterpret_cast<const char *>(Attribute->name));
+      char *ContentCopy =
+          strdup(reinterpret_cast<const char *>(Attribute->children->content));
+      xmlNewProp(OriginalNode,
+                 reinterpret_cast<const unsigned char *>(NameCopy),
+                 reinterpret_cast<const unsigned char *>(ContentCopy));
+    }
+  }
+#endif
+  return Error::success();
+}
+
+Error treeMerge(XMLNodeImpl OriginalRoot, XMLNodeImpl AdditionalRoot) {
+#if LLVM_LIBXML2_ENABLED
+
+  XMLNodeImpl AdditionalFirstChild = AdditionalRoot->children;
+  if (AdditionalFirstChild != nullptr) {
+    for (XMLNodeImpl Child = AdditionalFirstChild; Child; Child = Child->next) {
+      XMLNodeImpl OriginalChildWithName;
+      if (!strcmp(reinterpret_cast<const char *>(Child->name), "text") ||
+          !strcmp(reinterpret_cast<const char *>(Child->name), "comment"))
+        continue;
+      if (!isMergeableElement(Child->name) ||
+          !(OriginalChildWithName =
+                hasChildWithName(OriginalRoot, Child->name))) {
+        XMLNodeImpl NewChild = xmlCopyNode(Child, 1);
+        if (!NewChild)
+          return make_error<WindowsManifestError>(
+              Twine("error when copying ") +
+              Twine(reinterpret_cast<const char *>(Child->name)));
+        if (NewChild->ns)
+          xmlFreeNs(NewChild->ns); // xmlCopyNode explicitly defines default
+                                   // namespace, undo this here.
+        if (!xmlAddChild(OriginalRoot, NewChild))
+          return make_error<WindowsManifestError>(
+              Twine("could not merge ") +
+              Twine(reinterpret_cast<const char *>(NewChild->name)));
+      } else if (auto E = treeMerge(OriginalChildWithName, Child))
+        return E;
+    }
+  }
+  if (auto E = mergeAttributes(OriginalRoot, AdditionalRoot))
+    return E;
+#endif
+  return Error::success();
+}
+
+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, 0);
+                    "manifest.xml", nullptr, XML_PARSE_NOBLANKS);
   xmlSetGenericErrorFunc(nullptr, nullptr);
   if (auto E = getParseError())
     return E;
-  CombinedRoot = xmlDocGetRootElement(ManifestXML);
+  if (CombinedDoc == nullptr)
+    CombinedDoc = ManifestXML;
+  else {
+    XMLNodeImpl CombinedRoot = xmlDocGetRootElement(CombinedDoc);
+    XMLNodeImpl AdditionalRoot = xmlDocGetRootElement(ManifestXML);
+    if (strcmp(reinterpret_cast<const char *>(CombinedRoot->name),
+               reinterpret_cast<const char *>(AdditionalRoot->name)) == 0 &&
+        isMergeableElement(AdditionalRoot->name)) {
+      if (auto E = treeMerge(CombinedRoot, AdditionalRoot)) {
+        return E;
+      }
+    } else {
+      XMLNodeImpl NewChild = xmlCopyNode(AdditionalRoot, 1);
+      if (!NewChild)
+        return make_error<WindowsManifestError>("could not copy manifest");
+      if (!xmlAddChild(CombinedRoot, NewChild))
+        return make_error<WindowsManifestError>("could not append manifest");
+    }
+  }
+  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 (CombinedRoot) {
+  if (CombinedDoc) {
     std::unique_ptr<xmlDoc> OutputDoc(xmlNewDoc((const unsigned char *)"1.0"));
-    xmlDocSetRootElement(OutputDoc.get(), CombinedRoot);
-    xmlDocDumpMemory(OutputDoc.get(), &XmlBuff, &BufferSize);
+    xmlDocSetRootElement(OutputDoc.get(), xmlDocGetRootElement(CombinedDoc));
+    xmlKeepBlanksDefault(0);
+    xmlDocDumpFormatMemory(OutputDoc.get(), &XmlBuff, &BufferSize, 1);
   }
   if (BufferSize == 0)
     return nullptr;
   return MemoryBuffer::getMemBuffer(
       StringRef(reinterpret_cast<const char *>(XmlBuff), (size_t)BufferSize));
 #else
   return nullptr;
 #endif

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

 <?xml version="1.0"?>
 <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
   <trustInfo>
     <security>
       <requestedPrivileges>
-         <requestedExecutionLevel level="3" uiAccess="1"/>
+         <requestedExecutionLevel level="3" admin="false"/>
       </requestedPrivileges>
     </security>
   </trustInfo>
   <dependency>
     <dependentAssembly>
-      <assemblyIdentity program="displayDriver"/>
+      <assemblyIdentity program="logDisplay"/>
     </dependentAssembly>
   </dependency>
+  <compatibility>
+    <dependency>
+      <assemblyIdentity program="compatibilityCheck2"/>
+    </dependency>
+    <application>
+      <supportedOS Id="BarOS"/>
+    </application>
+  </compatibility>
 </assembly>

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

+<?xml version="1.0"?>
+<assembly xmlns="urn:schemas-microsoft-com:asm.v1">
+  <trustInfo>
+    <security>
+      <requestedPrivileges>
+        <requestedExecutionLevel level="2" uiAccess="1"/>
+      </requestedPrivileges>
+    </security>
+  </trustInfo>
+</assembly>

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

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

llvm/test/tools/llvm-mt/conflicting.test

+REQUIRES: libxml2
+UNSUPPORTED: windows
+
+RUN: not llvm-mt /manifest %p/Inputs/test_manifest.manifest /manifest \
+RUN:   %p/Inputs/conflicting.manifest /out:%t 2>&1 >/dev/null | FileCheck %s
+
+CHECK: llvm-mt error: conflicting attributes for requestedExecutionLevel

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-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>
+CHECK-NEXT: <dependency>
+CHECK-NEXT: <dependentAssembly>
+CHECK-NEXT: <assemblyIdentity program="displayDriver"/>
+CHECK-NEXT: </dependentAssembly>
+CHECK-NEXT: </dependency>
+CHECK-NEXT: <compatibility>
+CHECK-NEXT: <dependency>
+CHECK-NEXT: <assemblyIdentity program="compatibilityCheck1"/>
+CHECK-NEXT: </dependency>
+CHECK-NEXT: <application>
+CHECK-NEXT: <supportedOS Id="FooOS"/>
+CHECK-NEXT: <supportedOS Id="BarOS"/>
+CHECK-NEXT: </application>
+CHECK-NEXT: <dependency>
+CHECK-NEXT: <assemblyIdentity program="compatibilityCheck2"/>
+CHECK-NEXT: </dependency>
+CHECK-NEXT: </compatibility>
+CHECK-NEXT: <dependency>
+CHECK-NEXT: <dependentAssembly>
+CHECK-NEXT: <assemblyIdentity program="logDisplay"/>
+CHECK-NEXT: </dependentAssembly>
+CHECK-NEXT: </dependency>
+CHECK-NEXT: </assembly>

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

int main(int argc, const char **argv) {
   PrettyStackTraceProgram X(argc, argv);
 
   ExitOnErr.setBanner("llvm-mt: ");
 
   SmallVector<const char *, 256> argv_buf;
   SpecificBumpPtrAllocator<char> ArgAllocator;
   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) {