[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

Tue Feb 11 11:01:04 PST 2020

tejohnson marked an inline comment as done.
tejohnson added a comment.

In D73242#1861125 <https://reviews.llvm.org/D73242#1861125>, @tejohnson wrote:

> In D73242#1861111 <https://reviews.llvm.org/D73242#1861111>, @thakis wrote:
>
> > This makes lld crash when linking chromium's base_unittests and probably does the same for most other binaries that use both thinlto and cfi: https://bugs.chromium.org/p/chromium/issues/detail?id=1049434
>
>
> Reverted at 25aa2eef993e17708889abf56ed1ffad5074a9f4 <https://reviews.llvm.org/rG25aa2eef993e17708889abf56ed1ffad5074a9f4>. Will investigate using repro @thakis sent off patch.

Recommitted with fix and additional test case at 80d0a137a5aba6998fadb764f1e11cb901aae233 <https://reviews.llvm.org/rG80d0a137a5aba6998fadb764f1e11cb901aae233>.

================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1999
+    WholeProgramDevirtResolution *Res = nullptr;
+    if (ExportSummary && isa<MDString>(S.first.TypeID))
+      // Create the type id summary resolution regardlness of whether we can
----------------
The fix needed for the Chromium issue was to guard the TypeIdSummary creation here by whether the TypeID exists in the TypeIdMap (which makes it match the comment in fact), as we don't want to create a summary if the type id is not used on a global (in which case it should in fact be Unsat). The equivalent code in the index-only WPD is essentially already guarded by that condition, because of the way the CallSlots are created (and in fact there is an assert in that code that we have a use on a vtable, i.e. that a TypeIdCompatibleVtableSummary is found for the TypeID).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242