[M] Change in dart/sdk[main]: [dart2wasm] Fix dummy initializations of cyclic types
Ömer Ağacan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 2 comments![]( "Open in Gerrit")
.withNullability(classIsCyclic),I think this is kind of a workaround.
So the core issue is that the receiver class is not instantiatable. If so, then we shouldn't compile any instance members of that class in the first place, so we shouldn't come here.
How about we modify the `visitConstructorInvocation` (which is the only place that can instantiate the class - as we cannot have cyclic constants) to evaluate the arguments and then emit an `b.unreachable()` instead of a call to the constructor. That will make us never compile the constructor, never mark the class allocated, which will make us never enqueue any instance members, ...
Then we don't have to have this workaround here.
dartType = dartType.withDeclaredNullability(Nullability.nullable);If the class is cyclic, then it cannot be instantiated. If it cannot be instantiated we cannot have values of it. If we don't have values of it, then this is actually bottom type (which doesn't have values).
Though the rest of compiler may need to create wasm locals for such a variable and never assign to them.
So I'd think we can use any type here, so I'd just use a very simple type that has least overhead - e.g. nullable top type.
Would that be possible?
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
.withNullability(classIsCyclic),I think this is kind of a workaround.
So the core issue is that the receiver class is not instantiatable. If so, then we shouldn't compile any instance members of that class in the first place, so we shouldn't come here.
How about we modify the `visitConstructorInvocation` (which is the only place that can instantiate the class - as we cannot have cyclic constants) to evaluate the arguments and then emit an `b.unreachable()` instead of a call to the constructor. That will make us never compile the constructor, never mark the class allocated, which will make us never enqueue any instance members, ...
Then we don't have to have this workaround here.
Yeah, generating `unreachable` could work in Wasm because stack overflows are also traps.
But in general you can't replace stack overflows with traps as stack overflows can be caught in Dart.
We can't make stack overflows catchable in Dart in dart2wasm though so it doesn't matter for us.
I'll try compiling constructors as `unreachable`.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
.withNullability(classIsCyclic),Ömer AğacanI think this is kind of a workaround.
So the core issue is that the receiver class is not instantiatable. If so, then we shouldn't compile any instance members of that class in the first place, so we shouldn't come here.
How about we modify the `visitConstructorInvocation` (which is the only place that can instantiate the class - as we cannot have cyclic constants) to evaluate the arguments and then emit an `b.unreachable()` instead of a call to the constructor. That will make us never compile the constructor, never mark the class allocated, which will make us never enqueue any instance members, ...
Then we don't have to have this workaround here.
Yeah, generating `unreachable` could work in Wasm because stack overflows are also traps.
But in general you can't replace stack overflows with traps as stack overflows can be caught in Dart.
We can't make stack overflows catchable in Dart in dart2wasm though so it doesn't matter for us.
I'll try compiling constructors as `unreachable`.
@kuste...@google.com we can avoid allocating the members and constructor but the locals will remain, and they need to be initialized somehow. For example:
```
class Foo {
final Foo next;
Foo(this.next);
}
final Foo global = Foo(global);
bool get runtimeTrue => int.parse('1') == 1;
void test() {
Foo foo; // <------- HERE
if (runtimeTrue) {
foo = global;
} else {
throw 'error';
}
print(foo);
print(foo.next);
}
```Here `foo` will need a dummy initializer.
Note that do determine whether a type is cyclic we rely on Wasm structs to be generated. We could do the check on Dart but I wonder if that would be fragile, maybe class A refers to B but in Wasm sturct the type is actually `ref null #Top` or something, because of a subclass overriding the reference, or something like that?
So I feel like checking for cyclic types in Wasm makes more sense. It's easy to get right.
If we generate Wasm structs as before, we'll have to keep these changes to make variables of them nullable, or we could try something like:
- Create structs as before
- After all is created, check for cyclic structs, replace them with empty structs (drop the fields)
With this all structs will be dummy instantiable.
Thoughts?
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
Yes, for locals we need a solution & I mentioned that in the other comment in `translator.dart`.
A local of a type that cannot be instantiated will never receive a value. That means the type of that local could be bottom (which has no values). But because we create locals, possibly dummy initialize and then never read/write to them, I'd suggest to make them nullable top type.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
OK, that makes sense.
I've made progress but there seems to be a bug when compiling called functions, I think `FunctionCollector.getFunction` doesn't add member functions to pending allocations, it adds them to the compilation queue directly. So we can't avoid compiling instance members by avoiding compiling constructors. This needs to be fixed first if we want to keep the dummy value allocation for `this` locals. Or we could keep the work around when initializing `this` locals for now.
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
I think this happens when we devirtualize calls to instance members. This can happen in dynamic call forwarders as well as devirtualized method calls from TFA.
If we treated the uninstantiable class just like abstract classes, then not give them entries in dispatch table, then the dynamic forwarding part is fixed.
That leave devirtualized call sites. They are basically dead code / unreachable because the receiver can never be instantiated. So one may just issue a `unreachable` instead of trying to get the wasm function and call it.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
There are also tear-offs that need to be special cased (similar to direct calls).
I think it would be easier to implement and maintain if we generated all referenced functions always, but leave the bodies as `unreachable` until we allocate their classes. (for top-level functions generate bodies always)
Am I right that `FunctionCollector._pendingAllocation` is not used right now?
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
Am I right that FunctionCollector._pendingAllocation is not used right now?
This is only needed for non-devirtualized calls: It tells the system: Someone has called `foo` selector - so if you ever allocate class X, then enqueue X.foo.
Yes, we could also just have bodies of instance members contain `unreachable`.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
dartType = dartType.withDeclaredNullability(Nullability.nullable);If the class is cyclic, then it cannot be instantiated. If it cannot be instantiated we cannot have values of it. If we don't have values of it, then this is actually bottom type (which doesn't have values).
Though the rest of compiler may need to create wasm locals for such a variable and never assign to them.
So I'd think we can use any type here, so I'd just use a very simple type that has least overhead - e.g. nullable top type.
Would that be possible?
Done
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 6 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
6 comments
bool isCyclic(Translator translator) {Since we're going to call this method for each instance member we try to compile, consider pre-calculating this when we create the class information.
We can do that after the `_generateFields()` call.
Then this doesn't have to be a public method and we don't have to warn about "not calling this method in certain situations".
if (info.isCyclic(translator)) {We still have to evaluate arguments first (which may actually have side effects, throw or do anything - so it's not unreachable here).
So this should move below before the call instruction.
We could alternatively also make the constructor body throw (just like with other instance members of the unreachable classes)
final member = memberReference.asMember;Could you add the same code as in `getInlinableMemberCodeGenerator` here as well?
final asyncMarker = lambda.functionNode.asyncMarker;Could you add an `assert(!translator.classInfo[enclosingMember.enclosingClass].isCyclic(translator))`
dartType = dartType.withDeclaredNullability(Nullability.nullable);If the class is cyclic, then it cannot be instantiated. If it cannot be instantiated we cannot have values of it. If we don't have values of it, then this is actually bottom type (which doesn't have values).
Though the rest of compiler may need to create wasm locals for such a variable and never assign to them.
So I'd think we can use any type here, so I'd just use a very simple type that has least overhead - e.g. nullable top type.
Would that be possible?
Could you try whether making this nullable top type is possible?
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
dartType = dartType.withDeclaredNullability(Nullability.nullable);Martin KustermannIf the class is cyclic, then it cannot be instantiated. If it cannot be instantiated we cannot have values of it. If we don't have values of it, then this is actually bottom type (which doesn't have values).
Though the rest of compiler may need to create wasm locals for such a variable and never assign to them.
So I'd think we can use any type here, so I'd just use a very simple type that has least overhead - e.g. nullable top type.
Would that be possible?
Could you try whether making this nullable top type is possible?
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
1 comment
dartType = dartType.withDeclaredNullability(Nullability.nullable);Martin KustermannIf the class is cyclic, then it cannot be instantiated. If it cannot be instantiated we cannot have values of it. If we don't have values of it, then this is actually bottom type (which doesn't have values).
Though the rest of compiler may need to create wasm locals for such a variable and never assign to them.
So I'd think we can use any type here, so I'd just use a very simple type that has least overhead - e.g. nullable top type.
Would that be possible?
Ömer AğacanCould you try whether making this nullable top type is possible?
What do you mean by "this"? `topType` is already nullable.
Ah, you already addressed it. (maybe I was looking at an old patch set)
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 5 comments![]( "Open in Gerrit")
bool isCyclic(Translator translator) {Since we're going to call this method for each instance member we try to compile, consider pre-calculating this when we create the class information.
We can do that after the `_generateFields()` call.
Then this doesn't have to be a public method and we don't have to warn about "not calling this method in certain situations".
Done
}@kuste...@google.com it's a bit strange that `Translator.classes` doesn't have mixins but `Translator.classesSupersFirst` does. Is this intentional? What's the reason for this?
if (info.isCyclic(translator)) {We still have to evaluate arguments first (which may actually have side effects, throw or do anything - so it's not unreachable here).
So this should move below before the call instruction.
We could alternatively also make the constructor body throw (just like with other instance members of the unreachable classes)
Done
final member = memberReference.asMember;Could you add the same code as in `getInlinableMemberCodeGenerator` here as well?
Done
final asyncMarker = lambda.functionNode.asyncMarker;Could you add an `assert(!translator.classInfo[enclosingMember.enclosingClass].isCyclic(translator))`
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
1 comment
@kuste...@google.com it's a bit strange that `Translator.classes` doesn't have mixins but `Translator.classesSupersFirst` does. Is this intentional? What's the reason for this?
So anonymous mixin application classes are not something that one can allocate or use in type expressions. As such we don't need to assign a class id to them.
The `Translator.classes` maps class-id to class, but I guess `Translator.classesSuperFirst` will include all of the classes.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 2 comments![]( "Open in Gerrit")
Martin Kustermann@kuste...@google.com it's a bit strange that `Translator.classes` doesn't have mixins but `Translator.classesSupersFirst` does. Is this intentional? What's the reason for this?
So anonymous mixin application classes are not something that one can allocate or use in type expressions. As such we don't need to assign a class id to them.
The `Translator.classes` maps class-id to class, but I guess `Translator.classesSuperFirst` will include all of the classes.
OK, documented.
final asyncMarker = lambda.functionNode.asyncMarker;Ömer AğacanCould you add an `assert(!translator.classInfo[enclosingMember.enclosingClass].isCyclic(translator))`
Done
This assertion is currently failing. I debugged it a little bit and on the way figured out how some of these cycles happen even when the original program is legit.
```
class _DetachedSocket extends Stream<Uint8List> implements Socket {
final Stream<Uint8List> _incoming;
final Socket _socket;
...
}
```
Note that this class is a `Socket` and has a non-nullable reference to `Socket`.
`Socket` is an abstract interface class: https://github.com/dart-lang/sdk/blob/1c6496fe9211f74a31ad9c51dcbc1ff318117c8e/sdk/lib/io/socket.dart#L836
Depending on the target there are a few implementations:
- https://github.com/dart-lang/sdk/blob/1c6496fe9211f74a31ad9c51dcbc1ff318117c8e/sdk/lib/_http/http_impl.dart#L3785
- VM only: https://github.com/dart-lang/sdk/blob/1c6496fe9211f74a31ad9c51dcbc1ff318117c8e/sdk/lib/_internal/vm/bin/socket_patch.dart#L2675
- https://github.com/dart-lang/sdk/blob/1c6496fe9211f74a31ad9c51dcbc1ff318117c8e/sdk/lib/io/secure_socket.dart#L10 this is also abstract so it must have subclasses somewhere
- https://github.com/dart-lang/sdk/blob/1c6496fe9211f74a31ad9c51dcbc1ff318117c8e/sdk/lib/io/secure_socket.dart#L259
But when compiling to Wasm we don't compile all of these and based no TFA results and other things we get this struct for `_DetachedSocket`:
```
_DetachedSocket{const ref _Type, const ref _HttpDetachedIncoming, const ref _DetachedSocket}
```
and then another type that uses this (hence cyclic):
```
_HttpClientConnection{..., const ref _DetachedSocket, ...}
```
and then this lambda inside the `_HttpClientConnection` constructor: https://github.com/dart-lang/sdk/blob/1c6496fe9211f74a31ad9c51dcbc1ff318117c8e/sdk/lib/_http/http_impl.dart#L2188
triggers the assertion.
I think this answers how the reporter of the original issue got this crash. You don't have to have a cycle in your own code, things get cyclic after TFA and maybe other transformations.
I don't know why we're trying to compile this lambda. I suspect closure collector needs to take reachability of the enclosing member into account as well.
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
final asyncMarker = lambda.functionNode.asyncMarker;Done
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |