Closed
Bug 1260696
Opened 8 years ago
Closed 8 years ago
Baldr: Update binary to (temporary) text rendering
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
Details
Attachments
(2 files)
19.01 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
Details | Diff | Splinter Review |
This updates BinaryToText to fully validate the entire test suite, to ensure that the binary to text conversion is a bijection (modulo names, spaces, etc.). To ensure this, I've slightly changed wasmEvalText by adding this line: str = wasmBinaryToText(wasmTextToBinary(str)); // then Wasm.instantiate(wasmTextToBinary(str), etc.) Also, it makes wasmBinaryToText raise the same errors that wasmTextToBinary would, so the above transformation can work. Thinking about it, maybe some of the error checking should go to Wasm.cpp rather than WasmTextToBinary.cpp in the first place... I know the text format is supposed to be short-lived, but it has lived long enough, and binary to text has proven useful for understanding fuzzbugs. Plus I think some additions in this patch will show useful for the future (e.g. rendering select, branches with values, i64, etc.). Plus Dan hasn't touched the binary to text transformation yet in the postorder patch. So it seems worth to have in tree.
Attachment #8736283 -
Flags: review?(luke)
Assignee | ||
Comment 1•8 years ago
|
||
For what it's worth, the patch used for testing.
Comment 2•8 years ago
|
||
Comment on attachment 8736283 [details] [diff] [review] fix.binary.text.patch Review of attachment 8736283 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks!
Attachment #8736283 -
Flags: review?(luke) → review+
Comment 3•8 years ago
|
||
Christian, do you suppose you could have your (most excellent) wasm fuzzer scripts use wasmBinaryToText() to print the binaries as text in the bug reports? It won't always work (in cases where the binary is invalid), but for (the majority, I think) of the cases it does, it'd both save the time of posting the binary testcase *and* save us the time of doing it locally.
Comment 4•8 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #3) > Christian, do you suppose you could have your (most excellent) wasm fuzzer > scripts use wasmBinaryToText() to print the binaries as text in the bug > reports? It won't always work (in cases where the binary is invalid), but > for (the majority, I think) of the cases it does, it'd both save the time of > posting the binary testcase *and* save us the time of doing it locally. Posting the testcase is done fully automatically and doesn't involve any manual action on our side. The only thing I could do is have the reproduction script *also* output the text representation and include that output in the bug report in addition to the binary file. Not sure how well that works in our process but I could try it out.
Assignee | ||
Comment 5•8 years ago
|
||
That would be great, but I assume this wouldn't work, most of the time: the test cases often have some invalid sections after the functions section, so showing the output would just throw an error when decoding the module. If you could make it that all sections be valid (esp. ensure section sizes are correct), that would be awesome!
Comment 6•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #5) > That would be great, but I assume this wouldn't work, most of the time: the > test cases often have some invalid sections after the functions section, so > showing the output would just throw an error when decoding the module. > > If you could make it that all sections be valid (esp. ensure section sizes > are correct), that would be awesome! We have no way to do that. The fuzzer we use doesn't generate valid wasm but uses simple binary mutations on existing wasm. The only way I could imagine this to work is by *you* providing a function that syntactically "fixes" given wasm binary data. We could call this function and see if the resulting test still reproduces the problem.
Comment 7•8 years ago
|
||
Ah, good point bbouvier. I was thinking about the runtime errors (which means validation succeeded), but these compile-time errors are indeed likely to have trash at the end. A related idea we already discussed is having the BinaryToText() do a best-effort printing when the input is invalid by printing up to the last valid bits. We could use this for validation error messages as well.
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf8dc00cc76f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•