[sil] Expand immutable address use verification to in_guaranteed parameters.

[gottesmm]

The main commit in this PR expands our immutable address verification from only being used with open_existential_addr to also being used with in_guaranteed parameters. Beyond catching actual invalid uses of in_guaranteed parameters, this also ensures that we do not hit the assert after inlining a callee (with a mutable use of an in_guaranteed parameter) into a caller that has an immutable open_existential_addr (triggering the original assertion).

The 2nd commit fixes a small issue exposed by the test vtable_thunks_reabstraction.swift that causes the verifier to trip.

rdar://50212579

[gottesmm]

@swift-ci smoke test

[gottesmm]

Oops missed this one. I am going to need to fix this.

[gottesmm]

Hmmm... I thought I was able to get rid the entire compiler. I wonder what changed. The failure here looks like a real use after free.

[gottesmm]

Unless I am reading the result incorrectly, both are hitting something related to:

// function_ref specialized Array._appendElementAssumeUniqueAndCapacity(_:newElement:)
11:09:17   %89 = function_ref @$sSa37_appendElementAssumeUniqueAndCapacity_03newB0ySi_xntFs9CodingKey_p_Tg5Tf4nen_n : $@convention(thin) <�_0_0 where �_0_0 : CodingKey> (Int, @in �_0_0, @inout Array<CodingKey>) -> () // user: %90
11:09:17   %90 = apply %89<�_0_0>(%71, %1, %40) : $@convention(thin) <�_0_0 where �_0_0 : CodingKey> (Int, @in �_0_0, @inout Array<CodingKey>) -> ()
11:09:17   end_access %40 : $*A

[gottesmm]

(%1 above is an in_guaranteed parameter)

[atrick]

Looks fine so far. I wonder if we should verify that @in is nonmutating (but consuming).

[gottesmm]

@swift-ci smoke test

[gottesmm]

@swift-ci smoke test

[gottesmm]

@swift-ci smoke test

[gottesmm]

The failure on macOS isn't reproducing for me.

[gottesmm]

@swift-ci smoke test

[gottesmm]

Found the problem (turns out someone updated this code earlier today and I hadn't rebased to get the info). FSO is converting @in -> @in_guaranteed in a funky way. So this is most likely /not/ a real use-after-free. I need to understand it better.

[gottesmm]

@atrick this one is a good one. The bug was real. So this check has caught 2 bugs so far ; ). SILCombine is miscompiling the following code:

sil @test : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@in τ_0_0) -> () {
bb0(%0 : $*τ_0_0):
  %1 = alloc_stack $P
  %2 = init_existential_addr %1 : $*P, $τ_0_0
  copy_addr [take] %0 to [initialization] %2 : $*τ_0_0
  %3 = function_ref @in_caller : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@in τ_0_0) -> ()
  %4 = alloc_stack $P
  copy_addr %1 to [initialization] %4 : $*P
  %5 = open_existential_addr mutable_access %4 : $*P to $*@opened("C494A60E-71EA-11E9-B8C0-D0817AD3F8AD") P
  apply %3<@opened("C494A60E-71EA-11E9-B8C0-D0817AD3F8AD") P>(%5) : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@in τ_0_0) -> ()
  dealloc_stack %4 : $*P
  destroy_addr %1 : $*P
  dealloc_stack %1 : $*P
  %9999 = tuple()
  return %9999 : $()
}

by transforming it into:

sil @test : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@in τ_0_0) -> () {
// %0                                             // user: %2
bb0(%0 : $*τ_0_0):
  %1 = alloc_stack $τ_0_0                        // users: %5, %6, %2, %4
  copy_addr [take] %0 to [initialization] %1 : $*τ_0_0 // id: %2
  // function_ref in_caller
  %3 = function_ref @in_caller : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@in τ_0_0) -> () // user: %4
  %4 = apply %3<τ_0_0>(%1) : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@in τ_0_0) -> ()
  destroy_addr %1 : $*τ_0_0                      // id: %5
  dealloc_stack %1 : $*τ_0_0                     // id: %6
  %7 = tuple ()                                   // user: %8
  return %7 : $()                                 // id: %8
} // end sil function 'test'

[atrick]

@gottesmm there's a value substitution that ignores the ownership of the apply argument. I'm not sure how your new verification caught that since it's got nothing to do with immutability. Let me know if you want me to debug that bad SILCombine. I think the problem is that getStackInitInst is called recursively through copy_addr instructions. It doesn't check whether it's a [copy] vs. [take] or whether the argument is @in vs. @in_guaranteed. If we ever start at an @in argument and look through a [copy] then we'll end up destroying the original value twice. There's a bunch of horrible logic already to make sure the original source value is still live and not overwritten before the apply (this optimization approach was always doomed). We could either add more logic to find and eliminate any extra destroy_addr instructions immediately after the apply, or we could just bail.

[gottesmm]

@atrick The way this was caught by my check is FSO changed this to in_guaranteed in a specific case.

[gottesmm]

@swift-ci smoke test

[gottesmm]

@swift-ci smoke test

[gottesmm]

@swift-ci smoke test

[gottesmm]

@swift-ci smoke test

[gottesmm]

@atrick I updated the PR with the fix for sil-combine.

[gottesmm]

@swift-ci smoke test

[atrick]

LGTM.

[atrick]

There's a failure in lldb/ExclusivityREPL test. Could verification be exposing a bug there too?

[gottesmm]

@swift-ci smoke test os x platform

[gottesmm]

@atrick I ran the tests locally and I am seeing:

Testing Time: 295.52s

---

Failing Tests (1):
LLDB :: SwiftREPL/BreakpointSimple.test

Maybe the lldb tests are flaky now? Not sure.

[gottesmm]

@swift-ci smoke test

[gottesmm]

@swift-ci smoke test