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

32

Prefix + Message?

33

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

33

Format

98

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

98

Reverse the condition so that you can continue early.

109–110

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

125

Format

[ecbeckmann]

Take away extraneous error constructor. Some format changes.

[ecbeckmann]

llvm/lib/Support/WindowsManifestMerger.cpp

98

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

125

where is format error here?

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

36

I think you can remove explicit instantiation of StringRef.

98

Is it done?

125

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

98

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

[ecbeckmann]

Remove logging.

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

42

has -> get

54

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

80–82

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

109

Remove Twine from the second expression

116

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

117

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

138–158

Fix style

143–144

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

80–82

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.

143–144

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.

143–144

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>
 
+#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 {
 
 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 == FROM_XML_CHAR(ElementName))
+      return true;
+  }
+  return false;
+}
+
+XMLNodeImpl getChildWithName(XMLNodeImpl Parent,
+                             const unsigned char *ElementName) {
+#if LLVM_LIBXML2_ENABLED
+  for (XMLNodeImpl Child = Parent->children; Child; Child = Child->next)
+    if (!strcmp(FROM_XML_CHAR(Child->name), FROM_XML_CHAR(ElementName))) {
+      return Child;
+    }
+#endif
+  return nullptr;
+}
+
+const unsigned char *getAttribute(XMLNodeImpl Node,
+                                  const unsigned char *AttributeName) {
+#if LLVM_LIBXML2_ENABLED
+  for (xmlAttrPtr Attribute = Node->properties; Attribute != nullptr;
+       Attribute = Attribute->next) {
+    if (!strcmp(FROM_XML_CHAR(Attribute->name), FROM_XML_CHAR(AttributeName)))
+      return Attribute->children->content;
+  }
+#endif
+  return nullptr;
+}
+
+Error mergeAttributes(XMLNodeImpl OriginalNode, XMLNodeImpl AdditionalNode) {
+#if LLVM_LIBXML2_ENABLED
+  for (xmlAttrPtr Attribute = AdditionalNode->properties; Attribute != nullptr;
+       Attribute = Attribute->next) {
+    if (const unsigned char *OriginalValue =
+            getAttribute(OriginalNode, Attribute->name)) {
+      if (strcmp(FROM_XML_CHAR(OriginalValue),
+                 FROM_XML_CHAR(Attribute->children->content)))
+        return make_error<WindowsManifestError>(
+            Twine("conflicting attributes for ") +
+            FROM_XML_CHAR(OriginalNode->name));
+    } 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));
+    }
+  }
+#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(FROM_XML_CHAR(Child->name), "text") ||
+          !strcmp(FROM_XML_CHAR(Child->name), "comment"))
+        continue;
+      if (!isMergeableElement(Child->name) ||
+          !(OriginalChildWithName =
+                getChildWithName(OriginalRoot, Child->name))) {
+        XMLNodeImpl NewChild = xmlCopyNode(Child, 1);
+        if (!NewChild)
+          return make_error<WindowsManifestError>(Twine("error when copying ") +
+                                                  FROM_XML_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 ") + FROM_XML_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(FROM_XML_CHAR(CombinedRoot->name),
+                FROM_XML_CHAR(AdditionalRoot->name)) &&
+        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));
+      StringRef(FROM_XML_CHAR(XmlBuff), (size_t)BufferSize));
 #else
   return nullptr;
 #endif
 }
 
 void WindowsManifestMerger::errorCallback(void *Ctx, const char *Format, ...) {
   auto *Merger = (WindowsManifestMerger *)Ctx;
   Merger->ParseErrorOccurred = true;

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