Page MenuHomePhabricator

[10/10] [LLD] [COFF] Implement MinGW default manifest handling
ClosedPublic

Authored by mstorsjo on Tue, Aug 27, 12:15 PM.

Details

Summary

In mingw environments, resources are normally compiled to resource object files directly, instead of letting the linker convert them to COFF format.

Since some time, GCC supports the notion of a default manifest object. When invoking the linker, GCC looks for the default manifest object file, and if found in the expected path, it is added to linker commands.

The default manifest is one that indicates support for the latest known versions of windows, to implicitly unlock the modern behaviours of certain APIs. (No stance taken on whether doing that implicitly is a good thing.)

Not all mingw/gcc distributions include this file, but e.g. in msys2, the default manifest object is distributed in a separate package (which can be but might not always be installed).

This means that even if user projects only use one single resource object file, the linker can end up with two resource object files, and thus needs to support merging them.

The default manifest has a language id of zero, and GNU ld has got logic for dropping a manifest with a zero language id, if there's another manifest present with a nonzero language id. If there are multiple manifests with a nonzero language id, the merging process errors out.

Replicate the same logic from GNU ld in llvm's WindowsResourceParser class. (Should we limit this behaviour to object files, and/or when the mingw flag is used?)

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Tue, Aug 27, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 27, 12:15 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
ruiu added a comment.Wed, Aug 28, 12:56 AM

I think it is fine to make this logic run unconditionally if the logic doesn't harm non-mingw use cases.

llvm/lib/Object/WindowsResource.cpp
254 ↗(On Diff #217473)

Can you add a function comment?

266 ↗(On Diff #217473)

nit: no else after return.

307 ↗(On Diff #217473)

Detecting duplicate manifest objects having the same ID and skipping duplicate zero language IDs while reading an object file seems a bit tricky. Do you think you can do after reading the entire manifest file? If you can, you can separate the logic to parse a file from the logic that you want to add in this patch.

mstorsjo marked 2 inline comments as done.Wed, Aug 28, 3:37 AM

I think it is fine to make this logic run unconditionally if the logic doesn't harm non-mingw use cases.

Well, I guess it shouldn't harm other use cases no, unless someone tries to link .res files containing multiple manifests. As that is invalid anyway (as far as I've understood from the GNU ld source) it shouldn't really matter though, but it's possible to do right now with lld-link and MS tools

llvm/lib/Object/WindowsResource.cpp
254 ↗(On Diff #217473)

Sure

307 ↗(On Diff #217473)

I presume you mean "after reading the entire object file"? Yes it's a bit messy to have it integrated in the parsing/adding, but I chose doing it that way for a couple reasons.

One can almost implement most of the same logic by cleaning up the tree after parsing/adding, but there's two details that work less well that way:

  1. The current code silently ignores two zero-language manifests, while that would trigger a duplicate if the handling was moved to afterwards. This isn't something that should normally happen (the main usecase is user code having a non-zero language manifest and the default manifest object has a zero-language one) though so it's probably not a big issue.
  2. If we first add a zero-language manifest to the resource tree, and later remove it, we'd still have the data for it in the Data vector, which would end up in the binary, unreferenced. But I guess we could remove that entry and iterate over the tree, decrementing DataIndex for any node where it is >= the removed entry.
mstorsjo marked an inline comment as done.Wed, Aug 28, 1:28 PM
mstorsjo added inline comments.
llvm/lib/Object/WindowsResource.cpp
307 ↗(On Diff #217473)

I tried doing this and it probably looks more straightforward than this original version. But I kept a check to allow duplicate entries of manifests with language zero, in case that actually happens, but I tried making it a bit more readable.

mstorsjo updated this revision to Diff 217718.Wed, Aug 28, 1:40 PM

Moved the manifest duplicate cleanup to a separate method which is called after parsing and adding nodes to the tree, allowing sharing most of the code for both input file types. I still kept one check for ignoring duplicates of manifests with language zero in the parse phase though (and added a testcase for it) even though I'm unsure if that's a real issue that would arise.

By restructuring the code, it's much easier to limit most of this manifest handling to mingw mode.

ruiu added a comment.Thu, Aug 29, 1:31 AM

It looks better!

llvm/lib/Object/WindowsResource.cpp
254 ↗(On Diff #217718)

Sorry to be a bit too picky, but I'd write more comments as to what the default manifest, why we may have duplicate default manifests, and why we can safely ignore them if there's any other manifest. I'd also explain that this is for mingw.

282 ↗(On Diff #217718)

This function doesn't have any blank line, so it looks too dense. I'd insert blank lines at the places where it makes sense.

562 ↗(On Diff #217718)

I'd explain what this function is supposed to do -- this function shifts DataIndex of tree nodes to fill a gap created by removing a default language node.

MaskRay added inline comments.
lld/test/COFF/merge-resource-manifest.test
12 ↗(On Diff #217718)

Does yaml2obj lack any features that prevents it from generating manifest-lang0.o and other object files?

mstorsjo marked 4 inline comments as done.Thu, Aug 29, 1:53 AM
mstorsjo added inline comments.
lld/test/COFF/merge-resource-manifest.test
12 ↗(On Diff #217718)

Hmm, no, yaml2obj should work fine.

In principle we could also produce them with llvm-rc and llvm-cvtres, but we currently don't have those as test-depends for lld. (And for patch 09/10 I have object files generated with other tools, to test interoperability with them. But a yaml serialization of it should preserve all the necessary detail.

llvm/lib/Object/WindowsResource.cpp
254 ↗(On Diff #217718)

Ok, will expand the comment. Currently the explanation is mostly in the commit message, but I'll try to make the individual comments also understandable standalone.

282 ↗(On Diff #217718)

Sure, will do.

562 ↗(On Diff #217718)

Ah yes, will add a comment here as well.

mstorsjo marked an inline comment as done.Thu, Aug 29, 2:36 AM
mstorsjo added inline comments.
lld/test/COFF/merge-resource-manifest.test
12 ↗(On Diff #217718)

yaml2obj can't synthesize the .res files though, but if you want to, we could add llvm-rc to the list of test dependencies and create it with that. (We do have a number of existing tests with hardcoded .res and resource object files though.)

mstorsjo updated this revision to Diff 217809.Thu, Aug 29, 3:02 AM
mstorsjo marked 7 inline comments as done.

Changed accoring to review feedback

thakis resigned from this revision.Thu, Aug 29, 3:31 PM

There are enough highly capable reviewers actively looking at this; resigning from this one :)

@ruiu Anything else on this one?

ruiu added a comment.Tue, Sep 3, 2:05 AM

No, but I actually wanted Nico to take a look, as Nico is probably more familiar with this code than me.

No, but I actually wanted Nico to take a look, as Nico is probably more familiar with this code than me.

Ok, fair enough.

@thakis - Your review is (still) wanted here :-)

thakis added inline comments.Tue, Sep 3, 10:30 AM
llvm/include/llvm/Object/WindowsResource.h
160 ↗(On Diff #217809)

nit: cleanUpManifests – the verb is "to clean up" (with "cleanup" is the corresponding noun)

llvm/lib/Object/WindowsResource.cpp
310 ↗(On Diff #217809)

nit: call "shouldIgnoreDuplicate" and make const; as-is the function sounds like it modifies state

319 ↗(On Diff #217809)

same

353 ↗(On Diff #217809)

Do we have to add code for this to the merging logic? Or is the implicit manifest always res obj file 0 and we can skip that if there are more than 1 res obj files?

mstorsjo updated this revision to Diff 218510.Tue, Sep 3, 12:03 PM
mstorsjo marked 3 inline comments as done.
mstorsjo retitled this revision from [10/10] [LLD] [COFF] Silently drop a manifest with language 0, if there's another manifest with a nonzero language id to [10/10] [LLD] [COFF] Implement MinGW default manifest handling.
mstorsjo edited the summary of this revision. (Show Details)

Fixed method naming, made helper const.

mstorsjo marked an inline comment as done.Tue, Sep 3, 12:05 PM
mstorsjo added inline comments.
llvm/lib/Object/WindowsResource.cpp
353 ↗(On Diff #217809)

Yes, we need to handle it on a per-resource basis during/after merging; as GNU ld does support merging resource objs, the user could legitimately be passing multiple resource objects, and we don't know if one of them is user provided or the default manifest (as we don't know if the compiler driver did inject that or not). Additionally, even if there's one user provided resource object and one default manifest, they need to be merged as we don't know if the user provided resource object actually contained a manifest or not.

thakis accepted this revision.Wed, Sep 4, 6:10 AM

Looks horrible, please commit :)

llvm/lib/Object/WindowsResource.cpp
316 ↗(On Diff #218510)

Should these check for mingw? A normal user-provided resource could probably conceivably have language id 0 as well?

This revision is now accepted and ready to land.Wed, Sep 4, 6:10 AM
mstorsjo marked an inline comment as done.Wed, Sep 4, 7:21 AM
mstorsjo added inline comments.
llvm/lib/Object/WindowsResource.cpp
316 ↗(On Diff #218510)

Ideally yes, but we don't have the flag available here. We'd need to add it as parameter to the parse functions, passed down recursively. As the harm from it here is pretty small (only one missed warning/error in an obscure corner case) I thought it wasn't worth the churn. But I can add a parameter if you want to.

thakis added inline comments.Wed, Sep 4, 9:02 AM
llvm/lib/Object/WindowsResource.cpp
316 ↗(On Diff #218510)

Can't you make it a member variable on WindowsResourceParser?

mstorsjo marked an inline comment as done.Wed, Sep 4, 10:13 AM
mstorsjo added inline comments.
llvm/lib/Object/WindowsResource.cpp
316 ↗(On Diff #218510)

Yes, that works and is simpler. I think I'll push it with a flag named mingw; otherwise the correct name would be "ignoreManifestLangZeroDuplicates", which is more than a mouthful...

This revision was automatically updated to reflect the committed changes.