[sil-capture-promotion] Properly handle generic SILBoxTypes #8420
Conversation
|
@swift-ci please test |
|
@slavapestov Does this fix make sense? |
| { | ||
| Lowering::GenericContextScope GenericScope( | ||
| F->getModule().Types, paramBoxTy->getLayout()->getGenericSignature()); | ||
| auto ¶mTL = F->getModule().Types.getTypeLowering(paramBoxedTy); |
There was a problem hiding this comment.
Do we not have a getTypeLowering() method on SILFunction itself?
There was a problem hiding this comment.
Are you sure the context is needed? The box should know how to lower itself without you pushing it's generic signature. @jckarter
There was a problem hiding this comment.
The box knows how to lower itself, yes. But it returns a lowered SILType.
And we need this TypeLowering paramTL , because the code that follows uses it in conditional statements like:
if (paramTL.isFormallyPassedIndirectly()) {
convention = ParameterConvention::Indirect_In;
} else if (paramTL.isTrivial()) {
convention = ParameterConvention::Direct_Unowned;
} else {
convention = ParameterConvention::Direct_Owned;
That's why a used an explicit generic context to get the TypeLowering, as this is what was used before. I was not sure how to call isFormallyPassedIndirectly directly on SILType, because it asks for a GenericSignature and ResilienceExpansion.
|
@jckarter I'm not qualified to review this -- can you take a quick look? |
|
@swiftix Is there no test case and you noticed this by inspection, or...? |
|
@slavapestov No, it is a crasher on an external project. |
swiftix
force-pushed
the
fix-rdar-31309883
branch
from
8486648 to
7ce5860
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
swiftix
changed the title
| { | ||
| Lowering::GenericContextScope GenericScope( | ||
| F->getModule().Types, paramBoxTy->getLayout()->getGenericSignature()); | ||
| auto ¶mTL = F->getTypeLowering(paramBoxedTy); |
There was a problem hiding this comment.
paramBoxTy->getFieldType ought to give you a type in the surrounding generic context. You shouldn't need to push the box layout's generic signature as a context.
There was a problem hiding this comment.
@jckarter I think I found the actual issue. The current code in the master uses:
auto ¶mTL = F->getModule().Types.getTypeLowering(paramBoxedTy);
whereas the right thing to do would be:
auto ¶mTL = F->getTypeLowering(paramBoxedTy);
SILFunction:: getTypeLowering actually pushes the right generic context, etc.
I'll try to come up with a reduced test-case.
|
@swiftix This is why a reduced test case would be great -- then we can look at it and figure out what the correct behavior should be. At this stage I don't think either one of us is comfortable with this change. |
swiftix
force-pushed
the
fix-rdar-31309883
branch
from
7ce5860 to
1ec6a8e
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@jckarter @slavapestov Added a test case and simplified the code. Does it look OK now? |
|
|
||
| // CHECK-LABEL: sil @_T023generic_promotable_box2Tf2nni_n : $@convention(thin) <T> (@in R<T>, @in Builtin.Int32, @owned E<(R<T>) -> Builtin.Int32>) -> () | ||
| // CHECK: bb0(%0 : $*R<T>, %1 : $*Builtin.Int32, %2 : $E<(R<T>) -> Builtin.Int32>): | ||
| // CHECK-NOT: project_box |
There was a problem hiding this comment.
I think a CHECK: line that ensures the promoted stack slot has type $*E<(R<T>)->Int> would be a good idea. LGTM otherwise.
- Use SILFunction::getTypeLowering, because it knows how to handle generic contexts properly - Map interface types into context - Fix a use-after-release runtime bug, uncovered by recent Erik's changes. Fixes rdar://31309883.
swiftix
force-pushed
the
fix-rdar-31309883
branch
from
1ec6a8e to
7887eb3
Compare
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
Fixes rdar://31309883.