Skip to content

[ownership] Enable ownership verification by default. #23530

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

Merged

Conversation

gottesmm
Copy link
Contributor

[ownership] Enable ownership verification by default.

I also removed the -verify-sil-ownership flag in favor of a disable flag
-disable-sil-ownership-verifier. I used this on only two tests that still need
work to get them to pass with ownership, but whose problems are well understood,
small corner cases. I am going to fix them in follow on commits. I detail them
below:

  1. SILOptimizer/definite_init_inout_super_init.swift. This is a test case where
    DI is supposed to error. The only problem is that we crash before we error since
    the code emitting by SILGen to trigger this error does not pass ownership
    invariants. I have spoken with JoeG about this and he suggested that I fix this
    earlier in the compiler. Since we do not run the ownership verifier without
    asserts enabled, this should not affect compiler users. Given that it has
    triggered DI errors previously I think it is safe to disable ownership here.

  2. PrintAsObjC/extensions.swift. In this case, the signature generated by type
    lowering for one of the thunks here uses an unsafe +0 return value instead of
    doing an autorelease return. The ownership checker rightly flags this leak. This
    is going to require either an AST level change or a change to TypeLowering. I
    think it is safe to turn this off since it is such a corner case that it was
    found by a test that has nothing to do with it.

rdar://43398898


NOTE: This is mostly updating tests. Also I left in part of #23529 b/c I want to start the testing here.

I also removed the -verify-sil-ownership flag in favor of a disable flag
-disable-sil-ownership-verifier. I used this on only two tests that still need
work to get them to pass with ownership, but whose problems are well understood,
small corner cases. I am going to fix them in follow on commits. I detail them
below:

1. SILOptimizer/definite_init_inout_super_init.swift. This is a test case where
DI is supposed to error. The only problem is that we crash before we error since
the code emitting by SILGen to trigger this error does not pass ownership
invariants. I have spoken with JoeG about this and he suggested that I fix this
earlier in the compiler. Since we do not run the ownership verifier without
asserts enabled, this should not affect compiler users. Given that it has
triggered DI errors previously I think it is safe to disable ownership here.

2. PrintAsObjC/extensions.swift. In this case, the signature generated by type
lowering for one of the thunks here uses an unsafe +0 return value instead of
doing an autorelease return. The ownership checker rightly flags this leak. This
is going to require either an AST level change or a change to TypeLowering. I
think it is safe to turn this off since it is such a corner case that it was
found by a test that has nothing to do with it.

rdar://43398898
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

@gottesmm gottesmm merged commit 5f74067 into swiftlang:master Mar 25, 2019
@gottesmm gottesmm deleted the pr-9b33d504e655ecef59b2e8b29f155d0ffd2a47bd branch March 25, 2019 10:38
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.

1 participant