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

437

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

437

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

437

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

[ruiu]

lld/COFF/DriverUtils.cpp

437

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

437

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

435

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

435

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

435

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

435

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

452–457

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

390–393

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

449–451

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

390–393

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

364

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

383

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.

390–392

Ditto

418–419

Is this what clang-format formatted?

[ecbeckmann]

Use stringref

[ecbeckmann]

lld/COFF/DriverUtils.cpp

383

ack

418–419

yes

[ruiu]

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

lld/test/lit.cfg

276

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

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 ↗

(On Diff #113946)

Hmm, it wasn't fixed?

[ruiu]

Fixed the trailing empty line in r312652.

[ecbeckmann]

lld/trunk/test/lit.cfg

276 ↗

(On Diff #113946)

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

276 ↗

(On Diff #113946)

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/COFF/DriverUtils.cpp

OS << "  <dependency>\n"
        << "    </dependentAssembly>\n"
        << "  </dependency>\n";
   }
   OS << "</assembly>\n";
   return Ret;
 }
 
 static Expected<std::unique_ptr<MemoryBuffer>>
 createManifestXmlWithInternalMt(std::string &DefaultXml) {
   std::unique_ptr<MemoryBuffer> DefaultXmlCopy =
       MemoryBuffer::getMemBufferCopy(DefaultXml);
 
   windows_manifest::WindowsManifestMerger Merger;
   if (auto E = Merger.merge(*DefaultXmlCopy.get()))
     return std::move(E);
 
   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);
+      Error Ret = make_error<windows_manifest::WindowsManifestError>(
+          "internal manifest tool failed on file " + Filename + ": " +
+          toString(std::move(E)));
+      return std::move(Ret);
     }
   }
 
   return Merger.getMergedManifest();
 }
 
-static std::unique_ptr<MemoryBuffer>
+static Expected<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");
+    return make_error<windows_manifest::WindowsManifestError>(
+        "external mt.exe is not available because this is not a Windows "
+        "system");
   // 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);
 }
 
 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 =
+  Expected<std::unique_ptr<MemoryBuffer>> InternalOutputBufferOrError =
       createManifestXmlWithInternalMt(DefaultXml);
-  if (OutputBufferOrError)
-    return OutputBufferOrError.get()->getBuffer();
+  if (InternalOutputBufferOrError)
+    return InternalOutputBufferOrError.get()->getBuffer();
   // Using built-in library failed, possibly because libxml2 is not installed.
   // Shell out to mt.exe instead.
-  handleAllErrors(OutputBufferOrError.takeError(),
+  Expected<std::unique_ptr<MemoryBuffer>> ExternalOutputBufferOrError =
+      createManifestXmlWithExternalMt(DefaultXml);
+  if (ExternalOutputBufferOrError) {
+    // Supress internal mt errors when manifest merge succeeds.
+    consumeError(InternalOutputBufferOrError.takeError());
+    return ExternalOutputBufferOrError.get()->getBuffer();
+  }
+
+  // Both attempts to merge manifest have failed.  Display all errors and throw
+  // exception.
+  handleAllErrors(InternalOutputBufferOrError.takeError(),
                   [&](ErrorInfoBase &EIB) {
                     warn("error with internal manifest tool: " + EIB.message());
                   });
-  std::unique_ptr<MemoryBuffer> OutputBuffer;
-  OutputBuffer = createManifestXmlWithExternalMt(DefaultXml);
-  return OutputBuffer->getBuffer();
+  handleAllErrors(ExternalOutputBufferOrError.takeError(),
+                  [&](ErrorInfoBase &EIB) {
+                    warn("error with external manifest tool: " + EIB.message());
+                  });
+  fatal("could not create manifest xml");
 }
 
 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,
       object::WIN_RES_DATA_ALIGNMENT);
   return MemoryBuffer::getNewMemBuffer(ResSize);
 }
 
 static void writeResFileHeader(char *&Buf) {

lld/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: lld-link: warning: error with internal manifest tool: no libxml2
+# CHECK: lld-link: warning: error with external manifest tool: external mt.exe is not available because this is not a Windows system
+# CHECK: error: could not create manifest xml

lld/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 | FileCheck %s
+
+# CHECK-NOT: warning: error with internal manifest tool: no libxml2

lld/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