This is an archive of the discontinued LLVM Phabricator instance.

[09/10] [LLD] [COFF] Support merging resource object files
ClosedPublic

Authored by mstorsjo on Aug 27 2019, 12:14 PM.

Details

Summary

Extend WindowsResourceParser to support using a ResourceSectionRef for loading resources from an object file.

@thakis - do you want to keep the error on multiple resource object files if not in mingw mode, or just do the merging if needed? This patch so far keeps that error unless the mingw flag is set.

If there only is one resource object file and no .res resources, don't mangle the .rsrc section, but just link it in without inspecting it. This allows users to produce any .rsrc section (outside of what the parser supports), just like before. (I don't have a specific need for this, but it reduces the risk of this new feature.)

Only parse the .rsrc section and recreate the resource tree if needed (more than one resource object file, or one resource object file and one or more .res files).

Separate out the .rsrc section chunks in InputFiles.cpp, and only include them in the list of section chunks to link if we've determined that there only was one single resource object. (We need to keep the rest of those object files, as they can legitimately contain other sections as well, in addition to .rsrc section chunks.)

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 27 2019, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 12:14 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
mstorsjo updated this revision to Diff 217716.Aug 28 2019, 1:37 PM

Factorized error/warning reporting of duplicates between resource files and object files.

mstorsjo updated this revision to Diff 217813.Aug 29 2019, 3:19 AM

Changed new test object files to yaml form, added another test of a mixed object file with both code and resources, merged with another resource object.

I'd vote for not running complicated code unless we have to, so I'd keep this MinGW only.

Do you really need this feature in MinGW mode? IIRC you said this was for some "default res file" feature in gcc which adds a default res which is only supposed to be used if there's no user res file -- which doesn't require any merging.

I'd vote for not running complicated code unless we have to, so I'd keep this MinGW only.

TBH it's not all that complicated, but in any case, none of that code should run in the cases that are currently supported anyway. It's just choosing between if you want to keep the error, to keep matching link.exe, or be more tolerant and support multiple resource objects even for non-MinGW setups.

Do you really need this feature in MinGW mode? IIRC you said this was for some "default res file" feature in gcc which adds a default res which is only supposed to be used if there's no user res file -- which doesn't require any merging.

The user resource object file could be other resources (icons and dialogs and whatnot), where the manifest is supposed to be merged with the rest. Only if the user resources happens to have a manifest should the implicit default one be dropped.

mstorsjo marked an inline comment as done.Aug 29 2019, 1:41 PM
mstorsjo added inline comments.
lld/COFF/Driver.cpp
1012

@thakis - The question is only about whether to keep this explicit error here, or remove the whole condition.

I'd vote for not running complicated code unless we have to, so I'd keep this MinGW only.

TBH it's not all that complicated, but in any case, none of that code should run in the cases that are currently supported anyway. It's just choosing between if you want to keep the error, to keep matching link.exe, or be more tolerant and support multiple resource objects even for non-MinGW setups.

To be honest, I don't have a strong opinion. I can see arguments for both behaviors and I'm not sure which of them are better, assuming we have to have this resource merging code in lld anyways. If there's a way to prevent having to have this merging code and making the error behavior work for mingw, I'd prefer that :)

Do you really need this feature in MinGW mode? IIRC you said this was for some "default res file" feature in gcc which adds a default res which is only supposed to be used if there's no user res file -- which doesn't require any merging.

The user resource object file could be other resources (icons and dialogs and whatnot), where the manifest is supposed to be merged with the rest. Only if the user resources happens to have a manifest should the implicit default one be dropped.

Wouldn't users pass .res files to the linker? If not, why not?

Do you really need this feature in MinGW mode? IIRC you said this was for some "default res file" feature in gcc which adds a default res which is only supposed to be used if there's no user res file -- which doesn't require any merging.

The user resource object file could be other resources (icons and dialogs and whatnot), where the manifest is supposed to be merged with the rest. Only if the user resources happens to have a manifest should the implicit default one be dropped.

Wouldn't users pass .res files to the linker? If not, why not?

GNU ld doesn't support .res files at all, it only supports resource object files, so that's how it's always been done in MinGW project builds. (But these days, GNU ld _does_ have this whole resource object unpacking and merging logic, but still no support for plain .res files.)

thakis accepted this revision.Aug 29 2019, 2:03 PM

lgtm, with a suggestion for an (I think?) missing test:

lld/COFF/Driver.cpp
1012

Ack. I'd keep it, but I don't feel strongly.

lld/test/COFF/combined-resources.test
32

You could add a test where there's a dupe resource spread over two .res / res-.obj files.

This revision is now accepted and ready to land.Aug 29 2019, 2:03 PM
mstorsjo marked 2 inline comments as done.Aug 29 2019, 2:08 PM
mstorsjo added inline comments.
lld/COFF/Driver.cpp
1012

Ok, I can keep it. It allows multiple-resource-objs.test to keep testing that we don't leave the output file behind as well :-)

lld/test/COFF/combined-resources.test
32

Oh, good point, I'll add such a test as well, before committing it.

This revision was automatically updated to reflect the committed changes.

The test mixed-resource-obj.yaml is failing on our ARM/AArch64 buildbot http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/2131 as it doesn't have an X86 target. I think the test is missing a REQUIRES: x86

The test mixed-resource-obj.yaml is failing on our ARM/AArch64 buildbot http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/2131 as it doesn't have an X86 target. I think the test is missing a REQUIRES: x86

Yes, this seems to be the case. Sorry for the breakage, this should be fixed now.