Skip to content
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

[concurrency] Ban associated objects from being set on instances of actor classes. #34136

Merged

Conversation

gottesmm
Copy link
Contributor

Associated objects are actively dangerous there because they’re non-isolated
actor state, and it’s “new” code wher no backward compatibility concerns that
make it more difficult to ban this on other forms of classes.

rdar://69769048

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm
Copy link
Contributor Author

Windows failure is something in lldb, not this.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 1359aef3f127091d86e421fe5d9a61f5f274af45

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

Great!

I'm super excited and relieved that the runtime support is working well. Thanks @mikeash!

A couple of comments inline.

test/IRGen/actor_class_forbid_objc_assoc_objects.swift Outdated Show resolved Hide resolved
test/Interpreter/class_forbid_objc_assoc_objects.swift Outdated Show resolved Hide resolved
@gottesmm
Copy link
Contributor Author

Note to self: add test for meta type as well.

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch 5 times, most recently from bd59776 to 5a568e3 Compare October 22, 2020 22:16
@gottesmm
Copy link
Contributor Author

@atrick I added tests for the other policies.
@DougGregor I XFAILed the things that should work once the generic actor class isa stomping issue is dealt with.

@gottesmm
Copy link
Contributor Author

@swift-ci test

1 similar comment
@gottesmm
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 5a568e30efe85682e6c4549541176d299f0a01c1

@gottesmm
Copy link
Contributor Author

shakes fist at 32 bit

@gottesmm
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 5a568e30efe85682e6c4549541176d299f0a01c1

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 5a568e30efe85682e6c4549541176d299f0a01c1

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from 5a568e3 to 5640f0d Compare November 20, 2020 06:16
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm
Copy link
Contributor Author

@swift-ci test windows platform

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 5640f0def43f4fafcc20f0cec00b023151f115e3

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from 5640f0d to 15ddf11 Compare November 20, 2020 08:33
@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from 15ddf11 to 28d30d6 Compare November 20, 2020 11:52
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 28d30d66ffc4202fbc96431962a4bd875a949072

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from 28d30d6 to a4df48c Compare November 26, 2020 23:54
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm
Copy link
Contributor Author

We discovered that the simulator was still failing since the simulator being used is from before the banning feature was in the objc runtime. I put in availability conditions in the Interpreter tests corresponding to the introduction of said feature into libobjc to ensure that we just run any tests on such platforms.

@gottesmm
Copy link
Contributor Author

looks like the required bits are slightly different on arm64e for actors. I'll fix that.

@gottesmm
Copy link
Contributor Author

I have a fix. I am going to let the full testing finish, then push that test fix, and do another preset test.

@gottesmm
Copy link
Contributor Author

Ok! All tests passed except for the non-executable tests. I am going to fix that with a force push and rerun the preset.

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from a4df48c to 8bf3084 Compare November 27, 2020 03:53
@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from 8bf3084 to b61c3c3 Compare November 27, 2020 06:09
@gottesmm
Copy link
Contributor Author

Made a typo.

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from b61c3c3 to 7a67cd4 Compare November 27, 2020 22:45
…ctor classes that do not inherit from NSObject.

Associated objects are actively dangerous there because they’re non-isolated
actor state, and it’s “new” code wher no backward compatibility concerns that
make it more difficult to ban this on other forms of classes.

rdar://69769048
@gottesmm gottesmm force-pushed the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch from 7a67cd4 to 91209d3 Compare November 28, 2020 00:12
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 91209d3

@gottesmm
Copy link
Contributor Author

weird lldb test failure. I hope flaky.

@gottesmm
Copy link
Contributor Author

@swift-ci test OS X platform

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

@gottesmm
Copy link
Contributor Author

Retesting

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 91209d3

@gottesmm
Copy link
Contributor Author

@swift-ci test linux platform

@gottesmm gottesmm merged commit ba6c08f into swiftlang:main Nov 30, 2020
@gottesmm gottesmm deleted the pr-7da9c7145fbd96869d5e73a40a600d6c0e23fa91 branch November 30, 2020 18:15
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.

3 participants