Closed Bug 1318252 Opened 8 years ago Closed 8 years ago

Baldr: serialize Import's DefinitionKind

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

Otherwise, all imports are treated as function imports after deserialization.
Attached patch fix-wasm-importSplinter Review
Initially, I'd just like to get the fix landed and uplifted since some people are hitting this.  After that, obviously we need some wasm-specific caching tests since this is an example where the asm.js tests couldn't expose the bug.
Attachment #8811604 - Flags: review?(bbouvier)
Priority: -- → P1
Comment on attachment 8811604 [details] [diff] [review]
fix-wasm-import

Review of attachment 8811604 [details] [diff] [review]:
-----------------------------------------------------------------

Great catch too.

::: js/src/wasm/WasmModule.cpp
@@ +149,5 @@
>  Import::serialize(uint8_t* cursor) const
>  {
>      cursor = module.serialize(cursor);
>      cursor = field.serialize(cursor);
> +    cursor = WriteScalar<DefinitionKind>(cursor, kind);

I see this is not the first occurrence (it's done also in Sig::serialize), but using the enum class here doesn't specify the storage class it's using. Would it be better expliciting it, by making a conversion of kind to uint32_t, instead?
Attachment #8811604 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
That's a good question.  Unlike the other bug, which happened before the assumptions match, and thus had to be build/size/everything-invariant, everything after the Module::checkAssumptions can have any size it wants determined by a particular build.  So whatever representation the compiler picks here is fine since the same buildid will be writing and reading it.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60c2cde52bf
Baldr: Serialize Import's DefinitionKind (r=bbouvier)
Comment on attachment 8811604 [details] [diff] [review]
fix-wasm-import

Approval Request Comment
[Feature/regressing bug #]: since the beginning of this feature in wasm
[User impact if declined]: wasm modules fail to deserialize correctly
[Describe test coverage new/current, TreeHerder]: m-c
[Risks and why]: low, tiny change
Attachment #8811604 - Flags: approval-mozilla-aurora?
Note: for the aurora patch, the patch as posted should be used; in the m-c patch, the patched functions moved from WasmModule.cpp to WasmTypes.cpp.
https://hg.mozilla.org/mozilla-central/rev/a60c2cde52bf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8811604 [details] [diff] [review]
fix-wasm-import

bug in wasm, let's fix in aurora52
Attachment #8811604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: