Fix crbug 759265 by suppressing llvm mt warnings.

[ecbeckmann]

Summary

Previous would throw warning whenever libxml2 is not installed. Now
only give this warning if merging manifest fails.

[ecbeckmann]

Add -allow-empty

[ecbeckmann]

Remove unnecessary error move assignment.

[hans]

lld/COFF/DriverUtils.cpp

435 ↗

(On Diff #113007)

Can there be other errors than not having libxml2 available? We probably don't want to suppress those.

Can't we tell by some ifdef or something when libxml2 is not available, and simply not try to use the internal tool then?

[ecbeckmann]

lld/COFF/DriverUtils.cpp

435 ↗

(On Diff #113007)

No I believe we still do. The end developer will probably not care much about these errors if the manifest file is still merged in the end. We only really want to display them if the manifest file can't be merged at all, so we want to give a list of what went wrong.

Using an ifdef was considered during the integration but this was decided against. The reason is that it is an implementation detail of the internal library that it uses libxml2. Any code that relies on the library should not have to know about libxml2, so therefore should not need to check if libxml2 is enabled before including it.

[zturner]

lld/COFF/DriverUtils.cpp

435 ↗

(On Diff #113007)

What kind of errors are we talking about where mt could generate errors but manifest merge would still succeed?

[ruiu]

lld/COFF/DriverUtils.cpp

435 ↗

(On Diff #113007)

I believe what we want to do is this:

If the internal mt is present due to lack of libxml:
  If merge fails for some reason:
    Report that error and abort
Otherwise:
  Invoke external mt command, and if it fails, abort

[ecbeckmann]

Throw errors that internal mt has if they are not related to lack of libxml2.

[ecbeckmann]

lld/COFF/DriverUtils.cpp

435 ↗

(On Diff #113007)

I was talking about cases where the internal mt lib could fail, but external mt.exe would succeed. And in this case we can just silently fail because all the user cares about is that the manifests are merged.

I guess this is probably better though. We probably want to be catching bugs with our internal library, so errors should be reported if libxml2 exists.

[ecbeckmann]

Remove unnecessary consume because toString consumes error.

[ecbeckmann]

Remove one more consume.

[hans]

lld/COFF/DriverUtils.cpp

431 ↗

(On Diff #113339)

I suppose this will solve the immediate problem, but it still looks a bit strange.

I'm not very familiar with this code, but don't we know beforehand if the internal mt tool is available and expected to work?

As Rui pointed out, why can't we just do:

If the internal mt is present:
  If merge fails for some reason:
    Report that error and abort
Otherwise:
  Invoke external mt command, and if it fails, abort

[ecbeckmann]

lld/COFF/DriverUtils.cpp

431 ↗

(On Diff #113339)

The only way we can tell if it will work is if we see if libxml2 is enabled, but as I said before that is an implementation detail internal to the library that users should not need to know about. I suppose we can add a cmake variable that has the same value as the libxml2 flag.

[ruiu]

lld/COFF/DriverUtils.cpp

431 ↗

(On Diff #113339)

I don't think whether a library is available or not is an internal detail of the library. It is in fact a very important information that all users of the library would want to know. Why don't you define something like bool windows_manifest::WindowsManifestMerger::isAvailable() which is true if it is available?

[zturner]

We have similar issues in the PDB code, where we might want to use the native pdb reader or the DIA reader. To handle this, we have an enumeration:

/// Specifies which PDB reader implementation is to be used.  Only a value
/// of PDB_ReaderType::DIA is currently supported, but Native is in the works.
enum class PDB_ReaderType {
  DIA = 0,
  Native = 1,
};

Then, when you actually call the function to load the PDB, it looks like this:

  // Create the correct concrete instance type based on the value of Type.
  if (Type == PDB_ReaderType::Native)
    return NativeSession::createFromPdb(Path, Session);

#if LLVM_ENABLE_DIA_SDK
  return DIASession::createFromPdb(Path, Session);
#else
  return make_error<GenericError>("DIA is not installed on the system");
#endif

Then the user could write something like:

if (auto EC = loadDataForPDB(PDB_ReaderType::DIA, Session)) {
  if (auto EC2 = loadDataForPDB(PDB_ReaderType::Native, Session)) {
    return make_error<Foo>("PDB Unsupported");
  }
}

// Now I have either a native session or a DIA session, but I don't know (or care) which.
return Session;

[ecbeckmann]

Add flag for manifest library being enabled.

[ecbeckmann]

In D37240#859371, @zturner wrote:

We have similar issues in the PDB code, where we might want to use the native pdb reader or the DIA reader. To handle this, we have an enumeration:

/// Specifies which PDB reader implementation is to be used.  Only a value
/// of PDB_ReaderType::DIA is currently supported, but Native is in the works.
enum class PDB_ReaderType {
  DIA = 0,
  Native = 1,
};

Then, when you actually call the function to load the PDB, it looks like this:

  // Create the correct concrete instance type based on the value of Type.
  if (Type == PDB_ReaderType::Native)
    return NativeSession::createFromPdb(Path, Session);

#if LLVM_ENABLE_DIA_SDK
  return DIASession::createFromPdb(Path, Session);
#else
  return make_error<GenericError>("DIA is not installed on the system");
#endif

Then the user could write something like:

if (auto EC = loadDataForPDB(PDB_ReaderType::DIA, Session)) {
  if (auto EC2 = loadDataForPDB(PDB_ReaderType::Native, Session)) {
    return make_error<Foo>("PDB Unsupported");
  }
}

// Now I have either a native session or a DIA session, but I don't know (or care) which.
return Session;

This could work, however would require copying the Executor class code or else refactoring it into a library. Does adding a preprocessor flag work?

lld/COFF/DriverUtils.cpp

431 ↗

(On Diff #113339)

I added a preprocessor flag, is that suitable?

[hans]

Seems fine to me. Rui should probably sign off on it, though.

[ruiu]

Looks like this is towards a right direction.

lld/COFF/DriverUtils.cpp

427–432 ↗

(On Diff #113880)

Since createmanifestXmlWith{Internal,External}Mt are lld-specific functions, I wouldn't make them return Expected<T>. I'd handle errors inside these functions and return T instead.

(As to the formatting, please do not indent code for C macros.)

[ecbeckmann]

Sorry, previous patch did not compile and method of assigning config flag was incorrect.

[ecbeckmann]

Have internal lld functions return result directly and handle errors themselves.

[ruiu]

Hmm, now it doesn't look like it is towards a right direction.

As I understand the API of your WindowsManifestMerger class, it provides an empty implementation if libxml2 is not available so any code that uses your WindowsManifestMerger class can compile at least on any condition (though it may raise an error/returns an empty result at runtime if it wasn't compiled with libxml2). This patch uses conditional compilation to avoid compiler errors, which seems a violation of the rule.

[ecbeckmann]

In D37240#861561, @ruiu wrote:

Hmm, now it doesn't look like it is towards a right direction.

As I understand the API of your WindowsManifestMerger class, it provides an empty implementation if libxml2 is not available so any code that uses your WindowsManifestMerger class can compile at least on any condition (though it may raise an error/returns an empty result at runtime if it wasn't compiled with libxml2). This patch uses conditional compilation to avoid compiler errors, which seems a violation of the rule.

The reason why the "createManifestWithTool" functions are conditionally defined is not to avoid a compiler error, rather a compiler warning is given saying that the function is defined but never used.

[ruiu]

The reason why the "createManifestWithTool" functions are conditionally defined is not to avoid a compiler error, rather a compiler warning is given saying that the function is defined but never used.

I think this is a reason I'd completely avoid C macros in lld as a WindowsManifestMerger user.

If your WindowsManifestMerger is designed to provide an empty implementation when libxml2 is not available, then the aim of doing it should be to avoid C macros on the user side of the class. Currently, it is a mix of the two, so it doesn't eliminate a use of C macros on the user side but also provides an empty implementation (which is redundant if everything is conditionally-compiled). Since you took one approach (which is to provide an empty implementation), I think you want to make it usable without C macros.

If you define WindowsManifestMerger::isAvailable() function or something that returns true if libxml2 is available, you won't get a compiler warning.

[ruiu]

As a side note, zlib may or may not be linked to LLVM, and it provides zlib::isAvailable() function, so this approach is not new.

[ecbeckmann]

Make isAvailable() function instead of using flag.

[ruiu]

lld/COFF/DriverUtils.cpp

386–387 ↗

(On Diff #113925)

There are Windows systems that don't have mt.exe. :)

422–424 ↗

(On Diff #113925)

It is more straightforward if you return a string from createManifestXmlWith{Internal,External}Mt.

[ecbeckmann]

return string instead of buffer

[ecbeckmann]

lld/COFF/DriverUtils.cpp

386–387 ↗

(On Diff #113925)

You're right, Executor will cover the failure to launch mt.exe, whatever the context.

[ruiu]

Generally looking good. A few minor comments.

lld/COFF/DriverUtils.cpp

362 ↗

(On Diff #113929)

Can you pass StringRef instead of std::string? If not, can you make it const?

377 ↗

(On Diff #113929)

This is probably a sign that getMergedManifest should return a std::string instead of std::unique_ptr<MemoryBuffer>, but this doesn't have to be addressed in this patch.

380 ↗

(On Diff #113929)

Ditto

406–407 ↗

(On Diff #113929)

Is this what clang-format formatted?

[ecbeckmann]

Use stringref

[ecbeckmann]

lld/COFF/DriverUtils.cpp

377 ↗

(On Diff #113929)

ack

406–407 ↗

(On Diff #113929)

yes

[ruiu]

Please make this last change so that I can sign-off.

lld/test/lit.cfg

276 ↗

(On Diff #113940)

When you see this "No newline at end of file" message on Phab, it means that the last line of your new file does not end with '\n'. It is usually preferred that every line ends with '\n', so please add it here. (It does not mean that I want you to add a blank line at the end of this file. Or, in other words, I'm not asking you to end this file with "\n\n". What I'm saying is to end this file with "\n" which currently is not.)

[ecbeckmann]

lld/test/lit.cfg

276 ↗

(On Diff #113940)

Yes I know, I just forgot to check all the files again this time :P

[ruiu]

LGTM

[ecbeckmann]

Add line to end of file.

[ruiu]

lld/trunk/test/lit.cfg

276

Hmm, it wasn't fixed?

[ruiu]

Fixed the trailing empty line in r312652.

[ecbeckmann]

lld/trunk/test/lit.cfg

276

hmmm I think it was, I just didn't arc diff it.

276

wait I'm not sure? I'm certain I fixed it. Guess it didn't get pushed somehow? IDK

[ruiu]

Instead of adding "\n", you seems to have been added "\n ". So, the file still lacked the last newline character and it also had a redundant empty line at its end.

lld/trunk/COFF/DriverUtils.cpp

 
 #include "Config.h"
 #include "Driver.h"
 #include "Error.h"
 #include "Memory.h"
 #include "Symbols.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSwitch.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/WindowsResource.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
public:
   void add(StringRef S) { Args.push_back(Saver.save(S)); }
   void add(std::string &S) { Args.push_back(Saver.save(S)); }
   void add(Twine S) { Args.push_back(Saver.save(S)); }
   void add(const char *S) { Args.push_back(Saver.save(S)); }
 
   void run() {
     ErrorOr<std::string> ExeOrErr = sys::findProgramByName(Prog);
     if (auto EC = ExeOrErr.getError())
-      fatal(EC, "unable to find " + Prog + " in PATH: ");
+      fatal(EC, "unable to find " + Prog + " in PATH");
     StringRef Exe = Saver.save(*ExeOrErr);
     Args.insert(Args.begin(), Exe);
 
     std::vector<const char *> Vec;
     for (StringRef S : Args)
       Vec.push_back(S.data());
     Vec.push_back(nullptr);
 
OS << "  <dependency>\n"
        << "      <assemblyIdentity " << Config->ManifestDependency << " />\n"
        << "    </dependentAssembly>\n"
        << "  </dependency>\n";
   }
   OS << "</assembly>\n";
   return OS.str();
 }
 
-static Expected<std::unique_ptr<MemoryBuffer>>
-createManifestXmlWithInternalMt(std::string &DefaultXml) {
+static std::string createManifestXmlWithInternalMt(StringRef DefaultXml) {
   std::unique_ptr<MemoryBuffer> DefaultXmlCopy =
       MemoryBuffer::getMemBufferCopy(DefaultXml);
 
   windows_manifest::WindowsManifestMerger Merger;
   if (auto E = Merger.merge(*DefaultXmlCopy.get()))
-    return std::move(E);
+    fatal(E, "internal manifest tool failed on default xml");
 
   for (StringRef Filename : Config->ManifestInput) {
     std::unique_ptr<MemoryBuffer> Manifest =
         check(MemoryBuffer::getFile(Filename));
-    if (auto E = Merger.merge(*Manifest.get())) {
-      warn("internal manifest tool failed on file " + Filename);
-      return std::move(E);
-    }
+    if (auto E = Merger.merge(*Manifest.get()))
+      fatal(E, "internal manifest tool failed on file " + Filename);
   }
 
-  return Merger.getMergedManifest();
+  return Merger.getMergedManifest().get()->getBuffer();
 }
 
-static std::unique_ptr<MemoryBuffer>
-createManifestXmlWithExternalMt(std::string &DefaultXml) {
-  const Triple HostTriple(Triple::normalize(LLVM_HOST_TRIPLE));
-  if (!HostTriple.isOSWindows())
-    fatal("manifest ignored because no external manifest tool available");
+static std::string createManifestXmlWithExternalMt(StringRef DefaultXml) {
   // Create the default manifest file as a temporary file.
   TemporaryFile Default("defaultxml", "manifest");
   std::error_code EC;
   raw_fd_ostream OS(Default.Path, EC, sys::fs::F_Text);
   if (EC)
     fatal(EC, "failed to open " + Default.Path);
   OS << DefaultXml;
   OS.close();
 
   // Merge user-supplied manifests if they are given.  Since libxml2 is not
   // enabled, we must shell out to Microsoft's mt.exe tool.
   TemporaryFile User("user", "manifest");
 
   Executor E("mt.exe");
   E.add("/manifest");
   E.add(Default.Path);
   for (StringRef Filename : Config->ManifestInput) {
     E.add("/manifest");
     E.add(Filename);
   }
   E.add("/nologo");
   E.add("/out:" + StringRef(User.Path));
   E.run();
 
-  return check(MemoryBuffer::getFile(User.Path), "could not open " + User.Path);
+  return check(MemoryBuffer::getFile(User.Path), "could not open " + User.Path)
+      .get()
+      ->getBuffer();
 }
 
 static std::string createManifestXml() {
   std::string DefaultXml = createDefaultXml();
   if (Config->ManifestInput.empty())
     return DefaultXml;
 
-  // If manifest files are supplied by the user using /MANIFESTINPUT
-  // option, we need to merge them with the default manifest. If libxml2
-  // is enabled, we may merge them with LLVM's own library.
-  Expected<std::unique_ptr<MemoryBuffer>> OutputBufferOrError =
-      createManifestXmlWithInternalMt(DefaultXml);
-  if (OutputBufferOrError)
-    return OutputBufferOrError.get()->getBuffer();
-  // Using built-in library failed, possibly because libxml2 is not installed.
-  // Shell out to mt.exe instead.
-  handleAllErrors(OutputBufferOrError.takeError(),
-                  [&](ErrorInfoBase &EIB) {
-                    warn("error with internal manifest tool: " + EIB.message());
-                  });
-  std::unique_ptr<MemoryBuffer> OutputBuffer;
-  OutputBuffer = createManifestXmlWithExternalMt(DefaultXml);
-  return OutputBuffer->getBuffer();
+  if (windows_manifest::isAvailable())
+    return createManifestXmlWithInternalMt(DefaultXml);
+
+  return createManifestXmlWithExternalMt(DefaultXml);
 }
 
 static std::unique_ptr<MemoryBuffer>
 createMemoryBufferForManifestRes(size_t ManifestSize) {
   size_t ResSize = alignTo(
       object::WIN_RES_MAGIC_SIZE + object::WIN_RES_NULL_ENTRY_SIZE +
           sizeof(object::WinResHeaderPrefix) + sizeof(object::WinResIDs) +
           sizeof(object::WinResHeaderSuffix) + ManifestSize,

lld/trunk/test/COFF/manifestinput-error.test

+# UNSUPPORTED: manifest_tool
+# UNSUPPORTED: libxml2
+
+# RUN: yaml2obj %p/Inputs/ret42.yaml > %t.obj
+# RUN: not lld-link /out:%t.exe /entry:main \
+# RUN:   /manifest:embed \
+# RUN:   /manifestuac:"level='requireAdministrator'" \
+# RUN:   /manifestinput:%p/Inputs/manifestinput.test %t.obj 2>&1 | FileCheck %s
+
+# CHECK: error: unable to find mt.exe in PATH: No such file or directory

lld/trunk/test/COFF/manifestinput-nowarning.test

+# UNSUPPORTED: libxml2
+# REQUIRES: manifest_tool
+
+# RUN: yaml2obj %p/Inputs/ret42.yaml > %t.obj
+# RUN: lld-link /out:%t.exe /entry:main \
+# RUN:   /manifest:embed \
+# RUN:   /manifestuac:"level='requireAdministrator'" \
+# RUN:   /manifestinput:%p/Inputs/manifestinput.test %t.obj | \
+# RUN:   FileCheck -allow-empty %s
+
+# CHECK-NOT: warning: error with internal manifest tool: no libxml2

lld/trunk/test/lit.cfg

 config.environment['LLD_VERSION'] = 'LLD 1.0'
 
 # Indirectly check if the mt.exe Microsoft utility exists by searching for
 # cvtres, which always accompanies it.  Alternatively, check if we can use
 # libxml2 to merge manifests.
 if (lit.util.which('cvtres', config.environment['PATH'])) or \
  (config.llvm_libxml2_enabled == "1"):
     config.available_features.add('manifest_tool')
+
+if (config.llvm_libxml2_enabled == "1"):
+    config.available_features.add('libxml2')
     
 No newline at end of file

llvm/trunk/include/llvm/WindowsManifest/WindowsManifestMerger.h

 #include "llvm/Support/Error.h"
 
 namespace llvm {
 
 class MemoryBuffer;
 
 namespace windows_manifest {
 
+bool isAvailable();
+
 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;

llvm/trunk/lib/WindowsManifest/WindowsManifestMerger.cpp

if (!Merged) {
     Buffer.reset(Buff);
   }
 
   return BufferSize ? MemoryBuffer::getMemBufferCopy(StringRef(
                           FROM_XML_CHAR(Buffer.get()), (size_t)BufferSize))
                     : nullptr;
 }
 
+bool windows_manifest::isAvailable() { return true; }
+
 #else
 
 WindowsManifestMerger::WindowsManifestMergerImpl::~WindowsManifestMergerImpl() {
 }
 
 Error WindowsManifestMerger::WindowsManifestMergerImpl::merge(
     const MemoryBuffer &Manifest) {
   return make_error<WindowsManifestError>("no libxml2");
 }
 
 std::unique_ptr<MemoryBuffer>
 WindowsManifestMerger::WindowsManifestMergerImpl::getMergedManifest() {
   return nullptr;
 }
 
+bool windows_manifest::isAvailable() { return false; }
+
 #endif
 
 WindowsManifestMerger::WindowsManifestMerger()
     : Impl(make_unique<WindowsManifestMergerImpl>()) {}
 
 WindowsManifestMerger::~WindowsManifestMerger() {}
 
 Error WindowsManifestMerger::merge(const MemoryBuffer &Manifest) {