Closed
Bug 1258346
Opened 8 years ago
Closed 8 years ago
[wasm] Coverity error handling issue
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jorendorff, Unassigned)
Details
Attachments
(1 file)
19.37 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
________________________________________________________________________________________________________
*** CID 1357068: Error handling issues (CHECKED_RETURN)
/js/src/asmjs/WasmBinaryToText.cpp: 1145 in RenderSignature(WasmRenderContext &, const js::wasm::DeclaredSig &, bool)()
1139 return false;
1140 }
1141 } else if (paramsNum > 0) {
1142 if (!c.buffer.append(" (param"))
1143 return false;
1144 for (uint32_t i = 0; i < paramsNum; i++) {
>>> CID 1357068: Error handling issues (CHECKED_RETURN)
>>> Calling "append" without checking return value (as is done elsewhere 71 out of 73 times).
1145 c.buffer.append(" ");
1146 ValType arg = sig.arg(i);
1147 if (!RenderValType(c, arg))
1148 return false;
1149 }
1150 if (!c.buffer.append(")"))
Comment 1•8 years ago
|
||
Attachment #8732846 -
Flags: review?(jorendorff)
Comment 2•8 years ago
|
||
Forgot to say: this file is supposed to change a lot pretty soon anyways, so this patch is fine as a workaround in the meanwhile, until we get to decide a more concise way to check these calls.
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8732846 [details] [diff] [review] checkappend.patch Review of attachment 8732846 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thanks. ::: js/src/asmjs/WasmBinaryToText.cpp @@ +436,3 @@ > switch (expr) { > + case Expr::I32Clz: success = c.buffer.append("i32.clz"); break; > + case Expr::I32Ctz: success = c.buffer.append("i32.ctz"); break; Could use a temporary const char* variable for these, instead of having lots of calls to append(). (I only very recently learned that when you have switch statement where every case just populates a variable from a constant, the switch can be compiled to an array lookup, instead of a jump through a jump table. That made me smile.)
Attachment #8732846 -
Flags: review?(jorendorff) → review+
Comment 4•8 years ago
|
||
Thank you for the review!
> ::: js/src/asmjs/WasmBinaryToText.cpp
> @@ +436,3 @@
> > switch (expr) {
> > + case Expr::I32Clz: success = c.buffer.append("i32.clz"); break;
> > + case Expr::I32Ctz: success = c.buffer.append("i32.ctz"); break;
>
> Could use a temporary const char* variable for these, instead of having lots
> of calls to append().
>
> (I only very recently learned that when you have switch statement where
> every case just populates a variable from a constant, the switch can be
> compiled to an array lookup, instead of a jump through a jump table. That
> made me smile.)
wow, that'd be a pretty sweet and efficient refactoring! If only there was a variant of StringBuffer.append that would take const char* (the variant used in this patch uses template<ArrayLength> append(const char (&array)[ArrayLength])). Unless we also specify the size in which case (bleh), I don't see a way to do this with the const char* :/
Will keep the patch this way and land it tomorrow, unless somebody has a better idea.
Reporter | ||
Comment 5•8 years ago
|
||
Fine with me.
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c4beb0ec23a6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•