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

Add new runtime functions for handling dependencies when initializing class metadata #22386

Merged
merged 3 commits into from Feb 6, 2019

Conversation

Projects
None yet
3 participants
@rjmccall
Copy link
Member

rjmccall commented Feb 5, 2019

This is part of the fix for the race condition identified in rdar://47549859. The fix in #22381 is too naive because we cannot stage it in; instead we need to make a runtime-only change to add new runtime functions, then switch the compiler to use them only when the compiler entrypoints can be safely presumed to exist.

rjmccall added some commits Feb 5, 2019

Requestify the mangling-to-metadata APIs.
Note that I've called out a couple of suspicious places where we
are requesting abstract metadata for superclasses but probably
need to be requesting something more complete.

@rjmccall rjmccall force-pushed the rjmccall:init-class-metadata-depedencies-runtime branch from 5dfa8e1 to 887564d Feb 5, 2019

@rjmccall

This comment has been minimized.

Copy link
Member Author

rjmccall commented Feb 5, 2019

@swift-ci Please test.

@rjmccall rjmccall requested review from slavapestov and jckarter Feb 5, 2019

@jckarter

This comment has been minimized.

Copy link
Member

jckarter commented Feb 5, 2019

It looks like you didn't add compatibility override slots for the new entry points in this variation.

@rjmccall

This comment has been minimized.

Copy link
Member Author

rjmccall commented Feb 5, 2019

It looks like you didn't add compatibility override slots for the new entry points in this variation.

I don't think there were compatibility override slots for the existing entry points.

@rjmccall rjmccall force-pushed the rjmccall:init-class-metadata-depedencies-runtime branch from 887564d to a074bc2 Feb 5, 2019

@rjmccall

This comment has been minimized.

Copy link
Member Author

rjmccall commented Feb 5, 2019

@swift-ci Please test.

1 similar comment
@rjmccall

This comment has been minimized.

Copy link
Member Author

rjmccall commented Feb 5, 2019

@swift-ci Please test.

@jckarter

This comment has been minimized.

Copy link
Member

jckarter commented Feb 5, 2019

Ah, you're right, I had assumed that they were because of the other changes. Nevermind. Aside from this issue it LGTM.

@@ -808,6 +808,34 @@ FUNCTION(UpdateClassMetadata,
SizeTy->getPointerTo()),
ATTRS(NoUnwind))

// struct FieldInfo { size_t Size; size_t AlignMask; };

This comment has been minimized.

@brentdax

brentdax Feb 5, 2019

Collaborator

Just a comment fix, but what is this "FieldInfo"? Is it supposed to be the "TypeLayout" type mentioned in the comment's function signature?

This comment has been minimized.

@rjmccall

rjmccall Feb 5, 2019

Author Member

Yes. I just copied that comment from another declaration, but I can fix it.

@rjmccall

This comment has been minimized.

Copy link
Member Author

rjmccall commented Feb 6, 2019

Private testing shows that the actual runtime code works.

@rjmccall

This comment has been minimized.

Copy link
Member Author

rjmccall commented Feb 6, 2019

I'll do Brent's request in the compiler PR.

@rjmccall rjmccall merged commit 2e252e1 into apple:master Feb 6, 2019

4 checks passed

Swift Test Linux Platform 11676 tests run, 10541 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 58585 tests run, 2502 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@rjmccall rjmccall deleted the rjmccall:init-class-metadata-depedencies-runtime branch Feb 6, 2019

rjmccall added a commit to rjmccall/swift that referenced this pull request Feb 6, 2019

Fix a race condition with the initialization of class metadata.
In the initial approach for resolving metadata dependency cycles, superclass metadata was fetched (as non-transitively complete) by the initialization function and passed to `swift_initClassMetadata`. That was changed so that `swift_initClassMetadata` instead fetched the superclass metadata via a demangling. Unfortunately, it only fetched _abstract_ metadata, which is not actually safe to pass to the ObjC runtime, which can manifest as a crash in either the runtime or (potentially) downstream of it. Worse, `swift_initClassMetadata` has no way of reporting failure to its caller, and so we cannot fix this problem without using a blocking fetch and subverting the cyclic-metadata infrastructure.

This patch builds on apple#22386 by changing the compiler to actually use the new runtime function(s) added there.

Fixes rdar://47549859.

rjmccall added a commit to rjmccall/swift that referenced this pull request Feb 6, 2019

Fix a race condition with the initialization of class metadata.
In our initial approach for resolving metadata dependency cycles with classes, non-transitively complete superclass metadata was fetched by the subclass's metadata completion function and passed to `swift_initClassMetadata`. That could mean generating quite a lot of code in the completion function, and so we fairly recently changed it so that `swift_initClassMetadata` instead fetched the superclass metadata via a demangling. Unfortunately, the metadata demangler only fetches _abstract_ metadata by default, and class metadata cannot be considered even non-transitively complete when its superclass reference not at that stage.  If the superclass metadata is being completed on one thread, and a subclass is being completed on another, and the subclass installs the incomplete superclass metadata in its superclass field and attempts to register the subclass with the Objective-C runtime, the runtime may crash reading the incompletely-initialized superclass.

The proper fix is to make `swift_initClassMetadata` fetch non-transitively complete metadata for the superclass, delaying completion if that metadata is unavailable. Unfortunately, that can't actually be implemented on top of `swift_initClassMetadata` because that function has no means of reporting an unsatisfied dependency to its caller, and we can no longer simply change its signature without worrying about a small of internal code that might still be using it. We cannot simply perform a blocking metadata request in `swift_initClassMetadata` because it is deeply problematic to block within a metadata completion function. The solution is therefore to add a `swift_initClassMetadata2` which has the ability to report unsatisfied dependencies. That was done in apple#22386; this patch builds on that by teaching the compiler to generate code to actually use it. It is therefore not safe to use this patch if you might be running on an OS that only provides the old runtime function, but that should be a temporary Apple-internal problem.

Fixes rdar://47549859.

rjmccall added a commit to rjmccall/swift that referenced this pull request Feb 7, 2019

Fix a race condition with the initialization of class metadata.
In our initial approach for resolving metadata dependency cycles with classes, non-transitively complete superclass metadata was fetched by the subclass's metadata completion function and passed to `swift_initClassMetadata`. That could mean generating quite a lot of code in the completion function, and so we fairly recently changed it so that `swift_initClassMetadata` instead fetched the superclass metadata via a demangling. Unfortunately, the metadata demangler only fetches _abstract_ metadata by default, and class metadata cannot be considered even non-transitively complete when its superclass reference not at that stage.  If the superclass metadata is being completed on one thread, and a subclass is being completed on another, and the subclass installs the incomplete superclass metadata in its superclass field and attempts to register the subclass with the Objective-C runtime, the runtime may crash reading the incompletely-initialized superclass.

The proper fix is to make `swift_initClassMetadata` fetch non-transitively complete metadata for the superclass, delaying completion if that metadata is unavailable. Unfortunately, that can't actually be implemented on top of `swift_initClassMetadata` because that function has no means of reporting an unsatisfied dependency to its caller, and we can no longer simply change its signature without worrying about a small of internal code that might still be using it. We cannot simply perform a blocking metadata request in `swift_initClassMetadata` because it is deeply problematic to block within a metadata completion function. The solution is therefore to add a `swift_initClassMetadata2` which has the ability to report unsatisfied dependencies. That was done in apple#22386; this patch builds on that by teaching the compiler to generate code to actually use it. It is therefore not safe to use this patch if you might be running on an OS that only provides the old runtime function, but that should be a temporary Apple-internal problem.

Fixes rdar://47549859.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment