SILOptimizer: add a diagnostics pass to warn about lifetime issues with weak references.

[eeckstein]

The DiagnoseLifetimeIssuesPass pass prints a warning if an object is stored to a weak property (or is weakly captured) and destroyed before the property (or captured reference) is ever used again.
This can happen if the programmer relies on the lexical scope to keep an object alive, but copy-propagation can shrink the object's lifetime to its last use.
For example:

  func test() {
    let k = Klass()
      // k is deallocated immediately after the closure capture (a store_weak).
      functionWithClosure({ [weak k] in
                            // crash!
                            k!.foo()
                          })
    }

Unfortunately this pass can only catch simple cases, but it's better than nothing.

rdar://73910632

[eeckstein]

@swift-ci test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 514bd26f913d6ea4da2ad34294b9f39c79be0fc2

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 514bd26f913d6ea4da2ad34294b9f39c79be0fc2

[eeckstein]

@swift-ci test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - d37a3529e2e7da265806e3cf46a4edb117114691

[atrick]

@eeckstein We should probably have another iteration of this diagnostic after you merge this that recurses through all ForwardingConsumes, not just Enum. There is a ForwardingOperand abstraction that's close to what you need for that. Without that, I think we could have false positive warnings.

[atrick]

We may also want to be more conservative in the OperandOwnership::DestroyingConsume case. That only destroys the OSSA value, but could actually forward the reference through a store.

[atrick]

I'm concerned about false positive warnings in two case: ForwardingConsume and DestroyingConsume. For forwarding operations, we should be able to recognize most and look through them. For DestroyingConsume, the question is how conservative we want to be for stores, calls, and things we don't recognize. It may be good enough to just warn when the only destroys are actual Destroys.

I spent most of my time reviewing just trying to figure out what the Demangle nodes really mean. I could not find documentation anywhere in the source. I can guess what they mean but it would be helpful if you add comments with precise definitions.

It's up to you whether you want to address improvements in another PR later.

[atrick]

@eeckstein in general this looks really great!

[atrick]

@swift-ci smoke test linux

[atrick]

@swift-ci smoke test windows

[eeckstein]

It may be good enough to just warn when the only destroys are actual Destroys.

You are right. I'll change that

Update: Actually that's the case right now. Everything else than destroy_values are considered as "normal" uses and extend the lifetime, which prevents the warning.

[eeckstein]

@swift-ci smoke test linux

[eeckstein]

@swift-ci smoke test linux

[atrick]

@eeckstein I'm referring to cases like this

%0 = alloc_ref
%copy = copy_value %0
store %copy to ...
store_weak %0 to ...

or

%0 = alloc_ref
%copy = copy_value %0
%cast = cast %copy
store_weak %0 to ...
... = %cast

[eeckstein]

Ah, you are right. I fixed this.

[eeckstein]

@swift-ci smoke test

[atrick]

Thanks. It would be pretty easy to handle casts at some point.