Skip to content

[embedded] Remove unspecialized functions before running IRGen #68781

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Sep 28, 2023
Merged

[embedded] Remove unspecialized functions before running IRGen #68781

merged 9 commits into from
Sep 28, 2023

Conversation

kubamracek
Copy link
Contributor

This fixes a problem when emitting debuginfo in IRGen where we might have an "inlined at" debuginfo reference to an unspecialized function, which in turn causes it to get lazily emitted, which crashes in embedded Swift. See the attached testcase for a small reproducer.

Assertion failed: (!loweredFunctionHasGenericArguments(f)), function addLazyFunction, file GenDecl.cpp, line 1557.
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
7  swift-frontend           0x00000001053e89e8 swift::irgen::IRGenerator::noteUseOfTypeGlobals(swift::NominalTypeDecl*, bool, swift::irgen::RequireMetadata_t) (.cold.1) + 0
8  swift-frontend           0x0000000100a48c2c swift::irgen::IRGenerator::addLazyFunction(swift::SILFunction*) + 368
9  swift-frontend           0x0000000100a4ab24 swift::irgen::IRGenModule::getAddrOfSILFunction(swift::SILFunction*, swift::ForDefinition_t, bool, bool) + 484
10 swift-frontend           0x0000000100b547f4 (anonymous namespace)::IRGenDebugInfoImpl::getOrCreateScope(swift::SILDebugScope const*) + 812
11 swift-frontend           0x0000000100b5a680 (anonymous namespace)::IRGenDebugInfoImpl::getOrCreateContext(swift::DeclContext*) + 144
12 swift-frontend           0x0000000100b54e7c (anonymous namespace)::IRGenDebugInfoImpl::emitFunction(swift::SILDebugScope const*, llvm::Function*, swift::SILFunctionTypeRepresentation, swift::SILType, swift::DeclContext*, llvm::StringRef) + 1280
13 swift-frontend           0x0000000100b55aa4 (anonymous namespace)::IRGenDebugInfoImpl::emitFunction(swift::SILFunction&, llvm::Function*) + 160
14 swift-frontend           0x0000000100b8d968 (anonymous namespace)::IRGenSILFunction::emitSILFunction() + 840
15 swift-frontend           0x0000000100b8d078 swift::irgen::IRGenModule::emitSILFunction(swift::SILFunction*) + 1528

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek kubamracek added the embedded Embedded Swift label Sep 26, 2023
@kubamracek
Copy link
Contributor Author

@swift-ci please test Linux platform

@kubamracek
Copy link
Contributor Author

@swift-ci please test Windows platform

@kubamracek
Copy link
Contributor Author

@swift-ci please test

eeckstein
eeckstein reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM.
I have some comments/questions

lib/SILOptimizer/IPO/DeadFunctionElimination.cpp
@@ -526,6 +546,8 @@ class DeadFunctionAndGlobalElimination {

// Check vtable methods.
for (auto &vTable : Module->getVTables()) {
LLVM_DEBUG(llvm::dbgs() << " processing vtable "
<< vTable->getClass()->getName() << '\n');
Copy link
Contributor

@eeckstein eeckstein Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to be very careful with class methods. I'm not sure if DFE will now remove generic (non-final) vtable methods - which are not allowed in embedded swift. If we don't diagnose this earlier, calling such a method will result in a runtime error instead of a compiler error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and this PR is not changing anything around classes and DFE on them (it doesn't happen). I just added this log to get some more visibility into what is considered to be anchors when debugging.

lib/SILOptimizer/PassManager/PassPipeline.cpp Outdated
@@ -891,6 +891,13 @@ SILPassPipelinePlan::getIRGenPreparePassPipeline(const SILOptions &Options) {
// boundaries as required by the ABI.
P.addLoadableByAddress();

if (Options.EmbeddedSwift) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this pass run really needed? There is already a LateDeadFunctionElimination earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's only present in the -O pipeline. But I think we need it to happen in -Onone too, and the embedded/debuginfo.swift shows a problem specifically about -Onone + -g. Any better suggestions for how to ensure this pass runs in all pipelines?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could add it to getOnonePassPipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek
Copy link
Contributor Author

@swift-ci please test Windows platform

eeckstein
eeckstein approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@kubamracek kubamracek merged commit 439aa4b into swiftlang:main Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eeckstein eeckstein eeckstein approved these changes

@zoecarver zoecarver Awaiting requested review from zoecarver

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

Assignees
No one assigned
Labels
embedded Embedded Swift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kubamracek @eeckstein