Page MenuHomePhabricator

[ThinLto] Fix Ifunc symbol usage
Needs ReviewPublic

Authored by yota9 on Jun 29 2020, 2:33 AM.

Details

Summary

This patch fixes undefined reference problem when using ifunc symbols with
thinLTO.

The bug link: https://bugs.llvm.org/show_bug.cgi?id=46488#
This patch adds ifunc symbol to the global value summary. The ifunc
summary contains link to its resolver.

The llvm-dis results of the bug example:

Before patch:
^1 = gv: (name: "called", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0)))) ; guid = 9741048783615374159
^2 = gv: (name: "resolver", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0), refs: (^1)))) ; guid = 18291748799076262136

After patch:
^1 = gv: (name: "foo", summaries: (ifunc: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), resolver: ^3))) ; guid = 6699318081062747564
^2 = gv: (name: "called", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0)))) ; guid = 9741048783615374159
^3 = gv: (name: "resolver", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0), refs: (^2)))) ; guid = 18291748799076262136

Vladislav
Advanced Software Technology Lab, Huawei

Diff Detail

Event Timeline

yota9 created this revision.Jun 29 2020, 2:33 AM

Yikes, I guess ifunc weren't used very extensively before if we have been dropping them on the floor with thinlto all this time!

There are some uses of alias summaries other places (e.g. in ModuleSummaryIndex.cpp for exporting to a dot file, computing the cache key in LTO.cpp) that you'll want to adjust as well. There's some other uses of AliasSummary that may need similar handling as well that I'm not sure about offhand. E.g. see the uses of AliasSummary in LTO.cpp where we decide whether we can adjust the linkage to AvailableExternally. Also see my comments on the def of GlobalValueSummary::getBaseObject and one of its uses in FunctionImport.cpp since it is not being handled there. If we do decide to try to import ifuncs and resolvers, you'll need to think about how this should work when we do the importing. For aliases, we import the aliasee as a copy (see replaceAliasWithAliasee in FunctionImport.cpp). I'm not familiar enough with ifunc to know how it should be handled when importing.

Also, since most of the handling here for ifunc summary is essentially an exact duplicate of the equivalent alias summary handling, I'm wondering if we can just generalize the AliasSummary to be a more general GlobalIndirectionSummary? Perhaps we could distinguish between the two types as needed with different SummaryKind?

Before changing the patch significantly though, I think it would be good to decide how these should be handled for other types of ThinLTO optimizations as mentioned above (e.g. importing and weak symbol resolution). I'm fine with treating them conservatively (e.g. not importing), but right now as noted above I think we might get runtime failures when these are seen during importing analysis.

llvm/include/llvm/IR/ModuleSummaryIndex.h
364–365

Should be same for ifunc kind

531

Should this (and the method below) return the resolver for IfuncSummary? See my comment in FunctionImport.cpp in one of the callsites for what I think will happen currently.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
358

"aliasee or resolver"?

359

Missing space between first 2 words.

llvm/lib/Transforms/IPO/FunctionImport.cpp
215

With the current patch this will fail if the callee is an IfuncSummary, since getBaseObject on the GlobalValueSummary only handles alias summaries.

yota9 updated this revision to Diff 279766.Jul 22 2020, 4:41 AM
yota9 marked 7 inline comments as done.

Fixed rev 1 comments

yota9 added a comment.Jul 22 2020, 4:52 AM

Dear tejohnson !
Thank you for you comments!
I've also added the same conditions for the LTO.cpp, but I'm not sure about symbol linkage resolution in LTO.cpp, since I'm not extremely familiar with LLVM internal linkage types, hope you can help me to investigate this.
Thank you!
Vladislav
Advanced Software Technology Lab, Huawei

llvm/include/llvm/IR/ModuleSummaryIndex.h
364–365

Thank you!

531

Seems to be you're right, thank you!

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
359

Thank you!

yota9 edited the summary of this revision. (Show Details)Jul 22 2020, 4:55 AM
yota9 added a comment.Aug 9 2020, 5:12 AM

gentle ping :)
Dear @tejohnson maybe you can help/suggest something about this patch? Yes, theoretically it is not a big deal to combine both ifunc & alias cases, but I'm not sure if this won't give any problems in the future.. Thank you!

Sorry for the delayed review!

Looking at the code here, all of the handling for ifuncs is essentially now combined with or a duplicate of the equivalent handling for alias summary, I'm more convinced that the summary hierarchy should be changed to mirror the symbol hierarchy. i.e. a GlobalIndirectSummary with derived AliasSummary and IfuncSummary. Then the vast majority of the code modified here could be simply generalized to a single handling of GlobalIndirectSummary and its getBaseObject. BTW, there is still some missing handling for importing, see specifics further below.

However - before you try this I would recommend creating a more robust set of tests. The only test added here now simply tests the serialization/deserialization of the new summary to bitcode and llvm assembly. Tests are needed that actually invoke the thin link, e.g. via either llvm-lto or llvm-lto2, see tests in llvm/test/ThinLTO/X86/ for examples. The alias_import.ll and alias_resolution.ll tests there are good to model new ifunc tests on.

One of the tests should check the case where one module references an external symbol defined as an ifunc in the other module, to test the handling where we try to import it. AFAICT, the code in FunctionImport.cpp in computeImportForFunction with the comment '"Resolve" the summary', which gets the base object, will attempt to import an ifunc resolver. However, the handling in the backend where we actually do the importing (see importFunctions in the same file) will completely ignore any ifunc that we decided to import. We should either (conservatively for now) prevent importing of ifunc during the thin link (computeImportForFunction), or handle ifunc appropriately in importFunctions (not sure what the appropriate handling is - for alias we import as a copy of the aliasee, does that even make sense for ifunc? If not, probably the conservative handling is best).

llvm/lib/IR/Verifier.cpp
383

The changes in this file seem unrelated to ThinLTO. Can you split into another patch (with test)?