[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 an inline comment as done.
mstorsjo added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66825