1343581 - Baldr: make <return> value in Block scope display return value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[Luke Wagner [:luke]]

Attachment: retval.tar.gz

Attached is an example .html/.wasm that contains nested calls each of which has a return value.

Comment 2

[Yury Delendik (:yury)]

Attachment: Bug 1343581 - Expose wasm function return value to Debugger.Frame.

Review commit: https://reviewboard.mozilla.org/r/116728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116728/

Comment 3

[Luke Wagner [:luke]]

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?

Comment 4

[Luke Wagner [:luke]]

Thanks!

Comment 5

[Yury Delendik (:yury)]

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 6

[Yury Delendik (:yury)]

Comment on attachment 8842988 [details]
Bug 1343581 - Expose wasm function return value to Debugger.Frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116728/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45cc0fe2c022
Expose wasm function return value to Debugger.Frame. r=luke

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45cc0fe2c022