This is an archive of the discontinued LLVM Phabricator instance.

Integrate manifest merging library into LLD.
ClosedPublic

Authored by ecbeckmann on Aug 2 2017, 5:31 PM.

Details

Summary

Now that the llvm-mt manifest merging libraries are complete, we may use them to merge manifests instead of needing to shell out to mt.exe.

Contingent on D36201.

Event Timeline

ecbeckmann created this revision.Aug 2 2017, 5:31 PM
ecbeckmann updated this revision to Diff 110089.Aug 7 2017, 2:29 PM

Shared return.

ecbeckmann updated this revision to Diff 110091.Aug 7 2017, 2:30 PM

diff to correct revision

ecbeckmann edited the summary of this revision. (Show Details)
zturner added inline comments.Aug 9 2017, 8:07 PM
lld/COFF/DriverUtils.cpp
320

Can you mark this function static?

369

Indentation is off here. Is this clang-formatted?

384

I wonder if all this should be an implementation detail of WindowsManifestMerger itself. Then this entire function would jsut become:

WindowsManifestMerger Merger;
for (StringRef Filename : Config->ManifestInput)
  Merger.mergeFromFile(Filename);
return Merger.getMergedOutput();

and the decision about whether to use libxml or shell out to mt.exe would be part of the WindowsManifestMerger class.

405

If we're not on Windows we should emit a warning that the manifest file was ignored since mt will not be available.

ruiu added inline comments.Aug 10 2017, 11:21 AM
lld/COFF/DriverUtils.cpp
321

Can you make it more lld-ish?

DefaultXml -> Ret

347–364

I believe raw_string_ostream is unbuffered, so you don't need this.

384

I agree with Zach. lld shouldn't know whether libxml is available or not. So, #if LLVM_LIBXML2_ENABLED in lld doesn't seem right.

ecbeckmann marked 7 inline comments as done.

some format and style changes, also do not check if libxml2 is installed but rather just check if WindowsManifestMerger is successful or not.

ecbeckmann added inline comments.Aug 15 2017, 3:57 PM
lld/COFF/DriverUtils.cpp
369

hmm seems it wasn't

Diff with the correction revision.

Correct format.

ruiu added inline comments.Aug 16 2017, 3:37 PM
lld/COFF/DriverUtils.cpp
366–367

Separate this function into two functions, createManifestXmlLibxml and createManifestXmlMt, and call them sequentially from createManifestXml.

391–393

Please follow the coding style in this file, so that the new code looks like they existed from beginning. Remove {}.

398

Define OutputBuffer here instead of at the binning of this function, as this is the first line you are using that variable.

ecbeckmann marked 3 inline comments as done.

Separate manifest merge functions, style fix.

ecbeckmann added inline comments.Aug 16 2017, 6:11 PM
lld/COFF/DriverUtils.cpp
398

now defined right before use

Diff with correct revision.

ruiu added inline comments.Aug 16 2017, 6:19 PM
lld/COFF/DriverUtils.cpp
347–365

It is more natural to return a value rather than assigning to a variable passed by reference.

414

I'm a little confused. Is this safe?

I mean, you are returning a pointer to a buffer owned by an object, which is in turn managed by a unique_ptr. But the unique_ptr is destructed as soon as you return from this function. What am I missing?

ecbeckmann marked an inline comment as done.Aug 16 2017, 6:40 PM
ecbeckmann added inline comments.
lld/COFF/DriverUtils.cpp
414

Function returns std::string, not a pointer. std::string copy constructor is implicitly called in the return.

ecbeckmann marked an inline comment as not done.Aug 16 2017, 6:41 PM

Return mem buff pointer instead of pass by reference.

zturner added inline comments.Aug 21 2017, 3:32 PM
lld/test/lit.cfg
270

Can you line-break here? This goes past 80. Also, why are we checking for cvtres instead of mt? The comment makes sense, but why not just check for mt directly?

ecbeckmann added inline comments.Aug 21 2017, 4:43 PM
lld/test/lit.cfg
270

Checking for mt always seems to succeed, there are too many identically named tools I believe. Note that this was the standard check for finding Microsoft utils even before I started making changes, you could search for rc and cvtres but not for mt.

zturner accepted this revision.Aug 21 2017, 4:52 PM
This revision is now accepted and ready to land.Aug 21 2017, 4:52 PM

Instead of returning nullptr when libxml is not installed, it returns an error
which is more descriptive, and also doesn't require code outside the manifest
library to understand the cases for returning error/nullptr.

ecbeckmann marked an inline comment as done.Aug 21 2017, 5:27 PM

Rebase, diff with correction version.

Rebase and diff properly.

Pull out a lot of unnecessary changes.

ecbeckmann added inline comments.Aug 21 2017, 6:50 PM
lld/COFF/DriverUtils.cpp
405

I actually think we should continue to throw an error, because it is never valid to ignore the manifest when producing the executable.

Formatting change

Remove more unnecessary files.

This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Aug 22 2017, 11:14 AM
lld/COFF/DriverUtils.cpp
348

In general, we prefer StringRef over std::string & (or const std::string &). Can you replace?

368

Ditto

369

It is nice to explain why we don't want to execute mt command on non-Windows machines, because this is different from what we usually do (which is to try to execute a command and see if execution succeeded.)

372

std::string() -> ""

lld/test/lit.cfg
270

Yeah, mt is a command to operate magnetic tapes, and even though most Unix machines don't have tape drives anymore, it is still distributed and installed as part of GNU commands by default. So checking existence of cvtres makes sense.

hans added a subscriber: hans.Aug 29 2017, 5:20 PM
hans added inline comments.
lld/trunk/COFF/DriverUtils.cpp
432 ↗(On Diff #112108)

I'm not sure it makes sense to warn here. The user probably doesn't care which way the manifests were merged?

This breaks Chromium's build with LLD on Windows since libxml2 isn't availalbe and we treat warnings as errors. See https://bugs.chromium.org/p/chromium/issues/detail?id=760385

hans added inline comments.Aug 29 2017, 5:22 PM
lld/trunk/COFF/DriverUtils.cpp
432 ↗(On Diff #112108)

I take it back, we don't treat it as an error and that's not why the build is broken.

But I'm still not sure the warning is actually helpful.

orivej added a subscriber: orivej.Sep 14 2017, 12:07 PM

This broke building LLD with an installed LLVM: https://bugs.llvm.org/show_bug.cgi?id=34608