Skip to content

[cxx-interop] Do not crash while generating a diagnostic for un-instantiatable template #63063

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 1 commit into from
Jan 18, 2023
Merged

[cxx-interop] Do not crash while generating a diagnostic for un-instantiatable template #63063

merged 1 commit into from
Jan 18, 2023

Conversation

egorzhdan
Copy link
Contributor

Swift was previously crashing while emitting an error ("could not generate C++ types from the generic Swift types provided") for C++ decls that do not have a name, such as constructors: func->getName() triggered an assertion.

Assertion failed: (Name.isIdentifier() && "Name is not a simple identifier"), function getName, file Decl.h, line 275.
...
8  swift-frontend           0x000000010b455b00 swift::DiagnosticEngine::diagnose(swift::SourceLoc, swift::DiagID, llvm::ArrayRef<swift::DiagnosticArgument>) (.cold.1) + 0
9  swift-frontend           0x000000010384c608 swift::ClangImporter::instantiateCXXFunctionTemplate(swift::ASTContext&, clang::FunctionTemplateDecl*, swift::SubstitutionMap) + 636
10 swift-frontend           0x000000010384c99c swift::ClangImporter::getCXXFunctionTemplateSpecialization(swift::SubstitutionMap, swift::ValueDecl*) + 300
11 swift-frontend           0x000000010353ecac swift::constraints::Solution::resolveConcreteDeclRef(swift::ValueDecl*, swift::constraints::ConstraintLocator*) const + 284
12 swift-frontend           0x0000000103554c5c (anonymous namespace)::ExprRewriter::finishApply(swift::ApplyExpr*, swift::Type, swift::constraints::ConstraintLocatorBuilder, swift::constraints::ConstraintLocatorBuilder) + 372
13 swift-frontend           0x000000010355a060 (anonymous namespace)::ExprRewriter::visitApplyExpr(swift::ApplyExpr*) + 212
14 swift-frontend           0x00000001035457e4 (anonymous namespace)::ExprWalker::walkToExprPost(swift::Expr*) + 132
15 swift-frontend           0x0000000103a24d48 (anonymous namespace)::Traversal::doIt(swift::Expr*) + 1012
...

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Jan 17, 2023
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

zoecarver
zoecarver reviewed Jan 17, 2023
View reviewed changes
lib/ClangImporter/ClangImporter.cpp Outdated
// TODO: Use the location of the apply here.
// TODO: This error message should not reference implementation details.
// See: https://github.com/apple/swift/pull/33053#discussion_r477003350
ctx.Diags.diagnose(SourceLoc(),
diag::unable_to_convert_generic_swift_types.ID,
{func->getName(), StringRef(failedTypesStr)});
{StringRef(funcName), StringRef(failedTypesStr)});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is .diagnose going to copy when it constructs an in-flight diagnostic? Otherwise, is this going to be a dangling pointer?

I guess we do this with failedTypesStr too, so it's probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we can actually avoid the creation of StringRefs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The signature for unable_to_convert_generic_swift_types takes two StringRefs so are we just doing an implicit conversion?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be safe (depending on when the diagnostic is emitted/if it's stored anywhere), I really don't know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you're right. On a closer look, StringRefs in diagnostic parameters are either emitted in the destructor of InFlightDiagnostic or copied in DiagnosticEngine::onTentativeDiagnosticFlush, so this is safe.

@egorzhdan
[cxx-interop] Do not crash while generating a diagnostic for un-insta…
65f319a 
…ntiatable template

Swift was previously crashing while emitting an error ("could not generate C++ types from the generic Swift types provided") for C++ decls that do not have a name, such as constructors: `func->getName()` triggered an assertion.
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-diagnostic-crash branch from 2844fe8 to 65f319a Compare January 17, 2023 17:19
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan merged commit ca6fd61 into main Jan 18, 2023
@egorzhdan egorzhdan deleted the egorzhdan/cxx-diagnostic-crash branch January 18, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zoecarver zoecarver zoecarver approved these changes

@hyp hyp Awaiting requested review from hyp

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@egorzhdan @zoecarver