[Distributed] Correct handling ad-hoc requirements in non-final classes #80109
Conversation
ktoso
force-pushed
the
wip-reproduce-146101172
branch
from
e8485c7 to
a0ccbcb
Compare
ktoso
requested review from
AnthonyLatsis,
hborla,
slavapestov,
xedin,
eeckstein,
jckarter and
rjmccall
as code owners
ktoso
force-pushed
the
wip-reproduce-146101172
branch
2 times, most recently
from
d8dba5d to
b4ff64e
Compare
ktoso
force-pushed
the
wip-reproduce-146101172
branch
from
b4ff64e to
3c88726
Compare
ktoso
changed the title
|
@swift-ci please smoke test |
|
With some hints from @slavapestov I moved the witness generic signature fixing into However we have two very mysterious failures remaining: these are weird because the test is not very different than other remoteCall ones... And especially, the The failures of the two above are pretty mysterious because the backtrace is "nothing" so we must have messed up something real bad: The other one: The crashes are pretty nasty, as if we messed up where we jump - so the witnesses are somehow messed up, but unclear why only in those two tests, while other remoteCall roundtrip ones seemed fine hm. Stepping through the runtime bits of remoteCall etc isn't very informative sadly. Slava also suggested we may need to remove the newly added requirements from IR, but I'm not quite sure where to do that. Removing the @xedin @mikeash maybe you have some ideas what we might need to look into here, if you'd skim the change -- primarily inside the |
| @@ -762,14 +762,11 @@ struct FakeInvocationEncoder_recordArgument_wrongType: DistributedTargetInvocati | |||
| mutating func doneRecording() throws {} | |||
| } | |||
| struct FakeInvocationEncoder_recordArgument_missingMutating: DistributedTargetInvocationEncoder { | |||
| //expected-error@-1{{type 'FakeInvocationEncoder_recordArgument_missingMutating' does not conform to protocol 'DistributedTargetInvocationEncoder'}} | |||
| //expected-error@-1{{struct 'FakeInvocationEncoder_recordArgument_missingMutating' is missing witness for protocol requirement 'recordArgument'}} | |||
There was a problem hiding this comment.
I was debugging why the hell only the recordArgument changed behavior here and could not figure it out so far... all the checks are the same after all hm. May be a clue, I don't know yet.
This isn't wrong though, we still do miss the witness as expected after all -- since it has missing the mutating and logic to detect that still works after all hm
|
Ok even weirder, the Linux build fails (but is fine on macOS) like which did build fine on macOS, which makes me wonder what's up here... definitely more digging required. |
Reproduces #79318
The problem is that the protocol requirements for those "ad-hoc" requirements are declared as:
and then inside
EmitPolymorphicParameters::injectAdHocDistributedRequirementswe detect those few requirements that get this treatment, and we add theSuccess: SerializationRequirementconstraints.This works in debug mode, but as #79318 uncovered, it is not quite enough and will crash in release mode. Specifically, we add the witness information like this:
which seems is not enough in release mode (?) or something gets optimized away, but could not figure out what exactly would even be expected to "be" there.
The crash manifests as an assertion about an invalid requirement which is because we're missed to add (or... lost somehow, due to some optimization?) the
: Transferablethe inject was adding. For reference, here's where we end up triggering the assertion:We also notice that this problem does not happen when the type is a
struct, and it seems to be related with how the try_apply is made to the function because if we make (sorry example below hasremoteCall, but it is the same issue as theonReturn) the call to such method using a function ref:We fail; because we're missing the Transferable type information. If the call is made as a
class_methodcall like this (which happens in debug mode):This works because it seems the first thing the class method does is "get" that type information using a
conformsToProtocol...Not really sure what to explore further here. Key finding is that this is specifically a release mode issue, and that we're then missing the
t_0_2: Transferablebit that ourinjectAdHocDistributedRequirementswould have added. What could be different here in a release mode?rdar://146101172