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.Wed, Jul 22, 4:41 AM
yota9 marked 7 inline comments as done.

Fixed rev 1 comments

yota9 added a comment.Wed, Jul 22, 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)Wed, Jul 22, 4:55 AM