Closed
Bug 1321122
Opened 8 years ago
Closed 8 years ago
Baldr: add Module.customSections
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(2 files)
66.16 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
11.02 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
As added here: https://github.com/WebAssembly/design/pull/877
Assignee | ||
Comment 1•8 years ago
|
||
Overall, pretty easy. Because this patch ended up wanting the ModuleEnvironment threaded everywhere (so it could be passed to Decoder::startSection()), I ended up moving the remaining dataSegments/funcNames Vectors too. This simplified everything but involved moving the name-section code from WasmCompile.cpp into WasmValidate.cpp. I realized after that this should've been a separate patch, but that part is just code motion.
Attachment #8816359 -
Flags: review?(bbouvier)
Assignee | ||
Comment 2•8 years ago
|
||
This patch moves the start(Custom)Section funcs from WasmValidate.h to .cpp and properly comments and whitespaces them, since they are rather hefty.
Attachment #8816361 -
Flags: review?(bbouvier)
Comment 3•8 years ago
|
||
Comment on attachment 8816359 [details] [diff] [review] add-custom-sections Review of attachment 8816359 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, thank you! ::: js/src/wasm/WasmJS.cpp @@ +717,5 @@ > + JS::DeflateStringToUTF8Buffer(flat, RangedPtr<char>(name.begin(), name.length())); > + } > + > + AutoValueVector elems(cx); > + if (!elems.reserve(module->metadata().customSections.length())) This is a pretty pessimistic memory pre-allocation, should we just append() in the loop instead? @@ +725,5 @@ > + RootedArrayBufferObject buf(cx); > + for (const CustomSection& sec : module->metadata().customSections) { > + if (name.length() != sec.name.length) > + continue; > + if (memcmp(name.begin(), bytecode + sec.name.offset, name.length())) Is the section name in the bytecode also ensured to be encoded in utf8? The spec doesn't seem to say anything about it... ::: js/src/wasm/WasmValidate.h @@ +716,5 @@ > > MOZ_MUST_USE bool > DecodeLocalEntries(Decoder& d, ModuleKind kind, ValTypeVector* locals); > > +// Decoding a DecodeModuleEnvironment decodes all sections up to the code nit: Calling DecodeModuleEnvironment decodes ...
Attachment #8816359 -
Flags: review?(bbouvier) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8816361 [details] [diff] [review] out-of-line Review of attachment 8816361 [details] [diff] [review]: ----------------------------------------------------------------- Much better and cleaner, thanks.
Attachment #8816361 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #3) Thanks! > This is a pretty pessimistic memory pre-allocation, should we just append() > in the loop instead? Yes, good point. > Is the section name in the bytecode also ensured to be encoded in utf8? The > spec doesn't seem to say anything about it... It's kinda stated "for all names" in https://github.com/WebAssembly/design/blob/master/Web.md#names.
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/42ec15a11e13 Baldr: add Module.customSections (r=bbouvier)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8816359 [details] [diff] [review] add-custom-sections Approval Request Comment [Feature/Bug causing the regression]: new feature in spec, not regression [User impact if declined]: missing feature in spec in initial release [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: small and self-contained feature
Attachment #8816359 -
Flags: approval-mozilla-aurora?
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42ec15a11e13
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 9•7 years ago
|
||
Comment on attachment 8816359 [details] [diff] [review] add-custom-sections wasm update for aurora52
Attachment #8816359 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8816359 [details] [diff] [review] add-custom-sections Oh dear, all the righteous validation refactoring has made this patch a major pain to backport and more risky (since the patch would need significant changes). Since this is a relatively minor feature, I think it's probably best for this one to ride the trains with 53.
Flags: needinfo?(luke)
Attachment #8816359 -
Flags: approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•