Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime Crash Resulting from llvm.used.conditional GlobalDCE during Compilation #65021

Open
NuriAmari opened this issue Apr 7, 2023 · 3 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels

Comments

@NuriAmari
Copy link
Collaborator

In some testing we've discovered the -conditional-runtime-records optimization option can cause runtime crashes in the resulting binary.

Below we have attached a small example:

In this particular example, this protocol conformance record is stripped during GlobalDCE during compilation.

$sSo9EventTypeVSHSCHc ---> protocol conformance descriptor runtime record for __C.EventType : Swift.Hashable in __C_Synthesized

This is possible because it is conditional on $sSHMp, which is dead in this TU.

$sSHMp ---> protocol descriptor for Swift.Hashable

It seems to be dead because it is not used by any other live GVs in this TU, nor is it trivially live. This problem seems to go away if we restrict llvm.used.conditional based stripping in GlobalDCE to post monoLTO merge. This way the usages of $sSHMp present themselves.

cc @kubamracek @manman-ren

conditional-runtime-records.zip

@NuriAmari NuriAmari added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels labels Apr 7, 2023
@kubamracek
Copy link
Contributor

if we restrict llvm.used.conditional based stripping in GlobalDCE to post monoLTO merge

This sounds right, and I'm surprised it (llvm.used.conditional based stripping) is happening before LTO merging. For the stripping to be sound, we need full program visibility. What was your patch that made the problem go away?

@NuriAmari
Copy link
Collaborator Author

This sounds right, and I'm surprised it (llvm.used.conditional based stripping) is happening before LTO merging. For the stripping to be sound, we need full program visibility. What was your patch that made the problem go away?

It seems to me the only condition for running llvm.used.conditional based stripping is the existence of the metadata in the module: https://github.com/apple/llvm-project/blob/next/llvm/lib/Transforms/IPO/GlobalDCE.cpp#L327. I can upstream the patch eventually if we feel it's appropriate, but the patch essentially checks for the LTOPostLink flag on the module before running: https://llvm.org/docs/LangRef.html#lto-post-link-module-flags-metadata.

That said, even with restricting llvm.used.conditional to post full LTO merge, I still observed a crash. I haven't gotten around to a minimal repro or a fix, but I think we are incorrectly stripping conformances to _ObjCBridgeable. I don't know why yet, but we saw unregonized selector crashes because Swift types with custom bridging behavior were instead being boxed in __SwiftValue. My guess would be even full Swift mono LTO doesn't have enough visibilty into the ObjC side of things.

@manman-ren
Copy link

In terms of full program visibility, we are assuming everything has hidden visibility for now.

Last time I checked, Swift symbols with public linkage are using DefaultVisibility in getIRLinkage. If Swift support annotation to mark variables with beyond-linkage-unit visibility, maybe we can correctly mark symbols with visibility, teach conditional-runtime-records to check for visibility and/or teach GlobalDCE conditional stripping to look at visibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels
Projects
None yet
Development

No branches or pull requests

3 participants