Page MenuHomePhabricator

Fix crbug 759265 by suppressing llvm mt warnings.
ClosedPublic

Authored by ecbeckmann on Aug 28 2017, 5:58 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

ecbeckmann created this revision.Aug 28 2017, 5:58 PM

Add -allow-empty

Remove unnecessary error move assignment.

hans added a subscriber: hans.Aug 29 2017, 5:51 PM
hans added inline comments.
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 added inline comments.Aug 29 2017, 6:44 PM
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 added inline comments.Aug 29 2017, 8:23 PM
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 added inline comments.Aug 30 2017, 8:46 AM
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 marked an inline comment as done.

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

ecbeckmann added inline comments.Aug 30 2017, 4:27 PM
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.

Remove unnecessary consume because toString consumes error.

Remove one more consume.

zturner accepted this revision.Aug 30 2017, 5:29 PM
This revision is now accepted and ready to land.Aug 30 2017, 5:29 PM
hans added inline comments.Aug 31 2017, 8:39 AM
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 added inline comments.Sep 1 2017, 2:12 PM
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 added inline comments.Sep 1 2017, 2:15 PM
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?

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;

Add flag for manifest library being enabled.

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 added a comment.Sep 5 2017, 2:42 PM

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

ruiu edited edge metadata.Sep 5 2017, 2:47 PM

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 updated this revision to Diff 113919.Sep 5 2017, 2:55 PM

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

ecbeckmann updated this revision to Diff 113922.Sep 5 2017, 3:05 PM

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

ruiu added a comment.Sep 5 2017, 3:05 PM

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 marked an inline comment as done.Sep 5 2017, 3:06 PM
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 added a comment.Sep 5 2017, 3:21 PM

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 added a comment.Sep 5 2017, 3:25 PM

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 updated this revision to Diff 113925.Sep 5 2017, 3:51 PM

Make isAvailable() function instead of using flag.

ruiu added inline comments.Sep 5 2017, 4:01 PM
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 updated this revision to Diff 113929.Sep 5 2017, 4:14 PM
ecbeckmann marked an inline comment as done.

return string instead of buffer

ecbeckmann added inline comments.Sep 5 2017, 4:15 PM
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 added a comment.Sep 5 2017, 4:19 PM

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 updated this revision to Diff 113940.Sep 5 2017, 5:33 PM
ecbeckmann marked 2 inline comments as done.

Use stringref

ecbeckmann added inline comments.Sep 5 2017, 5:33 PM
lld/COFF/DriverUtils.cpp
377 ↗(On Diff #113929)

ack

406–407 ↗(On Diff #113929)

yes

ruiu added a comment.Sep 5 2017, 5:42 PM

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 added inline comments.Sep 5 2017, 6:25 PM
lld/test/lit.cfg
276 ↗(On Diff #113940)

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

ruiu accepted this revision.Sep 5 2017, 6:27 PM

LGTM

ecbeckmann updated this revision to Diff 113945.Sep 5 2017, 6:49 PM

Add line to end of file.

ecbeckmann marked an inline comment as done.Sep 5 2017, 6:49 PM
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Sep 5 2017, 6:54 PM
lld/trunk/test/lit.cfg
276

Hmm, it wasn't fixed?

ruiu added a comment.Sep 6 2017, 10:43 AM

Fixed the trailing empty line in r312652.

ecbeckmann added inline comments.Sep 6 2017, 11:06 AM
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 added a comment.Sep 6 2017, 11:12 AM

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.