[PATCH] D66825: [10/10] [LLD] [COFF] Silently drop a manifest with language 0, if there's another manifest with a nonzero language id

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added a comment.

In D66825#1648507 <https://reviews.llvm.org/D66825#1648507>, @ruiu wrote:

> 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



================
Comment at: llvm/lib/Object/WindowsResource.cpp:254
 
+bool WindowsResourceParser::checkManifestChildren(
+    TreeNode &Node, uint32_t Lang, uint32_t &DataIndex,
----------------
ruiu wrote:
> Can you add a function comment?
Sure


================
Comment at: llvm/lib/Object/WindowsResource.cpp:307
     uint32_t DataIndex = Data.size();
+    if (!Entry.checkTypeString() && Entry.getTypeID() == /* RT_MANIFEST */ 24 &&
+        !Entry.checkNameString() &&
----------------
ruiu wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66825/new/

https://reviews.llvm.org/D66825