This is an archive of the discontinued LLVM Phabricator instance.

[ThinLto] Fix Ifunc symbol usage
AbandonedPublic

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

507–508

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
216

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!

507–508

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 ↗(On Diff #279766)

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

yota9 updated this revision to Diff 298140.Oct 14 2020, 6:34 AM
yota9 marked an inline comment as done.

Create GlobalIndirectValueSummary class
Add ifunc_import test
Skip ifuncs import

yota9 added a comment.Oct 14 2020, 6:41 AM

Dear @tejohnson!
Sorry for the late answer, the task is not prioritized and I don't have much time to finish it :)
Still I've added GlobalIndirectSummary class as you've requested and decided not to import ifuncs for now, since it should be handled another way then aliases (still AFAIU there is not so much sense in it, maybe only if the resolver unconditionally returns only one function, but in other way unlike aliases ifunc is a relocation that should be processed at runtime).
It seems to be ifunc_import test is not needed, but still added it, maybe it will be useful if somebody will import ifuncs in the future..
Thank you so much for your patch review!

llvm/lib/IR/Verifier.cpp
383 ↗(On Diff #279766)

I see, no problems :)

Gentle ping.
Dear @tejohnson if there are some issues that I accidentally missed or not fully understood please let me know about them :)
Thank you for your understanding!

Thanks for doing this and sorry for the slow reply! Some comments and suggested fixes below.

llvm/include/llvm/IR/ModuleSummaryIndex.h
452

Changing Aliasee to Summary here and below is a little confusing, since it is ambiguous what summary you are talking about (the current one, the indirect symbol?). Suggest changing Summary here and in below interfaces to IndirectSymbol. It's a little longer but captures the meaning better (and is equivalent to similar interface naming on the GlobalIndirectSymbol itself).

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
359

s/an alias summary/an indirect value summary/

llvm/lib/AsmParser/LLParser.cpp
8431

Needs equivalent handling for the new IfuncSummary. To test this handling, add an ifunc summary case to llvm/test/Assembler/thinlto-summary.ll. Actually, add two cases, equivalent to the following two alias cases:

; Make this one an alias with a forward reference to aliasee.
^5 = gv: (guid: 4, summaries: (alias: (module: ^0, flags: (linkage: private, notEligibleToImport: 0, live: 0, dsoLocal: 1), aliasee: ^14)))

and

; Alias summary with backwards reference to aliasee.
^18 = gv: (guid: 17, summaries: (alias: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1), aliasee: ^14)))

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
460

Could this just be merged into the above alias handling by casting to GlobalIndirectSymbol?

4136

I don't see the associated post-pass for the Ifuncs. See where the Aliases vector is walked down around line 4251.

llvm/lib/IR/Globals.cpp
169

Could this just be merged into the above alias handling by casting to GlobalIndirectSymbol?

llvm/lib/IR/ModuleSummaryIndex.cpp
592

Cast to GlobalIndirectSymbolSummary so this covers both cases?

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

This is an expensive hash table lookup (related to the below FIXME), which is why we only want to do this when necessary (currently under debug compiler for the debug message below). Suggestion later on why you don't need this.

549

We already have a representative summary in GVSummary.second. However, I don't think you even want to skip ifuncs here. This is just deciding what to import for any functions defined in the current module. What you really want to do is to skip trying to import the ifunc base object on any references made to an ifunc in the function. To do that, you'll want to modify within selectCallee, which is called by computeImportForFunction for everything it calls. Specifically, the code which is around line 215:

auto *Summary = cast<FunctionSummary>(GVSummary->getBaseObject());

You will want to check before calling getBaseObject() if GVSummary is an IfuncSummary and return false from the lambda function so that we don't try to import the ifunc resolver.

After looking at your tests, I'm surprised the new ifunc_import test is working. I.e. this loop should see function main(), which isn't an ifunc, add it, and then compute imports based on all of its calls, which include all of the ifuncs. Ah - I see what is going on, you are checking that the ifuncs themselves are still declarations. But we would import the base object, which is the resolver. That test should probably check whether we are importing the resolver functions or not.

Actually, it shouldn't really be a problem to import them, since we haven't imported the defs of the ifuncs that resolve to them, they should get thrown away since there won't be any references to them after importing. However, it's just wasteful work that can be avoided by the change I suggest above.

llvm/test/Bitcode/thinlto-ifunc.ll
4

You don't need to redo the opt invocation, they are identical. Just use %t.o in the below llvm-dis.

Elvina added a subscriber: Elvina.Nov 11 2020, 6:19 AM
guy-david requested changes to this revision.Jan 9 2021, 3:53 PM

Hey, requesting a pair of changes changes after I ran into alias-to-ifunc issues.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
808–809

This should happen before the alias summary loop in case of an alias to an ifunc scenario.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
3983–3997

Same as before, this should go before the alias summary loop so the bitcode reader would have all the necessary information to parse the alias.

This revision now requires changes to proceed.Jan 9 2021, 3:53 PM
yota9 updated this revision to Diff 334798.Apr 1 2021, 12:43 PM
yota9 marked 12 inline comments as done.
yota9 added a comment.EditedApr 1 2021, 12:45 PM

@tejohnson Thank you for you comments and sorry for so long answer! Looking forward to your review!
@guy-david Thank you, fixed!

yota9 updated this revision to Diff 334826.Apr 1 2021, 2:07 PM
vleonen added a subscriber: vleonen.Apr 2 2021, 1:59 PM

It appears that this one is not fixed yet.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
3983

This loop should happen before emitting the FS_ALIAS records (which is just above it).

yota9 updated this revision to Diff 381389.Oct 21 2021, 2:09 PM
yota9 marked an inline comment as done.
yota9 added inline comments.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
3983

Thank you!

yota9 marked an inline comment as done.Oct 21 2021, 2:11 PM
yota9 updated this revision to Diff 381434.Oct 21 2021, 4:54 PM
yota9 updated this revision to Diff 381437.Oct 21 2021, 5:09 PM
yota9 added a comment.Oct 23 2021, 6:10 AM

Hello @guy-david @tejohnson ! I've rebased the patch to the current version of llvm and seems to be all the comments are fixed. All the test are passed (the linux failure seems has nothing to do with this patch).

Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 10:38 AM
yota9 abandoned this revision.Aug 29 2023, 12:45 PM