Skip to content

Conversation

compnerd
Copy link
Member

This adds tracking of the vtable holes due to a failure to deserialize
vtable entries. This allows for the user to be able to identify what
member failed to be deserialied and can aid in understanding why an
open class may not be subclassed.

Future improvements here would allow tracing the XRefPath which failed
to be deserialied. However, this still provides an improvement over the
existing experience where there is no available information on why the
class cannot be inherited from.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

@compnerd
Copy link
Member Author

CC: @xedin @CodaFi @xymus

@compnerd
Copy link
Member Author

@swift-ci please test

@@ -338,6 +338,9 @@ class ASTContext final {
llvm::SmallPtrSet<DerivativeAttr *, 1>>
DerivativeAttrs;

llvm::DenseMap<ClassDecl *, llvm::SmallVector<DeclName, 2>>
MissingClassVTableEntries;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you store the DeclName in the MissingMemberDecl instead of adding another table to the ASTContext?

Copy link
Member Author

@compnerd compnerd Jan 25, 2022

Choose a reason for hiding this comment

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

While possible to iterate the members and pull out the MissingMemberDecl, it may limit any additional context from being saved off (due to Decls being bump ptr allocated). If you still think that it's better to do it that way, LMK and I can change this pretty easily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what you mean by limiting context from being saved off?

What I'm suggesting is iterating over the members in TypeCheckDeclPrimary, yeah.

Copy link
Member Author

@compnerd compnerd Jan 25, 2022

Choose a reason for hiding this comment

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

Adding any additional context like the XRefTrace or the module which failed to load would not be possible AIUI. (Anything that is not trivially destructible is not embeddable).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can store that context in the MissingMemberDecl itself (it's fine for bump-allocated Decls to point to other things. You can either bump-allocate the additional context, or heap-allocate it and register a global destructor with the ASTContext on the MissingMemberDecl field in question).

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 87ee5bd6003b59ea34615ef367c35d2e76acfc9a

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 87ee5bd6003b59ea34615ef367c35d2e76acfc9a

This adds tracking of the vtable holes due to a failure to deserialize
vtable entries.  This allows for the user to be able to identify what
member failed to be deserialied and can aid in understanding why an
`open` class may not be subclassed.

Future improvements here would allow tracing the XRefPath which failed
to be deserialied.  However, this still provides an improvement over the
existing experience where there is no available information on why the
class cannot be inherited from.
@compnerd
Copy link
Member Author

@swift-ci please test

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

This new version is much cleaner in my opinion -- thank you for considering the change!

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 86de45a

@compnerd
Copy link
Member Author

@swift-ci please test macOS platform

@compnerd
Copy link
Member Author

This new version is much cleaner in my opinion -- thank you for considering the change!

I wasn't particularly enthused by the use of ASTContext to store the information either, so I agree with you that this version is better.

// RUN: %empty-directory(%t)
// RUN: %empty-directory(%t/swift)
// RUN: %target-swift-frontend -emit-module -emit-module-path %t/swift/SwiftLibrary.swiftmodule %s -module-name SwiftLibrary -I%S/Inputs
// RUN: %target-typecheck-verify-swift -DCLIENT -c %s -module-name client -I%t/swift
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Jan 26, 2022

Choose a reason for hiding this comment

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

What does -c do? I couldn't find it in either options list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, should have looked at the modes too.

@compnerd compnerd merged commit e7ae1f4 into swiftlang:main Jan 26, 2022
@compnerd compnerd deleted the undeserialized branch January 26, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants