Closed
Bug 1425076
Opened 6 years ago
Closed 6 years ago
Add a console warning message when running .wasm content with old unsupported debug symbols format
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: jujjyl, Assigned: luke)
Details
Attachments
(2 files, 1 obsolete file)
5.87 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
42.37 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
Currently due to product release timelines, the latest version of Unity still produces .wasm files with old debugging section format, and as result they do not get callstacks as Firefox (and Chrome and others) are unable to parse the old format. This could cause frustration to users, as they have no way to understand what is going on here. It would be good to add a warning message when compiling (or instantiating(?)) such a module that has the old names section to console log that says "This WebAssembly Module was produced with old incompatible names section format. Function names will not be available." or "Unable to parse WebAssembly names section. Function names will not be available." That would allow Unity to offer a troubleshooting/FAQ item on their page about how to deal with the issue.
Assignee | ||
Comment 1•6 years ago
|
||
That's a good idea. We should just generally print a warning if any custom section fails to validate.
Updated•6 years ago
|
status-firefox59:
--- → fix-optional
Priority: -- → P3
Assignee | ||
Comment 2•6 years ago
|
||
Finally got to this in the backlog. I think this could be pretty useful from preventing a swamp of garbage name sections since people might actually notice and fix the errors. In addition to unit tests, I also tested on a build with known-bad names (https://s3.amazonaws.com/mozilla-games/ZenGarden/EpicZenGarden.html) and known-good (https://kripken.github.io/embenchen/wasm/names.html).
Assignee | ||
Comment 3•6 years ago
|
||
Unrelated, but I've been meaning to do this for a while.
Assignee: nobody → luke
Attachment #8965141 -
Flags: review?(bbouvier)
Assignee | ||
Comment 4•6 years ago
|
||
Pretty easy; mostly just piping a new Vector<UniqueChars> "warnings" array around everywhere we pipe the UniqueChars "error" message. Note: there are apparently no uses of assertWarning(), which is why I was able to change the signature.
Attachment #8965143 -
Flags: review?(bbouvier)
Assignee | ||
Comment 5•6 years ago
|
||
Updated so that unknown name subsections are skipped instead of warning. Otherwise, emitting a perfectly-legal locals subsection: https://webassembly.github.io/spec/core/appendix/custom.html#subsections which we don't currently decode would warn.
Attachment #8965520 -
Flags: review?(bbouvier)
Assignee | ||
Updated•6 years ago
|
Attachment #8965143 -
Attachment is obsolete: true
Attachment #8965143 -
Flags: review?(bbouvier)
Comment 6•6 years ago
|
||
Comment on attachment 8965141 [details] [diff] [review] rename-binary-iterator.h Review of attachment 8965141 [details] [diff] [review]: ----------------------------------------------------------------- Cool. ::: js/src/wasm/WasmOpIter.h @@ +16,5 @@ > * limitations under the License. > */ > > +#ifndef wasm_op_iter_h > +#define wasm_op_iter_h preexisting: please also fix #endif comment at the end of the file
Attachment #8965141 -
Flags: review?(bbouvier) → review+
Comment 7•6 years ago
|
||
Comment on attachment 8965520 [details] [diff] [review] warn-on-invalid-custom-section Review of attachment 8965520 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks. ::: js/src/jit-test/tests/wasm/binary.js @@ +453,5 @@ > +badNameSec3.body.pop(); > +assertWarning(() => wasmEval(moduleWithSections([v2vSigSection, declSec, bodySec, badNameSec3])), nameWarning); > +assertNoWarning(() => wasmEval(moduleWithSections([nameSection([moduleNameSubsection('hi')])]))); > +assertWarning(() => wasmEval(moduleWithSections([nameSection([moduleNameSubsection('hi'), moduleNameSubsection('boo')])])), nameWarning); > +assertNoWarning(() => wasmEval(moduleWithSections([nameSection([moduleNameSubsection('hi'), [4, 0]])]))); Can you add a comment above this test, like "unknown name subsection"? ::: js/src/wasm/WasmJS.cpp @@ +863,5 @@ > +static bool > +ReportCompileWarnings(JSContext* cx, const UniqueCharsVector& warnings) > +{ > + // Avoid spamming the console. > + size_t numWarnings = Min<size_t>(warnings.length(), 3); If warnings.length() > 3, maybe worth adding an ultimate message saying "other warnings found, not displayed to avoid spamming the console"? @@ +2330,5 @@ > } > > static bool > +Reject(JSContext* cx, const CompileArgs& args, Handle<PromiseObject*> promise, > + const UniqueChars& error) nit: previous order was more respectful of our rule to put in-out parameters at the end of the signature? (same happens for Resolve below, where a handle to importObj is passed before the warnings) ::: js/src/wasm/WasmValidate.cpp @@ +242,5 @@ > + warnf("in the '%s' custom section: %" PRIu32 " unconsumed bytes", > + name, uint32_t(range.size - actualSize)); > + else > + warnf("in the '%s' custom section: %" PRIu32 " bytes consumed past the end", > + name, uint32_t(actualSize - range.size)); nit: please use {} for if/else. @@ +244,5 @@ > + else > + warnf("in the '%s' custom section: %" PRIu32 " bytes consumed past the end", > + name, uint32_t(actualSize - range.size)); > + finishSkippedCustomSection(range); > + return; nit: useless return @@ +251,5 @@ > + // Nothing to do! (c.f. finishSkippedCustomSection()) > +} > + > +void > +Decoder::finishSkippedCustomSection(const SectionRange& range) nit: the name confused me and suggested that the section had been skipped already. How about skipAndFinishCustomSection instead? Thinking about it just a bit more: "skip" would even be enough. Maybe we could rename the current `skipCustomSection` to `skipAnyCustomSection` and name this one `skipCustomSection` instead?
Attachment #8965520 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #7) Thanks, good catches! > > +Reject(JSContext* cx, const CompileArgs& args, Handle<PromiseObject*> promise, > > + const UniqueChars& error) > > nit: previous order was more respectful of our rule to put in-out parameters > at the end of the signature? > > (same happens for Resolve below, where a handle to importObj is passed > before the warnings) I don't quite follow in both cases: 'promise' and 'importObj' are in-params (a MutableHandle would be in-out) nor are the referenced objects mutated in an indirect outparam-y way. > @@ +244,5 @@ > > + else > > + warnf("in the '%s' custom section: %" PRIu32 " bytes consumed past the end", > > + name, uint32_t(actualSize - range.size)); > > + finishSkippedCustomSection(range); > > + return; > > nit: useless return Technically, but that depends on the coincidental property that there is nothing left to do in the valid-custom-section case. Should something be added later, it would be easy to not notice the missing return. Another reason is symmetry with the preceding finishSkippedCustomSection() case.
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f589a76063 Baldr: rename WasmBinaryIterator to OpIter (r=bbouvier) https://hg.mozilla.org/integration/mozilla-inbound/rev/350013b0e02d Baldr: warn on invalid name section (r=bbouvier)
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9f589a76063 https://hg.mozilla.org/mozilla-central/rev/350013b0e02d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•