Closed Bug 1624363 Opened 5 months ago Closed 4 months ago

Syntax and encoding alignment with V8's GC prototyping

Categories

(Core :: Javascript: WebAssembly, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

Attachments

(7 files)

This bug is for the syntax/binary encoding changes needed to align with WABT/V8's GC prototyping work. This is for the subset that we currently implement, and isn't about new features.

Depends on: 1612534
Summary: Syntax alignment with V8's GC prototyping → Syntax and encoding alignment with V8's GC prototyping
See Also: → 1625595

Depends on D69405

The encoding for ref/optref is left to its own commit as that implies a larger
change to rename the actual TypeCode.

Depends on D69406

Depends on D69407

WABT encodes type then flag in struct fields.

Depends on D69408

Depends on D69409

Depends on D69410

Duplicate of this bug: 1625660
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/6925085a3958
Vendor wat 0.1.14. r=lth
https://hg.mozilla.org/integration/autoland/rev/34b14f96f32c
Drop (gc_feature_optin). r=lth
https://hg.mozilla.org/integration/autoland/rev/5ee4bec9fb29
Add GcPrefix opcode space and align encoding with wat update. r=lth
https://hg.mozilla.org/integration/autoland/rev/44e1642ff102
Rename TypeCode::Ref to TypeCode::OptRef. r=lth
https://hg.mozilla.org/integration/autoland/rev/10af84a356f7
Decode struct flag after type in struct field. r=lth
https://hg.mozilla.org/integration/autoland/rev/293fc1055c2e
Fix duplicate identifier messages. r=lth
https://hg.mozilla.org/integration/autoland/rev/9a64ef57fd25
Update GC test syntax. r=lth

FTR, it appears this broke my build (on macOS), with a Rust compile failure:

 3:07.06    Compiling baldrdash v0.1.0 (/Users/jkew/mozdev/mozilla-unified/js/src/wasm/cranelift)
 3:07.57 error[E0599]: no variant or associated item named `OptRef` found for enum `bindings::low_level::TypeCode` in the current scope
 3:07.57    --> js/src/wasm/cranelift/src/bindings/mod.rs:112:27
 3:07.57     |
 3:07.57 112 |                 TypeCode::OptRef | TypeCode::AnyRef | TypeCode::FuncRef | TypeCode::NullRef => {
 3:07.90     |                           ^^^^^^ variant or associated item not found in `bindings::low_level::TypeCode`

After clobbering, it now seems to be building successfully, so I guess the push here should really have updated the CLOBBER file.

Flags: needinfo?(rhunt)

so I guess the push here should really have updated the CLOBBER file.

...or there's some kind of dependency missing from the build system.

(In reply to Jonathan Kew (:jfkthame) from comment #11)

FTR, it appears this broke my build (on macOS), with a Rust compile failure:

 3:07.06    Compiling baldrdash v0.1.0 (/Users/jkew/mozdev/mozilla-unified/js/src/wasm/cranelift)
 3:07.57 error[E0599]: no variant or associated item named `OptRef` found for enum `bindings::low_level::TypeCode` in the current scope
 3:07.57    --> js/src/wasm/cranelift/src/bindings/mod.rs:112:27
 3:07.57     |
 3:07.57 112 |                 TypeCode::OptRef | TypeCode::AnyRef | TypeCode::FuncRef | TypeCode::NullRef => {
 3:07.90     |                           ^^^^^^ variant or associated item not found in `bindings::low_level::TypeCode`

After clobbering, it now seems to be building successfully, so I guess the push here should really have updated the CLOBBER file.

Ah, I'm sorry about that! This seems like a potential issue with our use bindgen.

I changed a TypeCode in 'wasm/WasmConstants.h', which is included by 'baldrapi.h', which is processed by bindgen. It looks like the build.rs which triggers bindgen is set to only run if 'baldrapi.h' is changed. [1]

Benjamin, do we need to add 'WasmConstants.h' as a 'rerun-if-changed' file?

I also see bindgen has a 'builder.parse_callbacks(Box::new(bindgen::CargoCallbacks))' option which seems to automatically print rerun-if-changed for all includes. I don't see it used in any other bindgen projects in tree though, so I don't know if there's a reason for that.

[1] https://searchfox.org/mozilla-central/rev/4ccefc3181f9d237ef4ca8bd17b4e7c101ddf7b5/js/src/wasm/cranelift/build.rs#31

Flags: needinfo?(rhunt) → needinfo?(bbouvier)

Benjamin, do we need to add 'WasmConstants.h' as a 'rerun-if-changed' file?

Yes, this seems sensible. Sorry I didn't think about such a build failure possibility! (And it's a bit amazing it didn't happen earlier.) We could also try the parse_callbacks thing, if it worked.

Flags: needinfo?(bbouvier)

parse_callbacks(Box::new(CargoCallbacks)) should just work (tm), and if not I'd be interested in knowing why not :)

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/71042f6bc4f6
Require clobber for cbindgen update. a=CLOBBER
Blocks: 1627835
You need to log in before you can comment on or make changes to this bug.