[PATCH] D82883: [LLD][COFF] Deduplicate .pdata entries

[Alexandre Ganea via Phabricator via llvm-commits]

aganea marked an inline comment as done.
aganea added inline comments.


================
Comment at: lld/COFF/Writer.cpp:1869
+    // the PE image, but that should be fine.
+    memset(end - removed, 0, removed * sizeof(Entry));
+
----------------
mstorsjo wrote:
> aganea wrote:
> > mstorsjo wrote:
> > > Would it be possible to adjust the virtual size of the .pdata section as well, to exclude the pruned bits? I could imagine that some runtime introspection tools locate the section directly instead of using the data directory.
> > Unfortunately the .pdata is merged into .rdata, there're other things following the merged .pdata in the output, and at this point all the RVAs have been calculated and sections already merged & written.
> > 
> > I've tried pruning earlier, but it's a tail biting snake. We need the entire merged, reallocated & sorted .pdata stream, and the computed RVAs, but we don't have that unless all sections have been written. But once they're written, we can't prune them, unless we would re-write again.
> > 
> > If we did a two-step "writeSection" and perhaps computed everything in advance, before writing, it could maybe be possible to resize the output section, but that would involve some sizeable refactoring. I figured it wasn't worth it, but if anyone has a different view on this, please let me know.
> Right, I agree it's probably a sensible tradeoff to keep it like this - it's probably not worth rearchitecting things just to be able to prune it better.
> 
> To see that I understand the issue correctly: Before we've fixed the layout and set RVAs, the .pdata section is unsorted. At that stage, we should still be able to check which pdata entries actually point to the same symbol (although it might be messy, having to dereference the relocations?) - but without sorting it, a duplicate check would end up rather costly - and without RVAs we can't really sort it. Am I understanding the situation correctly?
> 
> Given the (presumed) small extent of the issue, this solution does sound like a good tradeoff.
> 
> Also, right, if merging .pdata into another section, there's no need to update section sizes (and in that case, a few noop bytes in the middle of the .rdata are just orphaned bytes anyway).
> 
> IIRC with mingw setups, .pdata doesn't normally end up merged though. Is it (within reasonable effort) possible to check if `.pdata` is left unmerged, and in that case fix the section size? Or is the association from the enclosing section lost earlier at some point? If not, then this is clearly good enough.
Yes, the description of the situation is correct. I think all this would potentially wouldn't be a problem if /Gy was used for compiling (which is not the case here), then perhaps de-dup would be done correctly. However even at that I'm not sure, it seems the de-dup in ICF.cpp is done by hashing the chunk content, which doesn't work in the case of .pdata. Even if the realloc was applied and RVA computed, the ICF would still need to understand that two different .pdata records (`RUNTIME_FUNCTION`) are equal if they point to two different .xdata records (`UNWIND_INFO`) but with the same content. I've also ran into a peculiar case where two .xdata records were //almost// the same, bearing the fact they were using a  different unwind handler (__CxxFrameHandler4 instead of 3) - however the unwind codes were all the same.

`link.exe` does seems to merge all .pdata records, because we didn't see this issue with the post-link tool I've mentioned (which asserts if two contigous .pdata records point to the same .text function).

As for the output section size, I'll extend the patch with your suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82883