Skip to content

[ownership] When computing usesNotContainedWithinLifetime make sure the error is only a use not in lifetime error. #33618

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

Merged
merged 1 commit into from
Aug 31, 2020
Merged

[ownership] When computing usesNotContainedWithinLifetime make sure the error is only a use not in lifetime error. #33618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions lib/SIL/Verifier/LinearLifetimeChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,6 @@ bool LinearLifetimeChecker::validateLifetime(
bool LinearLifetimeChecker::usesNotContainedWithinLifetime(
SILValue value, ArrayRef<Operand *> consumingUses,
ArrayRef<Operand *> usesToTest) {

auto errorBehavior = ErrorBehaviorKind(
ErrorBehaviorKind::ReturnFalse |
ErrorBehaviorKind::StoreNonConsumingUsesOutsideLifetime);
Expand All @@ -707,7 +706,13 @@ bool LinearLifetimeChecker::usesNotContainedWithinLifetime(
assert(numFoundUses == uniqueUsers.size());
#endif

// Return true if we /did/ found an error and when emitting that error, we
// If we found any error except for uses outside of our lifetime, bail.
if (error.getFoundLeak() || error.getFoundOverConsume() ||
error.getFoundUseAfterFree())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually I may want to have some sort of enum here. Makes me nervous doing these sorts of exhaustive checks. But this code doesn't change too much so I think it is ok for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I find it easier to think about APIs that don't involve double-negative. i.e. is should be '!usesContainded', not 'usesNotContained'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I am fine with changing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to invert that in a follow on commit.

return false;

// Return true if we /did/ find an error and when emitting that error, we
// found /all/ uses we were looking for.
return error.getFoundError() && numFoundUses == usesToTest.size();
return error.getFoundUseOutsideOfLifetime() &&
numFoundUses == usesToTest.size();
}
88 changes: 88 additions & 0 deletions test/SILOptimizer/semantic-arc-opts.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,19 @@ sil @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) ->
sil @owned_user : $@convention(thin) (@owned Builtin.NativeObject) -> ()
sil @get_owned_obj : $@convention(thin) () -> @owned Builtin.NativeObject
sil @unreachable_guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> MyNever
sil @inout_user : $@convention(thin) (@inout FakeOptional<NativeObjectPair>) -> ()

struct NativeObjectPair {
var obj1 : Builtin.NativeObject
var obj2 : Builtin.NativeObject
}

struct FakeOptionalNativeObjectPairPair {
var pair1 : FakeOptional<NativeObjectPair>
var pair2 : FakeOptional<NativeObjectPair>
}
sil @inout_user2 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> ()

sil @get_nativeobject_pair : $@convention(thin) () -> @owned NativeObjectPair

protocol MyFakeAnyObject : Klass {
Expand Down Expand Up @@ -2639,3 +2646,84 @@ bb6:
inject_enum_addr %0 : $*FakeOptional<Klass>, #FakeOptional.none!enumelt
br bb5
}

// CHECK-LABEL: sil [ossa] @destructure_with_differing_lifetimes_inout_1 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> () {
// CHECK-NOT: load_borrow
// CHECK: } // end sil function 'destructure_with_differing_lifetimes_inout_1'
sil [ossa] @destructure_with_differing_lifetimes_inout_1 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> () {
bb0(%0 : $*FakeOptionalNativeObjectPairPair):
%0a = struct_element_addr %0 : $*FakeOptionalNativeObjectPairPair, #FakeOptionalNativeObjectPairPair.pair1
%1 = load [copy] %0a : $*FakeOptional<NativeObjectPair>
switch_enum %1 : $FakeOptional<NativeObjectPair>, case #FakeOptional.some!enumelt: bb1, default bb2

bb2(%2 : @owned $FakeOptional<NativeObjectPair>):
destroy_value %2 : $FakeOptional<NativeObjectPair>
br bbEnd

bb1(%3 : @owned $NativeObjectPair):
(%3a, %3b) = destructure_struct %3 : $NativeObjectPair
cond_br undef, bb1a, bb1b

bb1a:
destroy_value %3a : $Builtin.NativeObject
destroy_value %3b : $Builtin.NativeObject
br bbEnd

bb1b:
destroy_value %3a : $Builtin.NativeObject
%f = function_ref @inout_user2 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> ()
apply %f(%0) : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> ()
destroy_value %3b : $Builtin.NativeObject
br bbEnd

bbEnd:
%9999 = tuple()
return %9999 : $()
}

// CHECK-LABEL: sil [ossa] @destructure_with_differing_lifetimes_inout_2 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> () {
// CHECK-NOT: load_borrow
// CHECK: } // end sil function 'destructure_with_differing_lifetimes_inout_2'
sil [ossa] @destructure_with_differing_lifetimes_inout_2 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> () {
bb0(%0 : $*FakeOptionalNativeObjectPairPair):
%0a = struct_element_addr %0 : $*FakeOptionalNativeObjectPairPair, #FakeOptionalNativeObjectPairPair.pair1
%1 = load [copy] %0a : $*FakeOptional<NativeObjectPair>
switch_enum %1 : $FakeOptional<NativeObjectPair>, case #FakeOptional.some!enumelt: bb1, default bb2

bb2(%2 : @owned $FakeOptional<NativeObjectPair>):
destroy_value %2 : $FakeOptional<NativeObjectPair>
br bbEnd

bb1(%3 : @owned $NativeObjectPair):
(%3a, %3b) = destructure_struct %3 : $NativeObjectPair
cond_br undef, bb1a, bb1b

bb1a:
destroy_value %3a : $Builtin.NativeObject
br bb1ab

bb1ab:
destroy_value %3b : $Builtin.NativeObject
br bbEnd

bb1b:
destroy_value %3a : $Builtin.NativeObject
%f = function_ref @inout_user2 : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> ()
apply %f(%0) : $@convention(thin) (@inout FakeOptionalNativeObjectPairPair) -> ()
cond_br undef, bb1ba, bb1bb

bb1ba:
br bb1baEnd

bb1bb:
br bb1baEnd

bb1baEnd:
destroy_value %3b : $Builtin.NativeObject
br bbEnd

bbEnd:
%9999 = tuple()
return %9999 : $()
}