Page MenuHomePhabricator

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.

Diff Detail

Repository
rL LLVM

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
316 ↗(On Diff #110091)

Can you mark this function static?

352 ↗(On Diff #110091)

Indentation is off here. Is this clang-formatted?

354 ↗(On Diff #110091)

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.

377 ↗(On Diff #110091)

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
317 ↗(On Diff #110091)

Can you make it more lld-ish?

DefaultXml -> Ret

343 ↗(On Diff #110091)

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

354 ↗(On Diff #110091)

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
352 ↗(On Diff #110091)

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
347 ↗(On Diff #111281)

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

362–364 ↗(On Diff #111281)

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

373 ↗(On Diff #111281)

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
373 ↗(On Diff #111281)

now defined right before use

Diff with correct revision.

ruiu added inline comments.Aug 16 2017, 6:19 PM
lld/COFF/DriverUtils.cpp
347–348 ↗(On Diff #111447)

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

414 ↗(On Diff #111447)

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 ↗(On Diff #111447)

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 ↗(On Diff #111452)

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 ↗(On Diff #111452)

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
377 ↗(On Diff #110091)

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 ↗(On Diff #111452)

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

367 ↗(On Diff #111452)

Ditto

368 ↗(On Diff #111452)

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

371 ↗(On Diff #111452)

std::string() -> ""

lld/test/lit.cfg
270 ↗(On Diff #111452)

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

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

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