This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO/WPD] Fix index-based WPD for available_externally vtables
ClosedPublic

Authored by tejohnson on Oct 25 2019, 3:04 PM.

Details

Summary

Clang does not add type metadata to available_externally vtables. When
choosing a summary to look at for virtual function definitions, make
sure we skip summaries for any available externally vtables as they will
not describe any virtual function functions, which are only summarized
in the presence of type metadata on the vtable def. Simply look for the
corresponding strong def's summary.

Diff Detail

Event Timeline

tejohnson created this revision.Oct 25 2019, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2019, 3:04 PM
evgeny777 added inline comments.Oct 26 2019, 9:09 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
845

The code below doesn't go well along with the comment. For example it allows having multiple linkonce_odr/weak_odr summaries and multiple ones with available_externally linkage.

848–849

Can we use single find_if instead of lines 856-871 for the sake of simplicity?

auto I = llvm::find_if(
          P.VTableVI.getSummaryList(),
          [&](const std::unique_ptr<GlobalValueSummary> &Summary) {
            return !GlobalValue::isAvailableExternallyLinkage(Summary->linkage());
          });
tejohnson marked 2 inline comments as done.Oct 28 2019, 7:00 AM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
845

That is the intent of both the code and the comment, which might not be written very clearly. The code previously allowed multiple weak/linkonce odr, and now should also allow for multiple available_externally. Hence the comment "all but one must be available_externally" - because we can have multiple available externally but only one external linkage. Does that clarify things, or do you have alternate wording in mind that could be clearer?

Now that I think about it, this code should probably also proactively guard against the type of situation encountered and handled in D28411 and D67322 of same-named locals in same-named files compiled without enough distinguishing path. In that case we should not assert and just be conservatively correct. The easiest way to do that is to simply return false from this routine if that situation is encountered. I will add that handling.

848–849

Good idea, will change.

evgeny777 added inline comments.Oct 28 2019, 11:06 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
845

Is it allowed having weak_odr + linkonce_odr + available_externally in P.VTableVI.getSummaryList()? If yes (I think it should be) than all but one must be available_externally is misleading. If not the code itself should be changed.

tejohnson marked 2 inline comments as done.Oct 29 2019, 6:29 PM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
845

Yes, that is a valid combination. I have changed the comment and the code. I made the lookup of the first non-available externally into a loop so I can also check for the case of multiple locals vtables with the same guid owing to not enough distinguishing path and same source file name (handled conservatively by returning false), and added a test case.

848–849

Changed as described above - use a loop so I can also look for the clashing local guid case.

tejohnson updated this revision to Diff 227014.Oct 29 2019, 6:30 PM

Update patch per comments.

This revision is now accepted and ready to land.Oct 29 2019, 11:44 PM
This revision was automatically updated to reflect the committed changes.
aganea added a subscriber: aganea.Dec 4 2019, 9:10 AM
aganea added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
874

Hi Theresa!
I'm getting a crash when building Clang with ThinLTO.
The issue seems that VS ends up pointing an AliasSummary * instead of a GlobalVarSummary *, because of the !GlobalValue::isAvailableExternallyLinkage() test above, which leads to a crash when dereferencing the first element of VS->vTableFuncs().begin(), because it doesn't point to the right thing.
I'm at checkout 1cc0ba4 (from yesterday)
Please see the full repro:

tejohnson marked an inline comment as done.Dec 4 2019, 9:24 AM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
874

Thanks for the report and repro. I will take a look today. Conceptually it seems like a simple fix of invoking getBaseObject() on the summary is needed at some point in this loop.

tejohnson marked an inline comment as done.Dec 4 2019, 6:00 PM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
874

Fix sent for review in D71040.