Skip to content

[ownership] On non-Darwin platforms, serialize the stdlib in OSSA form. #36025

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 2 commits into from
Feb 18, 2021
Merged

[ownership] On non-Darwin platforms, serialize the stdlib in OSSA form. #36025

merged 2 commits into from
Feb 18, 2021

Conversation

gottesmm
Copy link
Contributor

This PR has two commits in it:

  1. The first fixes a bunch of deserialization invalidation issues. These memory error causing bugs were being triggered in the global variable case when building Foundation when in this mode.
  2. The second changes non-Darwin platforms to match Darwin and serialize the stdlib in OSSA form. Previously I enabled this only for Darwin since I ran into the bug from the first PR. To ensure that the Darwin side didn't regress, I upstreamed that while I investigated 1. Now that I figured that out, this PR turns it on and restores the tests I had to modify to allow for the platforms to differ here in favor of their original form.

@gottesmm
[sil-deserialization] Implement cache invalidation for all serialized…
891bd00 
… SIL entities and use it to properly invalidate global variables/witness tables when we delete them.

Otherwise, one runs into memory corruption. I ran into this while enabling ossa
on the stdlib for non-Darwin platforms.

Hopefully we do not regress on this again when someone adds more optzns that
eliminate these since I added a big NOTE to warn people to do it and implemented
support even for the entities we do not support deleting at the SIL
level... yet.
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm requested review from atrick and meg-gupta February 17, 2021 21:37
@gottesmm
Copy link
Contributor Author

@swift-ci test windows platform

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - a93b091f0537626bbfbd2f21d21081d018418ef7

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - a93b091f0537626bbfbd2f21d21081d018418ef7

atrick
atrick approved these changes Feb 17, 2021
View reviewed changes
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.

Very nice! So out of curiosity, is there a potential issue with asking to deserialize entities that were already deleted, separate from memory corruption? We don't want to repeatedly deserialize.

@gottesmm gottesmm force-pushed the pr-36fe6ec4772e3d5e09f5031a505217e7d7aba8dc branch from a93b091 to 0abe458 Compare February 17, 2021 23:25
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@atrick I didn't look into that. In the deserialization case it is when we early on deserialize _swiftEmptyArrayStorage, then DCE it, then down the line after maybe additional inlining/etc it becomes exposed after deserializing a different function.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0abe4581e6bcac3d371e2d76e726476066882a29

@gottesmm
[ownership] On non-Darwin platforms start serializing the stdlib in O…
57654da 
…SSA as well.

I had to fix a memory corruption bug (see previous commit) to land this. But now
that it is fixed, we are ready!
@gottesmm gottesmm force-pushed the pr-36fe6ec4772e3d5e09f5031a505217e7d7aba8dc branch from 0abe458 to 57654da Compare February 17, 2021 23:59
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm merged commit 02784b3 into swiftlang:main Feb 18, 2021
@gottesmm gottesmm deleted the pr-36fe6ec4772e3d5e09f5031a505217e7d7aba8dc branch February 18, 2021 03:48
@drodriguez drodriguez mentioned this pull request Feb 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atrick atrick atrick approved these changes

@meg-gupta meg-gupta Awaiting requested review from meg-gupta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gottesmm @swift-ci @atrick