DI: self-consumed analysis rework, volume 2 #12454
Conversation
slavapestov
force-pushed
the
di-self-consumed-analysis-volume-2
branch
from
a0a125b to
0b1f941
Compare
|
@swift-ci Please smoke test |
slavapestov
force-pushed
the
di-self-consumed-analysis-volume-2
branch
3 times, most recently
from
87549a0 to
bdc687d
Compare
|
@gottesmm Executable tests now pass, and the crashes in the JIRAs I referenced in the PR description no longer crash. I need to add more tests though, and update some existing SIL test cases, since code generation patterns changed a bit. |
|
Found another dupe in JIRA :) |
slavapestov
force-pushed
the
di-self-consumed-analysis-volume-2
branch
from
5874f71 to
44a8e02
Compare
slavapestov
force-pushed
the
di-self-consumed-analysis-volume-2
branch
2 times, most recently
from
18fad41 to
e69c864
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@gottesmm Can you review this PR now? |
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
@slavapestov Doing it now. |
@harlanhaskins any ideas? |
|
Got a link to the specific failure? Likely a file importing Module.h that didn’t declare the syntax headers as a dependency. |
There was a problem hiding this comment.
Slava, I am just going to do post commit. I want to ensure you can make progress. I have a few comments so far, but most are about adding documentation and why you are able to make assumptions. I was able to understand why, but I had to think about it. I shouldn't have to think about why something is correct when certain assumptions are made in the source. It should be explicit either via asserts or comments.
| if (Op->getOperandNumber() == 1) { | ||
| // The initial store of 'self' into the box at the start of the | ||
| // function. Ignore it. | ||
| if (auto *Arg = dyn_cast<SILArgument>(SI->getSrc())) |
There was a problem hiding this comment.
I don't think this is safe. What if you have multiple stores of the argument to the box in the function?
There was a problem hiding this comment.
Oops, I didn't update this singular comment. This is a case where you should comment that you know that the argument is owned and can only be consumed once since otherwise the ownership verifier would blow up. That being said, you should probably add an assert that you only see one of these while iterating over the loop.
In a throwing or failable initializer for a class, the typical pattern
is that an apply or try_apply consumes the self value, and returns
success or failure. On success, a new self value is produced.
On failure, there is no new self value. In both cases, the original
self value no longer exists.
We used to model this by attempting to look at the apply or try_apply
instruction, and figure out from subsequent control flow which
successor block was the success case and which was the error case.
The error blocks were marked as such, and a dataflow analysis was used
to compute whether 'self' had been consumed in each block reachable
from the entry block.
This analysis was used to prevent invalid use of 'self' in catch
blocks when the initializer delegation was wrapped in do/catch;
more importantly, it was also used to know when to release 'self'
on exit from the initializer.
For example, when we 'throw e' here, 'self' was already consumed
and does not need to be released -- doing so would cause a crash:
do {
try self.init(...)
} catch let e {
// do some other cleanup
throw e
}
On the other hand, here we do have to release 'self', otherwise we
will exit leaking memory:
do {
try someOtherThing()
self.init(...)
} catch let e {
// do some other cleanup
throw e
}
The problem with the old analysis is that it was too brittle and did
not recognize certain patterns generated by SILGen. For example, it
did not correctly detect the failure block of a delegation to a
foreign throwing initializer, because those are not modeled as a
try_apply; instead, they return an Optional value.
For similar reasons, we did not correctly failure blocks emitted
after calls to initializers which are both throwing and failable.
The new analysis is simpler and more robust. The idea is that in the
success block, SILGen emits a store of the new 'self' value into
the self box. So all we need to do is seed the dataflow analysis with
the set of blocks where the 'self' box is stored to, excluding the
initial entry block.
The new analysis is called 'self initialized' rather than 'self
consumed'. In blocks dominated by the self.init() delegation,
the result is the logical not of the old analysis:
- If the old analysis said self was consumed, the new one says self
is not initialized.
- If the old analysis said self was not consumed, the new analysis
says that self *is* initialized.
- If the old analysis returned a partial result, the new analysis
will also; it means the block in question can be reached from
blocks where the 'self' box is both initialized and not.
Note that any blocks that precede the self.init() delegation now
report self as uninitialized, because they are not dominated by
a store into the box. So any clients of the old analysis must first
check if self is "live", meaning we're past the point of the
self.init() call. Only if self is live do we then go on to check
the 'self initialized' analysis.
This changes code generation a bit, because now the conditional state bitmap uses a bit to track if the 'self' box was stored, not if the 'self' value was consumed. In some cases, this eliminates an extra bit, in other places it introduces an extra bit, but it really doesn't matter because LLVM will optimize this bit manipulation easily.
…m definite_init_failable_initializers.swift
…lass-constrained protocol extensions
slavapestov
force-pushed
the
di-self-consumed-analysis-volume-2
branch
from
e69c864 to
196559e
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
1 similar comment
|
@swift-ci Please test source compatibility |
Thrilling sequel to #12445.
Fixes rdar://problem/28687665, https://bugs.swift.org/browse/SR-1714, https://bugs.swift.org/browse/SR-3132, https://bugs.swift.org/browse/SR-3637, https://bugs.swift.org/browse/SR-6067, https://bugs.swift.org/browse/SR-6171.