Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[dart2wasm] Constructor body type mismatch #53506

Closed
eyebrowsoffire opened this issue Sep 12, 2023 · 1 comment
Closed

[dart2wasm] Constructor body type mismatch #53506

eyebrowsoffire opened this issue Sep 12, 2023 · 1 comment
Assignees
@jessicalally
Labels
area-dart2wasm Issues for the dart2wasm compiler.

Comments

@eyebrowsoffire
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/315820 has broken the flutter engine unit tests. When attempting to instantiate the module in Chrome, we get the following error:

Failed to load "geometry_test.dart": Failed to fetch and instantiate wasm module: CompileError: WebAssembly.compileStreaming(): Compiling function #4727:"Highlighter. constructor body" failed: call[0] expected type (ref 384), found local.get of type (ref 47) @+688087

When running wasm-opt -all --print on our wasm modules, we get the following validation error:

[wasm-validator error in function Highlighter.\20constructor\20body] call param types must match, on 
(call $Highlighter._\20constructor\20body
 (local.get $4)
 (local.get $5)
 (local.get $6)
)
(on argument 0)

So there appears to be some sort of type mismatch going on when using this constructor. My best guess is that this is the class that is attempting to be instantiated: https://github.com/dart-lang/source_span/blob/48d0f574ee0a92a241c865d47f461803a664b5ba/lib/src/highlighter.dart#L19

Although honestly I'm not sure how that dependency gets pulled in when compiling the web engine.
copybara-service bot pushed a commit that referenced this issue Sep 12, 2023
@eyebrowsoffire
Revert "[dart2wasm] Replace struct.new_default with struct.new fo…
67f0d4d 
…r object"

This reverts commit 5a4b252.

Reason for revert: Causing Flutter engine unit test failures, see #53506

Original change's description:
> [dart2wasm] Replace `struct.new_default` with `struct.new` for object
> allocation.
>
> When using the `struct.new_default` instruction for object allocation,
> fields are always nullable and mutable. By using the `struct.new`
> instruction instead, class fields can now have the same mutability and
> nullability in Wasm as declared in Dart. In addition, the class ID and
> type parameters (which are also stored in an object's struct), can now
> be immutable and nonnullable as well.
>
> To do this, object construction is now split into three functions:
> (1) Initializer: evaluates initializers for instance fields and
> constructor initializers (this constructor before super constructor).
> (2) Constructor body: executes the constructor body (super constructor
> before this constructor), with `this` pointed to the constructed object.
> (3) Constructor allocator: which calls (1), allocates the object using
> `struct.new`, then calls (2).
>
> Because fields now have the correct mutability and nullability in Wasm,
> this removes unnecessary null checks for nonnullable fields, and may
> allow for better optimisations by Binaryen.
>
> Fixes #51492
>
> Change-Id: Ib26046686f772a70509a870301217e9b1c91b77e
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315820
> Commit-Queue: Jess Lally <jessicalally@google.com>
> Reviewed-by: Aske Simon Christensen <askesc@google.com>

Change-Id: I034d3acf3715abadc6811a7393ba780bee974329
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/325445
Commit-Queue: Martin Kustermann <kustermann@google.com>
Commit-Queue: Jackson Gardner <jacksongardner@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
@vsmenon vsmenon added the area-dart2wasm Issues for the dart2wasm compiler. label Sep 13, 2023
@osa1
Copy link
Member

osa1 commented Sep 15, 2023

The commit that introduced the bug was reverted in 67f0d4d. Reland CL with the bug fix: https://dart-review.googlesource.com/c/sdk/+/326080.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jessicalally jessicalally

Labels
area-dart2wasm Issues for the dart2wasm compiler.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@osa1 @vsmenon @eyebrowsoffire @jessicalally