Skip to content

Re-apply "Preserve SIL when merging modules" #11943

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

Conversation

slavapestov
Copy link
Contributor

I had to revert #8388 in #11938 because check-swift-validation-optimize was failing.

This PR fixes two issues exposed by running the tests with optimizations and re-applies the original changes:

  • Memory corruption with overly-aggressive inlining of resilient class allocation
  • SILGen was not serializing witness tables for imported conformances

The witness table had shared linkage, but we weren't serializing them,
which would cause linking errors if we emitted a reference to such a
witness table from a different module than the one where it was first
defined, as a result of deserializing and optimizing SIL.

This issue was introduced when SIL witness table serialization was
made conditional on the -sil-serialize-witness-tables flag, which is
normally only enabled for the standard library.

When the flag was added, existing tests were updated to pass the
flag, which masked the issue. Remove the flag from existing tests,
ensure that imported witness tables are still [serialized], and add
a new test specifically for the behavior enabled by this flag.
We were inlining the size and alignment, which was not correct.
This was the cause of a long-standing ASAN failure in some
library evolution tests.

Fixes <rdar://problem/24540778>.
This test used to fail because we would look up an associated type
in an incomplete conformance after Sema was torn down, causing a
crash.

Now, the ClangImporter is able to correctly complete the conformance.
Also pass flags to disable SIL optimization passes when merging
modules, since that's completely unnecessary.

An evolution test that used to fail with WMO disabled now passes
with this change.

FIxes <rdar://problem/18913977>.
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please ASAN test

@slavapestov slavapestov merged commit 768fb5d into swiftlang:master Sep 15, 2017
Copy link

@abadi4279 abadi4279 left a comment

Choose a reason for hiding this comment

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

ab2901a953db01c8bbfe35353fa1f966fb1530c4

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.

2 participants