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 ↗

(On Diff #107974)

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

MT_String[] = {

70 ↗

(On Diff #107974)

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.

84 ↗

(On Diff #107974)

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

llvm/lib/Support/WindowsManifestMerger.cpp

29 ↗

(On Diff #107974)

Prefix + Message?

34 ↗

(On Diff #107974)

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

84 ↗

(On Diff #107974)

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

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

52 ↗

(On Diff #107988)

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

llvm/lib/Support/WindowsManifestMerger.cpp

34 ↗

(On Diff #107988)

Format

99 ↗

(On Diff #107988)

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

99 ↗

(On Diff #107988)

Reverse the condition so that you can continue early.

110–111 ↗

(On Diff #107988)

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

126 ↗

(On Diff #107988)

Format

[ecbeckmann]

Take away extraneous error constructor. Some format changes.

[ecbeckmann]

llvm/lib/Support/WindowsManifestMerger.cpp

99 ↗

(On Diff #107988)

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

126 ↗

(On Diff #107988)

where is format error here?

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

33 ↗

(On Diff #107995)

I think you can remove explicit instantiation of StringRef.

99 ↗

(On Diff #107988)

Is it done?

126 ↗

(On Diff #107988)

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

99 ↗

(On Diff #107988)

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

[ecbeckmann]

Remove logging.

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

39 ↗

(On Diff #108136)

has -> get

51 ↗

(On Diff #108136)

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

77–79 ↗

(On Diff #108136)

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

106 ↗

(On Diff #108136)

Remove Twine from the second expression

113 ↗

(On Diff #108136)

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

114 ↗

(On Diff #108136)

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

143–145 ↗

(On Diff #108136)

Fix style

148–149 ↗

(On Diff #108136)

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 ↗

(On Diff #108136)

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.

148–149 ↗

(On Diff #108136)

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

148–149 ↗

(On Diff #108136)

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

19–20 ↗

(On Diff #108155)

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.

[ecbeckmann]

add string compare helper function.

llvm/lib/Support/WindowsManifestMerger.cpp

19–20 ↗

(On Diff #108155)

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

8–39 ↗

(On Diff #108170)

Please indent the expected result for the sake of readability.

[ecbeckmann]

Indent in test.

llvm/trunk/include/llvm/Support/WindowsManifestMerger.h

public:
   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/trunk/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; }
 
+static bool xmlStringsEqual(const unsigned char *A, const unsigned char *B) {
+  return strcmp(FROM_XML_CHAR(A), FROM_XML_CHAR(B)) == 0;
+}
+
+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 (xmlStringsEqual(Child->name, 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 (xmlStringsEqual(Attribute->name, 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)) {
+      // Attributes of the same name must also have the same value.  Otherwise
+      // an error is thrown.
+      if (!xmlStringsEqual(OriginalValue, 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;
+  for (XMLNodeImpl Child = AdditionalFirstChild; Child; Child = Child->next) {
+    XMLNodeImpl OriginalChildWithName;
+    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();
+}
+
+void stripCommentsAndText(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);
+    } else {
+      StoreNext.next = Child->next;
+      XMLNodeImpl Remove = Child;
+      Child = &StoreNext;
+      xmlUnlinkNode(Remove);
+      xmlFreeNode(Remove);
+    }
+  }
+}
+
+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);
+  XMLNodeImpl AdditionalRoot = xmlDocGetRootElement(ManifestXML);
+  stripCommentsAndText(AdditionalRoot);
+  if (CombinedDoc == nullptr) {
+    CombinedDoc = ManifestXML;
+  } else {
+    XMLNodeImpl CombinedRoot = xmlDocGetRootElement(CombinedDoc);
+    if (xmlStringsEqual(CombinedRoot->name, AdditionalRoot->name) &&
+        isMergeableElement(AdditionalRoot->name)) {
+      if (auto E = treeMerge(CombinedRoot, AdditionalRoot)) {
+        return E;
+      }
+    } 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/trunk/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>
+<!--Comment will be removed-->
+         <requestedExecutionLevel level="3" admin="false"/>
+      </requestedPrivileges>
+    </security>
+  </trustInfo>
+  <dependency>
+    <dependentAssembly>
+      <assemblyIdentity program="logDisplay"/>
+    </dependentAssembly>
+  </dependency>
+  <compatibility>
+    <dependency>
+      <assemblyIdentity program="compatibilityCheck2"/>
+    </dependency>
+    <application>
+      <supportedOS Id="BarOS"/>
+    </application>
+  </compatibility>
+</assembly>

llvm/trunk/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/trunk/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/trunk/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/trunk/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/trunk/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) {