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[] = {

76

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.

90

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

llvm/lib/Support/WindowsManifestMerger.cpp

29

Prefix + Message?

34

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

90

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

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

67

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

Format

99

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

99

Reverse the condition so that you can continue early.

110–111

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

126

Format

[ecbeckmann]

Take away extraneous error constructor. Some format changes.

[ecbeckmann]

llvm/lib/Support/WindowsManifestMerger.cpp

99

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

126

where is format error here?

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

37

I think you can remove explicit instantiation of StringRef.

99

Is it done?

126

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

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

[ecbeckmann]

Remove logging.

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

43

has -> get

55

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

81–83

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

110

Remove Twine from the second expression

117

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

118

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

148–164

Fix style

153–154

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

81–83

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.

153–154

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.

153–154

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 {
 
+static const char *const MT_Strings[]{
+    "application",
+    "assembly",
+    "assemblyIdentity",
+    "compatibility",
+    "noInherit",
+    "requestedExecutionLevel",
+    "requestedPrivileges",
+    "security",
+    "trustInfo",
+};
+
+static const std::vector<std::string> MTMergeableElements(MT_Strings,
+                                                          std::end(MT_Strings));
+
 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);
+  WindowsManifestError(const Twine &Prefix, const Twine &Message);
   void log(raw_ostream &OS) const override;
 
 private:
   std::string Msg;
 };
 
 class WindowsManifestMerger {
 public:
+#if LLVM_LIBXML2_ENABLED
+  ~WindowsManifestMerger();
+#endif
+
   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;
 #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()) {}
 
+WindowsManifestError::WindowsManifestError(const Twine &Prefix,
+                                           const Twine &Message)
+    : Msg((Prefix.concat(Message)).str()) {}
+
 void WindowsManifestError::log(raw_ostream &OS) const { OS << Msg; }
 
+bool isMergeableElement(const unsigned char *ElementName) {
+  bool result =
+      std::find(MTMergeableElements.begin(), MTMergeableElements.end(),
+                std::string(reinterpret_cast<const char *>(ElementName))) !=
+      MTMergeableElements.end();
+  return result;
+}
+
+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
+  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>(
+            "conflicting attributes for ",
+            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));
+      // errs() << std::string(NameCopy) << "\n";
+      xmlNewProp(OriginalNode, reinterpret_cast<const unsigned char*>(NameCopy), reinterpret_cast<const unsigned char*>(ContentCopy));
+      // XMLNodeImpl NewChild = xmlCopyNode(OriginalNode, 1);
+      // if(NewChild->ns)
+      //   xmlFreeNs(NewChild->ns);
+      // xmlNewProp(NewChild, reinterpret_cast<const unsigned char*>(NameCopy), reinterpret_cast<const unsigned char*>(ContentCopy));
+      //xmlAddChild(OriginalNode, NewChild);
+    }
+    }
+#endif
+  return Error::success();
+}
+
+Error treeMerge(XMLNodeImpl OriginalRoot, XMLNodeImpl AdditionalRoot) {
+#if LLVM_LIBXML2_ENABLED
+  XMLNodeImpl AdditionalFirstChild = AdditionalRoot->children;
+  if (AdditionalFirstChild != nullptr) {
+    //xmlNode Temp;
+    for (XMLNodeImpl Child = AdditionalFirstChild; Child; Child = Child->next) {
+      XMLNodeImpl OriginalChildWithName;
+      if ((strcmp(reinterpret_cast<const char *>(Child->name), "text") != 0) &&
+          (strcmp(reinterpret_cast<const char *>(Child->name), "comment") !=
+           0)) {
+        if (!(OriginalChildWithName =
+                  hasChildWithName(OriginalRoot, Child->name)) ||
+            !isMergeableElement(Child->name)) {
+          XMLNodeImpl NewChild = xmlCopyNode(Child, 1);
+          if(NewChild->ns)
+            xmlFreeNs(NewChild->ns); //xmlCopyNode explicitly defines default namespace, undo this here.
+          xmlAddChild(OriginalRoot, NewChild);
+          // The child newly added into the combined tree still has a reference
+          // to all its siblings.  We must unassign the reference and then set
+          // our walking through pointer to travel to the next sibling on the
+          // next loop.
+          // Temp.next = Child->next;
+          // Child->next = nullptr;
+          // Child = &Temp;
+        } else {
+          if (auto E = treeMerge(OriginalChildWithName, Child))
+            return E;
+        }
+      }
+    }
+  }
+  if (auto E = mergeAttributes(OriginalRoot, AdditionalRoot))
+   return E;
+  xmlTextMerge(OriginalRoot, AdditionalRoot);
+#endif
+  return Error::success();
+}
+
+#if LLVM_LIBXML2_ENABLED
+WindowsManifestMerger::~WindowsManifestMerger() {
+  xmlFreeDoc(CombinedDoc);
+}
+#endif
+
 Error WindowsManifestMerger::merge(const MemoryBuffer &Manifest) {
 #if LLVM_LIBXML2_ENABLED
   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)) {
+        xmlFreeDoc(ManifestXML);
+        return E;
+      }
+    }
+    else {
+      XMLNodeImpl NewChild = xmlCopyNode(AdditionalRoot, 1);
+      xmlAddChild(CombinedRoot, NewChild);
+    }
+  }
 #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);
+    errs() << "about to dump memory\n";
+    xmlDocDumpFormatMemory(OutputDoc.get(), &XmlBuff, &BufferSize, 1);
+    errs() << "dumped\n";
   }
   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) {