Closed Bug 1258346 Opened 8 years ago Closed 8 years ago

[wasm] Coverity error handling issue

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jorendorff, Unassigned)

Details

Attachments

(1 file)

________________________________________________________________________________________________________
*** 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(")"))
Attachment #8732846 - Flags: review?(jorendorff)
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.
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+
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.
Fine with me.
https://hg.mozilla.org/mozilla-central/rev/c4beb0ec23a6
Status: NEW → 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.