[rbi] Fix demangling of sending results. #78612
Conversation
…}() propagates result options. I discovered this while trying to figure out why we were dropping a sending bit when computing a reabstraction thunk in the tests for the following commit. This turned out to be the reason why.
gottesmm
force-pushed
the
pr-d09f63ed212874b52ff15e2838b55f7462908323
branch
from
7e799e9 to
62838d5
Compare
gottesmm
force-pushed
the
pr-d09f63ed212874b52ff15e2838b55f7462908323
branch
2 times, most recently
from
3a6a5f5 to
ebd076d
Compare
|
@swift-ci smoke test |
The issue here is that the demangler (since we have a postfix mangling) parses parameters/results/etc and then uses earlier postfix type arguments to attach the relevant types to the parameters/results/etc. Since the flag for a sending result was placed in between the parameters and results, we get an off by one error. Rather than fix that specific issue by introducing an offset for the off by one error, I used the fact that the impl-function part of the mangling is not ABI and can be modified to move the bit used to signify a sending result to before the parameters so the whole problem is avoided. I also while I was doing this looked through the sending result mangling for any further issues and fixed them as I found them. rdar://141962865
gottesmm
force-pushed
the
pr-d09f63ed212874b52ff15e2838b55f7462908323
branch
from
ebd076d to
cb2d756
Compare
|
@swift-ci smoke test |
| // E.x.: | ||
| // | ||
| // func foo() -> sending X // No mangling | ||
| // func bar(_ x: () -> sending X) {} // Add to mangling for x |
There was a problem hiding this comment.
This comment implies that impl-function-type mangling is ABI, and it should probably be removed
There was a problem hiding this comment.
Yes. Sorry! I forgot to remove it.
| } | ||
|
|
||
| /// Return a version of this result info with the convention replaced. | ||
| SILResultInfo getWithConvention(ResultConvention c) const { | ||
| return SILResultInfo(getInterfaceType(), c); | ||
| return SILResultInfo(getInterfaceType(), c, getOptions()); |
There was a problem hiding this comment.
I noticed this when I was preparing the test cases for this PR. For some reason we were generating the thunks I was not expecting (since we were losing the sending result bit) and then due to the adjustFunctionType code generation, we were recovering. I tracked down that this was occurring due to SILGenPoly using this API and found this thinko. It didn't change any actual test output though, so I think we have been getting lucky.
| @@ -1093,7 +1093,14 @@ class TypeDecoder { | |||
| flags = flags.withSendable(); | |||
| } else if (child->getText() == "@async") { | |||
| flags = flags.withAsync(); | |||
| } else if (child->getText() == "sending-result") { | |||
There was a problem hiding this comment.
this is part of the code that I added that handles the legacy demangler code.
| } | ||
| } else if (child->getKind() == NodeKind::ImplSendingResult) { |
There was a problem hiding this comment.
This block is needed of course since we were not handling ImplSendingResult at all. The reason why it being not there was not an issue is b/c we also place a bit on the individual result info... so we got the appropriate bits from there.
| @@ -677,6 +677,10 @@ Type ASTBuilder::createImplFunctionType( | |||
| auto type = result.getType()->getCanonicalType(); | |||
| auto conv = getResultConvention(result.getConvention()); | |||
| auto options = *getResultOptions(result.getOptions()); | |||
| // We currently set sending result at the function level, but we set sending | |||
| // result on each result. | |||
| if (flags.hasSendingResult()) | |||
There was a problem hiding this comment.
This ensures that we maintain the invariant that I mentioned above that if the flags have a sending result, we also set the options bit on each SILResultInfo to mark them as sending.
| // E.x.: | ||
| // | ||
| // func foo() -> sending X // No mangling | ||
| // func bar(_ x: () -> sending X) {} // Add to mangling for x |
There was a problem hiding this comment.
Yes. Sorry! I forgot to remove it.
[rbi] Fix demangling of sending results.
The issue here is that the demangler (since we have a postfix mangling) parses parameters/results/etc and then uses earlier postfix type arguments to attach the relevant types to the parameters/results/etc. Since the flag for a sending result was placed in between the parameters and results, we get an off by one error.
The reason why I originally did it this way is that part of sending was cargo culted from the autodiff parts of the mangling and parts from the async/isolated(any) part of the function type mangling. In this case, I followed the location of the autodiff part of the mangling (which is per parameter) instead of the async/isolated(any) part of the mangling (which are global flags) which is what exposed this issue.
The good thing is if we eventually allow for sending to be placed on individual parameters, we can potentially eliminate seeing this patter in future versions of the ABI by just placing sending onto the parameters instead. This would additionally allow for us to eliminate this offset code by checking for the flag where it is and in such a case, just place it on each parameter as a flag.
rdar://141962865