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

74

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

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

llvm/lib/Support/WindowsManifestMerger.cpp

73

Prefix + Message?

104

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

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

[ruiu]

llvm/include/llvm/Support/WindowsManifestMerger.h

65

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

llvm/lib/Support/WindowsManifestMerger.cpp

104

Format

169

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

169

Reverse the condition so that you can continue early.

180–181

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

196

Format

[ecbeckmann]

Take away extraneous error constructor. Some format changes.

[ecbeckmann]

llvm/lib/Support/WindowsManifestMerger.cpp

169

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

196

where is format error here?

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

107

I think you can remove explicit instantiation of StringRef.

169

Is it done?

196

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

169

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

[ecbeckmann]

Remove logging.

[ruiu]

llvm/lib/Support/WindowsManifestMerger.cpp

113

has -> get

113–117

Fix style

118–119

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

125

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

151–153

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

180

Remove Twine from the second expression

187

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

188

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

[ecbeckmann]

Readability improvements.

[ecbeckmann]

Readability improvements.

llvm/lib/Support/WindowsManifestMerger.cpp

118–119

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.

151–153

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.

[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.

118–119

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",
+  "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:
   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;
 #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) {
+  //outs() << "Check if " << std::string(reinterpret_cast<const char *>(ElementName)) << " is mergeable\n";
+    bool result = std::find(MTMergeableElements.begin(), MTMergeableElements.end(), std::string(reinterpret_cast<const char *>(ElementName))) != MTMergeableElements.end();
+  //outs() << "result = " << result << "\n";
+    return result;
+  }
+
+  XMLNodeImpl hasChildWithName(XMLNodeImpl Parent, const unsigned char* ElementName) {
+  #if defined(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) {
+        outs() << "found child with name " << std::string(reinterpret_cast<const char *>(ElementName)) << "\n";
+        return Child;
+      }
+  #endif
+      return nullptr;
+    }
+
+    const unsigned char* hasAttribute(XMLNodeImpl Node, const unsigned char* AttributeName) {
+  #if defined(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 defined(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
+      xmlSetProp(OriginalNode, Attribute->name, Attribute->children->content);
+    }
+  #endif
+    return Error::success();
+  }
+
+  Error treeMerge(XMLNodeImpl OriginalRoot, XMLNodeImpl AdditionalRoot) {
+  #if defined(LIBXML2_ENABLED)
+    outs() << "treeMerging " << std::string(reinterpret_cast<const char *>(OriginalRoot->name)) << "\n";
+    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)) {
+        outs() << "analyzing " << std::string(reinterpret_cast<const char *>(Child->name)) << "\n";
+        if (!(OriginalChildWithName = hasChildWithName(OriginalRoot, Child->name)) || !isMergeableElement(Child->name)) {
+          outs() << "about to add " << std::string(reinterpret_cast<const char *>(Child->name)) << "\n";
+          xmlAddChild(OriginalRoot, Child);
+          Temp.next = Child->next;
+          Child->next = nullptr;
+          Child = &Temp;
+      //xmlNewTextChild(OriginalRoot, nullptr, reinterpret_cast<const unsigned char *>("text"), reinterpret_cast<const unsigned char *>("\n"));
+        } else {
+          if(auto E = treeMerge(OriginalChildWithName, Child))
+            return E;
+        }
+      }
+    }
+  }
+    if(auto E = mergeAttributes(OriginalRoot, AdditionalRoot))
+     return E;
+    xmlTextMerge(OriginalRoot, AdditionalRoot);
+  #endif
+    outs() << "pop " << std::string(reinterpret_cast<const char *>(OriginalRoot->name)) << "\n";
+    return Error::success();
+  }
+
 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;
+  if (CombinedRoot == nullptr)
   CombinedRoot = xmlDocGetRootElement(ManifestXML);
+    else {
+      return (treeMerge(CombinedRoot, xmlDocGetRootElement(ManifestXML)));
+    }
 #endif
     return Error::success();
   }
 
 std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
 #if LLVM_LIBXML2_ENABLED
   unsigned char *XmlBuff;
   int BufferSize = 0;
   if (CombinedRoot) {
     std::unique_ptr<xmlDoc> OutputDoc(xmlNewDoc((const unsigned char *)"1.0"));
     xmlDocSetRootElement(OutputDoc.get(), CombinedRoot);
-    xmlDocDumpMemory(OutputDoc.get(), &XmlBuff, &BufferSize);
+      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
 }
 
   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/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>
 </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>
 </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: <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) {
if (InputArgs.hasArg(OPT_out)) {
     OutputFile = InputFiles[0];
   } else {
     reportError("no output file specified");
   }
 
   WindowsManifestMerger Merger;
 
   for (const auto &File : InputFiles) {
+    outs() << "FOUND NEW INPUT\n";
     ErrorOr<std::unique_ptr<MemoryBuffer>> ManifestOrErr =
         MemoryBuffer::getFile(File);
     if (!ManifestOrErr)
       reportError(File, ManifestOrErr.getError());
     MemoryBuffer &Manifest = *ManifestOrErr.get();
     error(Merger.merge(Manifest));
   }