Fixes for exotic materializeForSet #15140
Conversation
|
@swift-ci Please smoke test |
| // CHECK-NEXT: [[TEMP2:%.*]] = alloc_stack $A | ||
| // SEMANTIC SIL TODO: This is an issue caused by the callback for materializeForSet in the class case taking the value as @inout when it should really take it as @guaranteed. |
There was a problem hiding this comment.
@gottesmm This comment doesn't make sense so I removed it. We cannot use @guaranteed convention for the 'self' parameter of the callback, because the callback for a property of a class type has to be ABI compatible with the callback for a protocol requirement witnessed by the class property. So it always has to be indirect.
There was a problem hiding this comment.
I meant to say @in_guaranteed here.
There was a problem hiding this comment.
Can you put back in the comment with that update.
There was a problem hiding this comment.
The callback takes it as @in_guaranteed now. However the subsequent alloc_stack / store_borrow remains, because we originally had a loaded value.
There was a problem hiding this comment.
I talked with Slava offline. He said that he fixed the underlying issue (this being inout instead of in_guaranteed). So it makes sense for the comment to go. Thanks Slava!
…-mutating setter I admit this is an odd corner case that is unlikely to ever come up in practice. Fixes <rdar://problem/32860203>.
…eForSet callback of non-mutating setter We cannot in general use @guaranteed here, otherwise classes will not be able to conform to protocols with mutable property requirements (or we could always open-code materializeForSet witness thunks for classes, but that has its own downsides so its not a clear win). Fixes <rdar://problem/36867783>.
slavapestov
force-pushed
the
materialize-for-set-fixes
branch
from
eb5a95b to
f7dd583
Compare
|
@swift-ci Please smoke test |
| @@ -964,7 +964,7 @@ MaterializeForSetEmitter::createSetterCallback(SILFunction &F, | |||
| } | |||
|
|
|||
| // The callback gets the address of 'self' at +0. | |||
| ManagedValue mSelf = ManagedValue::forLValue(self); | |||
| ManagedValue mSelf = ManagedValue::forUnmanaged(self); | |||
There was a problem hiding this comment.
I wonder if there is a more explicit API we could use depending on whether self is @inout or @in_guaranteed: forLValue and forBorrowedAddressRValue maybe (though we can have trivial r-values).
Not that this should block this commit.
|
@swift-ci Please smoke test Linux |
|
Will this fix https://bugs.swift.org/browse/SR-7176 as well or is that something else? |
We had a SIL verifier workaround for non-mutating setters, and used to crash if we had both a mutating getter and non-mutating setter.