-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cxx-interop] Fix crash when importing C++ forward-declared template specializations in typedefs #84186
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
[cxx-interop] Fix crash when importing C++ forward-declared template specializations in typedefs #84186
Conversation
@swift-ci please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem you describe and the fix you propose make sense to me at a high level, but I'm having a bit of trouble connecting it to your explanation:
Forward-declared explicit specializations (
TSK_ExplicitSpecialization
withoutisCompleteDefinition()
) cause Clang'sgetTypeInfoInChars()
to access the primary template's dependent types, triggering this assertion failure :
Assertion failed: (!T->isDependentType() && "should not see dependent types here"), function getTypeInfoImpl, file TypeNodes.inc, line 79
Where is getTypeInfoInChars()
being called (by Swift)? And why does an incomplete definition lead to that code path (in Clang, presumably)? The relationship between the incomplete definition and dependent types is a little unclear to me here.
test/Interop/Cxx/templates/Inputs/ForwardDeclaredSpecialization.h
Outdated
Show resolved
Hide resolved
test/Interop/Cxx/templates/Inputs/ForwardDeclaredSpecialization.h
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
7108c3e
to
7688401
Compare
test/Interop/Cxx/templates/Inputs/ForwardDeclaredSpecialization.h
Outdated
Show resolved
Hide resolved
I think not importing these types is the right fix. That being said, similar to John, I'd expect a different failure/assertion about incomplete types rather than something about dependent types. The question is, whether we end up switching to the primary template instead of the specialization inadvertently in the importer (or in Clang). If that is the case, this might be a symptom that we have a different bug (in addition to the one you fixed here). So it might be interesting to understand why is this happening, but I think that investigation can happen independently of this fix. |
…specializations in typedefs rdar://147595723
7688401
to
7bfbd68
Compare
7bfbd68
to
b5a0a4e
Compare
@swift-ci please smoke test |
After some printf debugging in clang, I identified this root cause of the assertion failure: in clang/lib/AST/RecordLayoutBuilder.cpp:1444, the
This field iteration returns Call chain: Swift's ClangImporter calls My guess is that in pure C++ compilation, we get an "incomplete type" error during semantic analysis before reaching layout. But Swift's ClangImporter takes a different path - it directly calls layout APIs ( CC: @j-hui, @Xazax-hun For reference, pasting the assertion failure log (just the clang part) below:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing up all that investigation! This all makes sense to me, and not importing these incomplete types is the right thing to do.
My only concern is here that this may be confusing for users who see the explicit template specialization and do not realize that there’s no definition associated with it. We could emit a more specific diagnostic there, but I don’t think that’s necessary since trying to use that incomplete type definition is also an error in C++.
float value; | ||
}; | ||
|
||
// Case 4: For comparison - forward-declared non-templated struct (have same behavior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it’s not clear what the behavior is the same as. Better to make it explicit imo:
// Case 4: For comparison - forward-declared non-templated struct (have same behavior) | |
// Case 4: For comparison - forward-declared non-templated struct (also does not import) |
This suggestion isn’t blocking, i.e., no need to change the comment and be forced to re-run CI.
Thanks for investigating!
This is the surprising part. The specialization is an incomplete types, so it does not have definition data, does not have fields. I'd expect clang to error out when we call |
Swift crashes when importing C++ typedefs to forward-declared explicit template specializations. In assert build we see this assertion failure happening in clang:
Assertion failed: (!T->isDependentType() && "should not see dependent types here"), function getTypeInfoImpl, file TypeNodes.inc, line 79.
Example that crashes:
In this patch, I propose detecting forward-declared explicit specializations in typedef imports. Instead of crashing, these specializations should be blocked from being imported with a diagnostic similar to the forward declared (but not defined) structs.
rdar://147595723