Closed
Bug 1318252
Opened 8 years ago
Closed 8 years ago
Baldr: serialize Import's DefinitionKind
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(1 file)
1.32 KB,
patch
|
bbouvier
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Otherwise, all imports are treated as function imports after deserialization.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
status-firefox52:
--- → affected
Updated•8 years ago
|
Priority: -- → P1
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
(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)
Assignee | ||
Comment 5•8 years ago
|
||
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?
Assignee | ||
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a60c2cde52bf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6527a5a10d0
You need to log in
before you can comment on or make changes to this bug.
Description
•