SIL: support store_borrow and partial_apply in memory lifetime verification #36211

eeckstein · 2021-03-01T16:17:50Z

... and document store_borrow in SIL.rst

eeckstein · 2021-03-01T16:18:11Z

@swift-ci test

swift-ci · 2021-03-01T17:40:53Z

Build failed
Swift Test Linux Platform
Git Sha - 3e511f3c4d0f8a1b62e78b0aed8335708b15dcbe

eeckstein · 2021-03-01T18:13:18Z

@swift-ci test

swift-ci · 2021-03-01T21:30:04Z

Build failed
Swift Test OS X Platform
Git Sha - e342fba161f46bb2570679f808e5899458713101

gottesmm · 2021-03-01T21:50:27Z

docs/SIL.rst

+  // $T must be a loadable type
+  // %1 must be an alloc_stack $T
+
+Stores the value ``%0`` to a stack location ``%1``, which must be an


Can you add here that this is just the current implementation and the design is not final.

gottesmm

Some questions.

gottesmm · 2021-03-01T21:50:44Z

include/swift/SIL/ApplySite.h

+  /// For details see the partial_apply documentation in SIL.rst.
+  SILArgumentConvention getArgumentOperandConvention(const Operand &oper) const {
+    SILArgumentConvention conv = getArgumentConvention(oper);
+    auto *PAI = dyn_cast<PartialApplyInst>(Inst);


PAI -> pai.

gottesmm · 2021-03-01T21:52:00Z

lib/SIL/Verifier/SILVerifier.cpp

+                    SI->getSrc()->getType(),
+                    "Store operand type and dest type mismatch");
+    require(isa<AllocStackInst>(SI->getDest()),
+            "store_borrow destination can only be an alloc_stack");


Can you add a comment here similar to the SIL.rst comment. I want to make sure it is clear for posterity that this is not the final design and that we want to iterate further on this.

gottesmm · 2021-03-01T21:53:01Z

test/SIL/ownership-verifier/load_borrow_invalidation_partial_apply.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -enable-sil-verify-all -inline %s -verify-continue-on-failure=true -o /dev/null 2>&1 | %FileCheck %s
+// RUN: %target-sil-opt -enable-sil-verify-all -inline %s -verify-continue-on-failure=true -dont-abort-on-memory-lifetime-errors -o /dev/null 2>&1 | %FileCheck %s
 // REQUIRES: asserts


What is broken in this test file?

In addition to the existing error, the memory lifetime verifier also issues an error. The test would fail if the memory lifetime verifier aborts

Can you add a comment here explaining that and also saying which test fails? Something like:

NOTE: We disable the memory lifetime verifier on this file since along side a _____ error that is caught by the load borrow verifier, @mytestname also contains a memory lifetime error. This is expected.

…gn_by_wrapper So far we left this cleanup to SILCombine. But the unused partial_apply (i.e. the "set" in case it's an init or the "init" in case it's a set) violates memory lifetime rules in case "self" is an inout. Once we verify that, it would result in a memory lifetime violation.

…rification.

Verify that * the destination address is an alloc_stack * the stack location is not modified beside a store_borrow * the stack location has been initialized when used

eeckstein · 2021-03-02T11:12:53Z

@swift-ci test

gottesmm

Quick thoughts. Following up about the store_borrow thing via DM.

gottesmm · 2021-03-02T18:51:33Z

lib/SILOptimizer/Mandatory/RawSILInstLowering.cpp

@@ -177,6 +178,7 @@ static void lowerAssignByWrapperInstruction(SILBuilderWithScope &b,
  SILValue dest = inst->getDest();
  SILLocation loc = inst->getLoc();
  SILBuilderWithScope forCleanup(std::next(inst->getIterator()));
+  SingleValueInstruction *closureToDelete = nullptr;


why was this change made? Also can you add a test?

gottesmm · 2021-03-02T18:54:09Z

test/SIL/ownership-verifier/load_borrow_invalidation_partial_apply.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -enable-sil-verify-all -inline %s -verify-continue-on-failure=true -o /dev/null 2>&1 | %FileCheck %s
+// RUN: %target-sil-opt -enable-sil-verify-all -inline %s -verify-continue-on-failure=true -dont-abort-on-memory-lifetime-errors -o /dev/null 2>&1 | %FileCheck %s
 // REQUIRES: asserts


Can you add a comment here explaining that and also saying which test fails? Something like:

NOTE: We disable the memory lifetime verifier on this file since along side a _____ error that is caught by the load borrow verifier, @mytestname also contains a memory lifetime error. This is expected.

gottesmm · 2021-03-02T18:54:39Z

test/SIL/memory_lifetime_failures.sil

@@ -238,3 +238,115 @@ bb0(%0 : @owned $T):
  return %r : $()
 }

+sil [ossa] @closure : $@convention(thin) (@in_guaranteed T) -> () 
+
+// CHECK: SIL memory lifetime failure in @test_non_escaping_closure: memory is initialized at function return but shouldn't


I would make these CHECK-LABEL:. Then I think file check will emit errors for all of them individually rather than only the first error.

gottesmm · 2021-03-02T19:01:23Z

test/SIL/ownership-verifier/interior_pointer.sil

 // CHECK-NEXT: Block: bb0
 // CHECK: Error#: 0. End Error in Function: 'recursive_interior_pointer_error'
 sil [ossa] @recursive_interior_pointer_error : $@convention(thin) (@owned KlassUser, @guaranteed Klass) -> @owned Klass {
 bb0(%0 : @owned $KlassUser, %0a : @guaranteed $Klass):
  %1 = begin_borrow %0 : $KlassUser
  %2 = ref_tail_addr %1 : $KlassUser, $Klass
-  %result = store_borrow %0a to %2 : $*Klass


This was testing that we are able to flag errors on store_borrow. This makes me a bit nervous about restricting store_borrow only to alloc_stack. I think this test case IIRC was from real code that we are emitting today. Now that I think further, one could imagine eliminating a temporary rvalue causing a store_borrow to be moved off of an alloc_stack.

Talked with eeckstein offline. This test is making sure that the use of %result is treated as a liveness requirement of %0. So by removing the store_borrow the test isn't testing anything anymore. Can you delete it?

Also for anyone listening in, we actually do not need to test this anymore since this PR makes it so that store_borrow's destination is always an alloc_stack meaning it can never store to an interior pointer destination.

gottesmm

Can you make these changes and then LGTM!

gottesmm · 2021-03-02T19:23:32Z

I am also fine with a follow on commit to fix (seems testing is finished). All of the changes I asked for are comment sort of changes, no functionality change so I would do that instead.

This is a follow-up of apple#36211

eeckstein requested a review from gottesmm March 1, 2021 16:17

eeckstein force-pushed the memory-lifetime branch from 3e511f3 to e342fba Compare March 1, 2021 18:12

gottesmm reviewed Mar 1, 2021

View reviewed changes

eeckstein added 4 commits March 2, 2021 12:02

MemoryLifetime: support partial_apply arguments in memory lifetime ve…

d6736e9

…rification.

docs: Document the store_borrow instruction

e064ff3

SIL: verify store_borrow

aa16864

Verify that * the destination address is an alloc_stack * the stack location is not modified beside a store_borrow * the stack location has been initialized when used

eeckstein force-pushed the memory-lifetime branch from e342fba to aa16864 Compare March 2, 2021 11:12

gottesmm reviewed Mar 2, 2021

View reviewed changes

gottesmm approved these changes Mar 2, 2021

View reviewed changes

eeckstein merged commit 2d01286 into apple:main Mar 2, 2021

eeckstein deleted the memory-lifetime branch March 2, 2021 19:30

eeckstein mentioned this pull request Mar 3, 2021

SIL: some improvements/fixes around assign_by_wrapper #36253

Merged

eeckstein added a commit to eeckstein/swift that referenced this pull request Mar 3, 2021

tests: cleanup for two ownership verifier tests.

ebbbb6d

This is a follow-up of apple#36211

eeckstein mentioned this pull request Mar 3, 2021

tests: cleanup for two ownership verifier tests. #36255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIL: support store_borrow and partial_apply in memory lifetime verification #36211

SIL: support store_borrow and partial_apply in memory lifetime verification #36211

eeckstein commented Mar 1, 2021

eeckstein commented Mar 1, 2021

swift-ci commented Mar 1, 2021

eeckstein commented Mar 1, 2021

swift-ci commented Mar 1, 2021

gottesmm Mar 1, 2021

eeckstein Mar 2, 2021

gottesmm left a comment

gottesmm Mar 1, 2021

eeckstein Mar 2, 2021

gottesmm Mar 1, 2021

eeckstein Mar 2, 2021

gottesmm Mar 1, 2021

eeckstein Mar 1, 2021

gottesmm Mar 2, 2021 •

edited

eeckstein commented Mar 2, 2021

gottesmm left a comment

gottesmm Mar 2, 2021

gottesmm Mar 2, 2021 •

edited

gottesmm Mar 2, 2021

gottesmm Mar 2, 2021

gottesmm Mar 2, 2021

gottesmm left a comment

gottesmm commented Mar 2, 2021

SIL: support store_borrow and partial_apply in memory lifetime verification #36211

SIL: support store_borrow and partial_apply in memory lifetime verification #36211

Conversation

eeckstein commented Mar 1, 2021

eeckstein commented Mar 1, 2021

swift-ci commented Mar 1, 2021

eeckstein commented Mar 1, 2021

swift-ci commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gottesmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gottesmm Mar 2, 2021 • edited

Choose a reason for hiding this comment

eeckstein commented Mar 2, 2021

gottesmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gottesmm Mar 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gottesmm left a comment

Choose a reason for hiding this comment

gottesmm commented Mar 2, 2021

gottesmm Mar 2, 2021 •

edited

gottesmm Mar 2, 2021 •

edited