If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[wasm] Allow to inspect wasm locals values during debugging

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine: JIT
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: yury, Assigned: yury)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
At bug 1286948 we introduced wasm::DebugFrame, WasmFunctionCallObject environment and debugEnabled mode for wasm baseline compiler. Next step is to allow Debugger.Frame/.Environment to inspect local values during instrumented wasm function execution.
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review114304

Overall, makes total sense.  Sorry for the delay in reviewing.  One high-level question: could we get away with only saving the ValTypeVector of local types in the function metadata from which we could reconstruct frame offsets?  Ideally, this would be encapsulated by a tiny wasm::LocalOffsetIter which iterated over a ValTypeVector giving offsets for each local; much like ABIArgIter today.  This iter would be used by both BaselineCompiler and wasm::DebugFrame::getLocal.

::: js/src/vm/EnvironmentObject.cpp:1255
(Diff revision 1)
>  EnvironmentIter::incrementScopeIter()
>  {
> +    if (frame_ && si_.scope()->is<WasmFunctionScope>()) {
> +        // The initial frame is kept around for WasmFunctionScope to manage
> +        // live environments, reset after first iteration.
> +        frame_ = NullFramePtr();

It feels like this logic should be within EnvironmentIter::settle().  I see the problem is that, by the time settle() is called, both the env_ and si_ no longer refer to the wasm env/scope.  Could the condition instead be: (frame_.isWasmDebugFrame() && !si_.scope()->is<WasmFunctionScope>())?
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 4

7 months ago
mozreview-review-reply
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review114304

We have to save only args types (we don't save function signature anywhere) -- the rest of the locals can be parsed from binary function body. The saves args array also give information about how many args function accepts, which is needed to run ABIArgIter.

Comment 5

7 months ago
mozreview-review
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review116750

Mostly nits and, after these are addressed, should be ready to land.  Nice work!

::: js/src/vm/EnvironmentObject.cpp:1648
(Diff revision 2)
>  
> +        if (env->is<WasmFunctionCallObject>()) {
> +            if (maybeLiveEnv) {
> +                RootedScope scope(cx, getEnvironmentScope(*env));
> +                uint32_t index = 0;
> +                for (Rooted<BindingIter> bi(cx, BindingIter(scope)); bi; bi++) {

Since there's nothing doing GC inside this loop, I think you can use the bare BindingIter (like the LexicalEnvironment case above).

::: js/src/vm/Scope.cpp:1193
(Diff revision 2)
>  // TODO Check what Debugger behavior should be when it evaluates a
>  // var declaration.
>  static const uint32_t WasmFunctionEnvShapeFlags =
>      BaseShape::NOT_EXTENSIBLE | BaseShape::DELEGATE;
>  
> +static JSAtom* GenerateWasmVariableName(JSContext* cx, uint32_t index)

nit: \n after return type

::: js/src/wasm/WasmBaselineCompile.h:40
(Diff revision 2)
>  
>  // Generate adequate code quickly.
>  bool
>  BaselineCompileFunction(CompileTask* task, FuncCompileUnit* unit, UniqueChars* error);
>  
> +class BaseCompilerFunctionLocalsIter

nit: how about BaselineLocalIter?

::: js/src/wasm/WasmBaselineCompile.h:43
(Diff revision 2)
>  BaselineCompileFunction(CompileTask* task, FuncCompileUnit* unit, UniqueChars* error);
>  
> +class BaseCompilerFunctionLocalsIter
> +{
> +  private:
> +    jit::ABIArgIter<const ValTypeVector> argsIter_;

nit: could you change the #include "MacroAssembler" above to #include "WasmGenerator.h" and then you can use the ABIArgValTypeIter typedef here.

::: js/src/wasm/WasmBaselineCompile.h:67
(Diff revision 2)
> +
> +    jit::MIRType mirType() const { MOZ_ASSERT(!done_); return mirType_; }
> +    int32_t frameOffset() const { MOZ_ASSERT(!done_); return frameOffset_; }
> +    size_t index() const;
> +    int32_t currentLocalSize() const { return localSize_; }
> +    int32_t reservedSize() const { return reservedSize_; }

Instead of having a field for reservedSize\_, could you instead have the baseline compiler just call currentLocalSize() right after constructing the iterator (which is actually where it's calling reservedSize() right now)?

::: js/src/wasm/WasmBaselineCompile.cpp:202
(Diff revision 2)
> +static constexpr int32_t TlsSlotSize = sizeof(void*);
> +static constexpr int32_t TlsSlotOffset = TlsSlotSize;
> +
> +BaseCompilerFunctionLocalsIter::BaseCompilerFunctionLocalsIter(const ValTypeVector& args,
> +                                                                     const ValTypeVector& locals,
> +                                                                     bool debugEnabled)

nit: column alignment

::: js/src/wasm/WasmBaselineCompile.cpp:227
(Diff revision 2)
> +
> +    settle();
> +}
> +
> +int32_t
> +BaseCompilerFunctionLocalsIter::pushLocal(size_t nbytes) {

nit: { on new line

::: js/src/wasm/WasmBaselineCompile.cpp:316
(Diff revision 2)
> +BaseCompilerFunctionLocalsIter::index() const
> +{
> +    MOZ_ASSERT(!done_);
> +    if (!argsIter_.done())
> +        return argsIter_.index();
> +    return argsLength_ + localsIndex_;

There's sortof an ambiguity since "local index" in wasm spec terms ranges over [args]+[locals] (args are just special locals).  Could you perhaps have there just be an index_ field and then you derive the index into locals_ by subtracting off argsLength_?

::: js/src/wasm/WasmBaselineCompile.cpp:7535
(Diff revision 2)
> -        // above and regular wasm::Frame data starts after locals.
> -        localSize_ += DebugFrame::offsetOfTlsData();
> -        MOZ_ASSERT(DebugFrame::offsetOfFrame() == localSize_);
> -    }
>  
> -    for (ABIArgIter<const ValTypeVector> i(args); !i.done(); i++) {
> +    BaseCompilerFunctionLocalsIter i(args, onlyLocals, debugEnabled_);

Could you change the iterator to take a single ValTypeVector (ranging over all local types)?  Then you won't have to make an onlyLocals copy.

::: js/src/wasm/WasmCode.h:480
(Diff revision 2)
>      CacheableChars        filename;
>  
>      // Debug-enabled code is not serialized.
>      bool                  debugEnabled;
>      Uint32Vector          debugTrapFarJumpOffsets;
> +    FuncArgTypesVector    funcArgTypes;

Can you name debugFuncArgTypes?

::: js/src/wasm/WasmCode.h:673
(Diff revision 2)
>      bool incrementStepModeCount(JSContext* cx, uint32_t funcIndex);
>      bool decrementStepModeCount(JSContext* cx, uint32_t funcIndex);
>  
> +    // Stack inspection helpers.
> +
> +    bool getFuncArgsAndLocalsTypes(uint32_t funcIndex, ValTypeVector* args, ValTypeVector* locals);

With the suggested iterator change, I think both uses of this function will be better suited if it returns a single ValTypeVector with all local types in it.  In that case, and to account for the debug-only restriction, I'd rename to debugGetLocalTypes.

::: js/src/wasm/WasmCode.cpp:1182
(Diff revision 2)
> +    if (!args->appendAll(metadata_->funcArgTypes[funcIndex]))
> +        return false;
> +
> +    // Decode local var types from wasm binary function body.
> +    MOZ_ASSERT(maybeBytecode_);
> +    const CodeRange* range = lookupRangeByFuncIndexSlow(funcIndex);

IIUC, lookupRangeByFuncIndexSlow() may be called repeatedly (e.g., for every local when displaying a stack frame) so this O(n) operation seems likely to explode since n can be 50k.

How about we have a Metadata::debugFuncToCodeRange and make this O(1).  Conveniently, you don't even need to build this, there's already a ModuleGenerator::funcToCodeRange\_ you can Move() in finish().  (Btw, you'll want to put funcToCodeRange\_ into the "Data that is moved into the result of finish()" field group in WasmGenerator.h.)  Then lookupRangeByFuncIndexSlow() could be deleted.

::: js/src/wasm/WasmDebugFrame.cpp:88
(Diff revision 2)
> +    switch (iter.mirType()) {
> +      case jit::MIRType::Int32:
> +          vp.set(Int32Value(*static_cast<int32_t*>(dataPtr)));
> +          break;
> +      case jit::MIRType::Int64:
> +          // TODO int64 support

nit: I wouldn't call this a "TODO" but rather: "// Just display as a Number; it's ok if we lose some precision".

::: js/src/wasm/WasmGenerator.h:239
(Diff revision 2)
>      Uint32Vector                    funcToCodeRange_;
>      Uint32Set                       exportedFuncs_;
>      uint32_t                        lastPatchedCallsite_;
>      uint32_t                        startOfUnpatchedCallsites_;
>      Uint32Vector                    debugTrapFarJumps_;
> +    FuncArgTypesVector              funcArgTypes_;

nit: Can you name this debugFuncArgTypes_?

::: js/src/wasm/WasmGenerator.cpp:1177
(Diff revision 2)
>      // parallel copilation). Shrink it back down to fit.
>      if (isAsmJS() && !metadata_->tables.resize(numTables_))
>          return nullptr;
>  
> +    // Additional debug information to copy.
> +    metadata_->funcArgTypes.swap(funcArgTypes_);

nit: could you move this up to right under the initialization of metadata_->customSections and use Move() instead of swap for symmetry?
Comment hidden (mozreview-request)
(Assignee)

Comment 7

7 months ago
mozreview-review-reply
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review116750

> Instead of having a field for reservedSize\_, could you instead have the baseline compiler just call currentLocalSize() right after constructing the iterator (which is actually where it's calling reservedSize() right now)?

After settle() call in the constructor, currentLocalSize() includes size of the first locals.

That was the reason why I introduced reservedSize_ and, on different note, localSize_ cannot be used for varLow/varHigh after the loops.
Comment hidden (mozreview-request)

Comment 9

7 months ago
mozreview-review
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review116930

r+ with this one request.  Also, I realized that we should probable get a thumbs-up from Lars for the WasmBaselineCompile changes.

::: js/src/wasm/WasmBaselineCompile.h:48
(Diff revision 4)
> +    using ConstValTypeRange = mozilla::Range<const ValType>;
> +
> +    const ValTypeVector&               locals_;
> +    size_t                             argsLength_;
> +    ConstValTypeRange                  argsRange_; // range struct cache for ABIArgIter
> +    jit::ABIArgIter<ConstValTypeRange> argsIter_;

Can't argsIter_ just be of type ABIArgValTypeIter and then its constructor takes a const ValTypeVector& so you don't need argsRange_ here?

::: js/src/wasm/WasmGenerator.cpp:1164
(Diff revision 4)
>      metadata_->customSections = Move(env_->customSections);
>  
> +    // Additional debug information to copy.
> +    metadata_->debugFuncArgTypes = Move(debugFuncArgTypes_);
> +    if (metadata_->debugEnabled)
> +      metadata_->debugFuncToCodeRange = Move(funcToCodeRange_);

nit: four space indent
Attachment #8832507 - Flags: review?(luke) → review+

Comment 10

7 months ago
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

(for the baseline changes)
Attachment #8832507 - Flags: feedback?(lhansen)
(Assignee)

Comment 11

7 months ago
mozreview-review-reply
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review116930

> Can't argsIter_ just be of type ABIArgValTypeIter and then its constructor takes a const ValTypeVector& so you don't need argsRange_ here?

Not sure if `const ValTypeVector&` will work -- we just needs a portion of locals vector. It needs to be a Range, otherwise we have to create a vector clone for args (that was a reason, we cloned onlyLocals in earlier patch revisions)
Comment hidden (mozreview-request)

Comment 13

7 months ago
Oh, I see, makes sense.

Comment 14

7 months ago
mozreview-review
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review117352

::: js/src/wasm/WasmBaselineCompile.h:40
(Diff revision 5)
>  
>  // Generate adequate code quickly.
>  bool
>  BaselineCompileFunction(CompileTask* task, FuncCompileUnit* unit, UniqueChars* error);
>  
> +class BaselineLocalIter

It looks like BaselineLocalIter is only used in the baseline compiler.  If you think that's going to continue to be the case then it should be in the .cpp file, not in this header file, which I'm trying to keep small.

::: js/src/wasm/WasmBaselineCompile.cpp:7539
(Diff revision 5)
> -    }
> -
> -    for (ABIArgIter<const ValTypeVector> i(args); !i.done(); i++) {
>          Local& l = localInfo_[i.index()];
> -        switch (i.mirType()) {
> -          case MIRType::Int32:
> +        l.init(i.mirType(), i.frameOffset());
> +        varLow_ = i.currentLocalSize();

Hoist outside loop?

::: js/src/wasm/WasmBaselineCompile.cpp:7547
(Diff revision 5)
> -    varLow_ = localSize_;
> -
> -    for (size_t i = args.length(); i < locals_.length(); i++) {
> -        Local& l = localInfo_[i];
> -        switch (locals_[i]) {
> -          case ValType::I32:
> +    varHigh_ = varLow_;
> +    for (; !i.done() ; i++) {
> +        MOZ_ASSERT(!i.isArg());
> +        Local& l = localInfo_[i.index()];
> +        l.init(i.mirType(), i.frameOffset());
> +        varHigh_ = i.currentLocalSize();

Hoist outside loop?

Comment 15

7 months ago
Oh, and since it's "BaseCompiler" it might as well be "BaseLocalIter", esp if it is moved to the .cpp file.
(Assignee)

Comment 16

7 months ago
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review117352

> Hoist outside loop?

I cannot really hoist currentLocalSize() since with i++, currentLocalSize changes to next local position.
(Assignee)

Comment 17

7 months ago
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review117352

> It looks like BaselineLocalIter is only used in the baseline compiler.  If you think that's going to continue to be the case then it should be in the .cpp file, not in this header file, which I'm trying to keep small.

BaselineLocalIter is extracted to be used in the DebugFrame::getLocal method. Will it be preferable to just provide something like debugGetBaseCompilerLocalTypeAndOffset and move class definition inside .cpp file?

Comment 18

7 months ago
mozreview-review
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review117426

::: js/src/wasm/WasmBaselineCompile.h:40
(Diff revision 5)
>  
>  // Generate adequate code quickly.
>  bool
>  BaselineCompileFunction(CompileTask* task, FuncCompileUnit* unit, UniqueChars* error);
>  
> +class BaselineLocalIter

No, it's OK as it is, but if a lot of other definitions start showing up in the header because of further dependencies/requirements then I may change my mind :)

Comment 19

7 months ago
mozreview-review
Comment on attachment 8832507 [details]
Bug 1335773 - Inspects wasm function locals.

https://reviewboard.mozilla.org/r/108756/#review117430

::: js/src/wasm/WasmBaselineCompile.cpp:7539
(Diff revision 5)
> -    }
> -
> -    for (ABIArgIter<const ValTypeVector> i(args); !i.done(); i++) {
>          Local& l = localInfo_[i.index()];
> -        switch (i.mirType()) {
> -          case MIRType::Int32:
> +        l.init(i.mirType(), i.frameOffset());
> +        varLow_ = i.currentLocalSize();

Oh, for the last iteration?  That's ugly.  But OK, it is what it is.
Comment hidden (mozreview-request)
(Assignee)

Comment 21

7 months ago
(In reply to Lars T Hansen [:lth] from comment #15)
> Oh, and since it's "BaseCompiler" it might as well be "BaseLocalIter", esp
> if it is moved to the .cpp file.

Done.

Comment 22

7 months ago
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36098d6bd314
Inspects wasm function locals. r=luke

Comment 23

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36098d6bd314
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.