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)

defect
Not set
normal

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.
Attached file retval.tar.gz
Attached is an example .html/.wasm that contains nested calls each of which has a return value.
Assignee: nobody → ydelendik
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+
Thanks!
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.
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45cc0fe2c022
Expose wasm function return value to Debugger.Frame. r=luke
https://hg.mozilla.org/mozilla-central/rev/45cc0fe2c022
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.