1294550 - Change AllFrameIter to return wasm frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Yury Delendik (:yury)]

Currently AllFrameIter returns only frames that have script object (non-wasm). Introducing debugging for wasm will require all to have iterator that supports wasm frames. It will be nice to rename the AllFrameIter to the AllScriptFrameIter (similar to NonBuiltinScriptFrameIter), while new AllFrameIter class will return wasm frames.

Comment 1

[Yury Delendik (:yury)]

Attachment: Bug 1294550 - Change AllFrameIter to return wasm frames.

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

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8780301 [details]
Bug 1294550 - Change AllFrameIter to return wasm frames.

https://reviewboard.mozilla.org/r/71014/#review68576

Thanks for looking at this.  So I guess the motivation here is that Debugger.cpp uses AllFramesIter so we need that to include wasm?

::: js/src/asmjs/WasmCode.h:523
(Diff revision 1)
>  
>      // Frame iterator support:
>  
>      const CallSite* lookupCallSite(void* returnAddress) const;
>      const CodeRange* lookupRange(void* pc) const;
> +    uint32_t pcToOffset(void* pc) const;

I'd like to hold off adding this until there is a specific need for it because right now it's totally clear if this is the right interface since we're just printing it.

::: js/src/asmjs/WasmFrameIterator.h:75
(Diff revision 1)
>      bool mutedErrors() const;
>      JSAtom* functionDisplayAtom() const;
>      unsigned lineOrBytecode() const;
> +    inline const Code* code() const { return code_; }
> +    inline void* fp() const { return fp_; }
> +    inline uint8_t* pc() const { return pc_; }

Is there a particular use case for the pc()/fp() yet or just printing?  I was hoping most of these stayed as internal impl details of wasm::FrameIterator and a more abstract interface would be exposed to the outside.

::: js/src/jsfriendapi.cpp:22
(Diff revision 1)
>  #include "jsprf.h"
>  #include "jswatchpoint.h"
>  #include "jsweakmap.h"
>  #include "jswrapper.h"
>  
> +#include "asmjs/WasmCode.h"

I expect these WasmCode.h #includes can be removed.

::: js/src/jsfriendapi.cpp:1017
(Diff revision 1)
>  {
>      int num = 0;
>  
>      for (AllFramesIter i(cx); !i.done(); ++i) {
> -        buf = FormatFrame(cx, i, buf, num, showArgs, showLocals, showThisProps);
> +        buf = i.hasScript() ? FormatFrame(cx, i, buf, num, showArgs, showLocals, showThisProps) :
> +                              FormatWasmFrame(cx, i, buf, num, showArgs);

Instead of ternary, I think it'd read a bit better to have:

  if (i.hasScript())
     buf = FormatFrame(...);
  else
     buf = FormatWasmFrame(...);

::: js/src/jsobj.cpp:3641
(Diff revision 1)
>      for (AllFramesIter i(cx); !i.done(); ++i, ++depth) {
> -        const char* filename = JS_GetScriptFilename(i.script());
> -        unsigned line = PCToLineNumber(i.script(), i.pc());
> -        JSScript* script = i.script();
> +        const char* filename;
> +        unsigned line;
> +        const void* referent;
> +        uint32_t referentOffset;
> +        if (!i.isWasm()) {

Can you test if (i.hasScript()) instead?

::: js/src/vm/Stack.h:1803
(Diff revision 1)
>      bool        isConstructing() const;
>      jsbytecode* pc() const { MOZ_ASSERT(!done()); return data_.pc_; }
>      void        updatePcQuadratic();
>  
> +    // The asm functions
> +    inline const wasm::Code* wasmCode() const;

I'd like to keep wasm::Code as mostly an internal impl detail of src/asmjs/ and instead just expose anything needed through wasm::FrameIterator.

Comment 3

[Yury Delendik (:yury)]

Comment on attachment 8780301 [details]
Bug 1294550 - Change AllFrameIter to return wasm frames.

https://reviewboard.mozilla.org/r/71014/#review68576

The Debugger::getNewestFrame is the user of AllFramesIter, which is needed for devtools to report current stack. So it will be cleaner to allow AllFramesIter return wasm frames.

> I'd like to hold off adding this until there is a specific need for it because right now it's totally clear if this is the right interface since we're just printing it.

Okay, we can just print pc there.

> Is there a particular use case for the pc()/fp() yet or just printing?  I was hoping most of these stayed as internal impl details of wasm::FrameIterator and a more abstract interface would be exposed to the outside.

atm printing/dump needs pc/fp, however in the future the FrameIter will need those to manupulate with the AbstractFramePtr and DebuggerFrame objects.

Comment 4

[Yury Delendik (:yury)]

Comment on attachment 8780301 [details]
Bug 1294550 - Change AllFrameIter to return wasm frames.

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

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8780301 [details]
Bug 1294550 - Change AllFrameIter to return wasm frames.

https://reviewboard.mozilla.org/r/71014/#review68820

::: js/src/vm/Stack.h:1803
(Diff revision 2)
>      bool        isConstructing() const;
>      jsbytecode* pc() const { MOZ_ASSERT(!done()); return data_.pc_; }
>      void        updatePcQuadratic();
>  
> +    // The asm frame functions.
> +    inline unsigned lineOrBytecode() const;

Wasm's lineOrBytecode() is returned via FrameIter::computeLine() so I think you can use that instead without adding anything for wasm.

Comment 6

[Yury Delendik (:yury)]

Comment on attachment 8780301 [details]
Bug 1294550 - Change AllFrameIter to return wasm frames.

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

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8780301 [details]
Bug 1294550 - Change AllFrameIter to return wasm frames.

https://reviewboard.mozilla.org/r/71014/#review68844

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d4db92a32a0
Change AllFrameIter to return wasm frames. r=luke

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d4db92a32a0