Closed
Bug 1343581
Opened 7 years ago
Closed 7 years ago
Baldr: make <return> value in Block scope display return value
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: luke, Assigned: yury)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
When stepping out of a function, there is a special step-point in which <return> is displayed in the Block scope. Right now this always displays "undefined" for wasm, but it'd be great if it had the real return value.
Reporter | ||
Comment 1•7 years ago
|
||
Attached is an example .html/.wasm that contains nested calls each of which has a return value.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ydelendik
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8842988 [details] Bug 1343581 - Expose wasm function return value to Debugger.Frame. https://reviewboard.mozilla.org/r/116728/#review118488 ::: js/src/wasm/WasmCode.h:676 (Diff revision 1) > bool decrementStepModeCount(JSContext* cx, uint32_t funcIndex); > > // Stack inspection helpers. > > bool debugGetLocalTypes(uint32_t funcIndex, ValTypeVector* locals, size_t* argsLength); > + void debugGetResultType(uint32_t funcIndex, ExprType* returnType); Since infallible, I'd make `returnType` be the return value, not an outparam. ::: js/src/wasm/WasmDebugFrame.cpp:96 (Diff revision 1) > + break; > + case ExprType::F32: > + cachedReturnJSValue_.setDouble((double)resultF32_); > + break; > + case ExprType::F64: > + cachedReturnJSValue_.setDouble(resultF64_); Because this resultF64 can be a non-canonical nan (which can then be interpreted as some ObjectValue b/c NaN-boxing), you need to CanonicalizeNaN(resultF32) as well as resultF64. I forgot this case when reviewing DebugFrame::getLocal(), could you fix it there too? ::: js/src/wasm/WasmGenerator.cpp:207 (Diff revision 1) > } > > if (metadata_->debugEnabled) { > - if (!debugFuncArgTypes_.resize(env_->funcSigs.length())) > + if (!debugFuncArgTypes_.resize(env_->funcSigs.length()) || > + !debugFuncReturnTypes_.resize(env_->funcSigs.length())) > return false; nit: multi-line condition needs { } around then-branch, but that's ugly so I'd rather have two if's. ::: js/src/wasm/WasmTypes.cpp:178 (Diff revision 1) > for (FrameIterator iter(activation, FrameIterator::Unwind::True); !iter.done(); ++iter) { > if (!iter.debugEnabled()) > continue; > > DebugFrame* frame = iter.debugFrame(); > + frame->clearReturnJSValue(); Just to check my understanding: this is just to catch bugs (use-after-pop of DebugFrame), right?
Attachment #8842988 -
Flags: review?(luke) → review+
Reporter | ||
Comment 4•7 years ago
|
||
Thanks!
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8842988 [details] Bug 1343581 - Expose wasm function return value to Debugger.Frame. https://reviewboard.mozilla.org/r/116728/#review118488 > Just to check my understanding: this is just to catch bugs (use-after-pop of DebugFrame), right? Correct. Also the Debugger::slowPathOnLeaveFrame also pulls frame's returnValue() -- this will define some initialized value for this field.
Comment hidden (mozreview-request) |
Pushed by ydelendik@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/45cc0fe2c022 Expose wasm function return value to Debugger.Frame. r=luke
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/45cc0fe2c022
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•