Currently we don't inject or handle any debug traps (for breakpoint or stepping) in the baseline compiler. We need to generate calls and devtools is shown -- it's when we are showing WebAssembly text representation, so we can set breakpoint and step through the code. At the moment we chose offset in the binary format as index/offset of the WebAssembly expression/instruction. The text format will produce mapping of this offset to the generated text (see bug 1285976) Current DebuggerScript/DebuggerSource/BreakpointSite/Breakpoint abstractions are relying on JSScript or BaselineScript to perform such operation. We need to introduce new abstractions to handle the WebAssembly debugging.

Some WIP to help to plan future work of the needed functionality.

(jsshell shall be run in baseline mode, e.g. js --wasm-always-baseline sqrtOfSqrs.js)

Comment on attachment 8784886 [details] Bug 1286948 - Renames Profiling to Instrument for prolog/epilogue. https://reviewboard.mozilla.org/r/74200/#review95110 ::: js/src/wasm/WasmCode.cpp:846 (Diff revision 12) > - for (const CodeRange& codeRange : metadata_->codeRanges) > + for (const CodeRange& codeRange : metadata_->codeRanges) > - ToggleProfiling(*this, codeRange, newProfilingEnabled); > + ToggleInstrumentation(*this, codeRange, true); > - } > +} > > - return true; > +void > +Code::decrementInstrumentationMode(JSContext* cx) Most of the code is common between these two functions so could you collapse these back down into a single function adjustInstrumentationMode(cx, int32_t delta)?

Comment on attachment 8784886 [details] Bug 1286948 - Renames Profiling to Instrument for prolog/epilogue. https://reviewboard.mozilla.org/r/74200/#review95112 Oops, reviewboard is oconfusing, I meant to add one more request: "Instrumentation" is kindof a pretty long word; could we do "Instrument"? It's not quite as proper english, but I think it conveys the same point in 5 less chars :)

Comment on attachment 8792075 [details] Bug 1286948 - Adds debug mode for WASM baseline compiler. https://reviewboard.mozilla.org/r/79304/#review95114 ::: js/src/wasm/WasmBaselineCompile.cpp:504 (Diff revision 7) > int32_t localSize_; // Size of local area in bytes (stable after beginFunction) > int32_t varLow_; // Low byte offset of local area for true locals (not parameters) > int32_t varHigh_; // High byte offset + 1 of local area for true locals > int32_t maxFramePushed_; // Max value of masm.framePushed() observed > bool deadCode_; // Flag indicating we should decode & discard the opcode > + bool compileDebugInformation_; While this is gloriously specific, I think 'debug' would be sufficient (here and elsewhere) and shorter. ::: js/src/wasm/WasmCode.cpp:597 (Diff revision 7) > metadata_(&metadata), > maybeBytecode_(maybeBytecode), > instrumentationModeCounter_(0), > - profilingEnabled_(false) > + profilingEnabled_(false), > + debugEnabled_(false) > {} Can you add a MOZ_ASSERT_IF(debugEnabled_, maybeBytecode)? ::: js/src/wasm/WasmCode.cpp:867 (Diff revision 7) > for (const CodeRange& codeRange : metadata_->codeRanges) > ToggleInstrumentation(*this, codeRange, false); > } > > +bool > +Code::ensureDebugEnabled(JSContext* cx, bool enabled) Could you put this function right under ensureProfilingState and rename to ensureDebugState for symmetry? ::: js/src/wasm/WasmCompile.cpp:650 (Diff revision 7) > + > + // Debug information such as source view or debug traps will require > + // additional memory, so we try to only enabled it when a developer > + // actually cares: when the compartment is debuggable (which is true when > + // the web console is open). > + debuggee = cx->compartment()->isDebuggee(); This will be true whenever devtools is open at all, including when the console or profiler is open, in which case we want to run full speed. I think instead we should use cx->compartment()->debuggerObservesAsmJS() which is only true when the debugger tab is actually open. But this also means that debuggee is a narrower predicate than saving bytecode (which makes sense: the minor size hit of saving bytecode doesn't affect perf) which breaks assumptions in other parts of this patch. Another idea: what if we synchronously recompiled Ion code into baseline code whenever we flip on the debug state (and there are no live activations, see wasm::Compartment::ensureProfilingState)? I think that would only take a few lines of code. ::: js/src/wasm/WasmGenerator.cpp:963 (Diff revision 7) > : IonCompileTask::CompileMode::Ion; > > - fg->task_->init(Move(func), mode); > + // If baseline compiler was not selected when compileDebugInformation_, > + // reset the flag, so the debug mode will not be enabled for the instance. > + if (compileDebugInformation_ && mode != IonCompileTask::CompileMode::Baseline) > + compileDebugInformation_ = false; I'd express the logic: ``` if ((alwaysBaseline_ || debug_) && BaselineCanCompile(fg)) { mode = IonCompileTask::CompileMode::Baseline; } else { mode = IonCompileTask::CompileMode::Ion; debug_ = false; // Ion does not support debugging } ```

(In reply to Luke Wagner [:luke] from comment #97) > Comment on attachment 8792075 [details] > Bug 1286948 - Adds debug mode for WASM baseline compiler. > > https://reviewboard.mozilla.org/r/79304/#review95114 > > ::: js/src/wasm/WasmBaselineCompile.cpp:504 > (Diff revision 7) > > int32_t localSize_; // Size of local area in bytes (stable after beginFunction) > > int32_t varLow_; // Low byte offset of local area for true locals (not parameters) > > int32_t varHigh_; // High byte offset + 1 of local area for true locals > > int32_t maxFramePushed_; // Max value of masm.framePushed() observed > > bool deadCode_; // Flag indicating we should decode & discard the opcode > > + bool compileDebugInformation_; > > While this is gloriously specific, I think 'debug' would be sufficient (here > and elsewhere) and shorter. Since "debug_" would be fabulously generic :) might I propose "debugInfo_" as a compromise?

(In reply to Lars T Hansen [:lth] from comment #98) > > While this is gloriously specific, I think 'debug' would be sufficient (here > > and elsewhere) and shorter. > > Since "debug_" would be fabulously generic :) might I propose "debugInfo_" > as a compromise? If the bool stays constrained to only triggering the emission of debug info, you're right that's a better name. But will this flag also change how code is generated (with the toggleable trap calls)?

(In reply to Luke Wagner [:luke] from comment #97) > Another idea: what if we synchronously recompiled Ion code into baseline > code whenever we flip on the debug state (and there are no live activations, > see wasm::Compartment::ensureProfilingState)? I think that would only take > a few lines of code. Thinking more about this (and the power of being able to compile baseline code on demand Real Fast), what about the following strategy: - wasm::Code holds 2 UniqueCodeSegment's: segment_, maybeDebugSegment_. - we always start by compiling a (non-debug) segment_ during wasm::Compile - if cx->compartment().isDebuggee() || have-names-section, we save the bytecode (as we do today) - we define a new JSCompartment::debuggerCanBreak() that is true when there are any active breakpoints set in the compartment - we generalize *::ensureProfilingState() to *::ensureInstrumentState() and have it compile a maybeDebugSegment_ when debuggerCanBreak(). - Instance::callExport calls maybeDebugSegment_ if debuggerCanBreak() and !!maybeDebugSegment_. What I'm hoping to get here is that performance is only affected when the developer is actively debugging. In particular, devs may just have the Debugger tab open for viewing code (not intending to debug anything) and this avoids unexpected performance artifacts that may mess up their measurements/experiments.

(In reply to Luke Wagner [:luke] from comment #100) One refinement: part of Metadata is also CodeSegment-specific (funcImports/Exports, memoryAccesses, memoryPatches, boundsChecks, callSites, callThunks), which suggests splitting metadata and making a new type that groups a CodeSegment with this code-specific metadata. (I can write this patch if we agree on the overall approach.)

Comment on attachment 8784887 [details] Bug 1286948 - Adds prolog and epilog debug traps and handlers. https://reviewboard.mozilla.org/r/74202/#review95120 Overall, this is looking great. Clearing review for now; interested to see what you think about comments 100/101 and also simplification suggested below: ::: js/src/jit/MacroAssembler.cpp:2921 (Diff revision 12) > + RepatchLabel jump; > + jump.use(codeOffset); > + bind(&jump); > + } > + bindWasmDebugTrapFarJump(farJumpWithPatch().offset()); > + clearWasmDebugToggableCalls(); IIUC, you don't need the out-of-line code at all. The only reason regular traps need it is because they use *jumps* inline (viz., conditional jumps, which you can do with calls) and we need a CallSite for stack-walking to work correctly. But if we can use *calls* inline (unconditionally), then I think you can just use a regular call(wasm::CallSiteDesc, wasm::CallSiteDesc::TrapExit (and then add a new wasm::Trap::Debug) and the existing machinery in MG::patchCallSites will handle the far jumps as necessary. ::: js/src/jit/arm/MacroAssembler-arm.h:630 (Diff revision 12) > CodeOffset toggledJump(Label* label); > > // Emit a BLX or NOP instruction. ToggleCall can be used to patch this > // instruction. > CodeOffset toggledCall(JitCode* target, bool enabled); > + CodeOffset toggledDebugTrap(bool enabled); Would it be possible to instead model these after the nopPatachableToNearJump/patchNopToNearJump/patchNearJumpToNop trio in MacroAssembler.h (making a new nopPatchableToCall/patchNoToCall/patchCallToNop trio)? If you look at, in particular, the ARM impl, it's a good deal simpler. ::: js/src/jit/shared/Assembler-shared.h:848 (Diff revision 12) > wasm::MemoryAccessVector memoryAccesses_; > wasm::MemoryPatchVector memoryPatches_; > wasm::BoundsCheckVector boundsChecks_; > wasm::GlobalAccessVector globalAccesses_; > wasm::SymbolicAccessVector symbolicAccesses_; > + wasm::DebugTrapJump wasmDebugTrapJump_; nit: no need for "wasm" prefix in field name ::: js/src/wasm/WasmBaselineCompile.cpp:2088 (Diff revision 12) > > masm.wasmEmitTrapOutOfLineCode(); > > + if (compileDebugInformation_) { > + debugTraps_->farJumpOffset = masm.wasmDebugTrapFarJump().offset(); > + } nit: no {} for single-line then ::: js/src/wasm/WasmCode.h:22 (Diff revision 12) > */ > > #ifndef wasm_code_h > #define wasm_code_h > > +#include "jscntxt.h" jscntxt.h is one of those pull-in-the-world headers whereas, with the exception of WasmJS.h, I try to keep all that out of most of the Wasm*.h headers. Is this really needed here or could we get away with another forward declaration? ::: js/src/wasm/WasmCode.cpp:898 (Diff revision 12) > + AutoFlushICache afc("Code::incrementEnterFrameTraps"); > + AutoFlushICache::setRange(uintptr_t(segment_->base()), segment_->codeLength()); > + for (const CodeRange& codeRange : metadata_->codeRanges) { > + if (!codeRange.isFunction()) > + continue; > + const FuncDebugTraps& debugTraps = metadata_->funcDebugTraps[codeRange.funcIndex()]; Could you instead iterate `for (const FuncDebugTraps& debugTraps : metadata_->funcDebugTraps)`? ::: js/src/wasm/WasmGenerator.cpp:403 (Diff revision 12) > + if (task->compileDebugInformation()) { > + FuncDebugTraps& debugTraps = *task->maybeDebugTraps(); > + debugTraps.enterFrameTrap.offsetBy(offsetInWhole); > + debugTraps.leaveFrameTrap.offsetBy(offsetInWhole); > + debugTraps.farJumpOffset += offsetInWhole; > + debugTraps_[func.index()] = debugTraps; Can you move FuncDebugTraps into AssemblerShared (in jit/shared/AssemblerShared.h), right under the trap fields? Then this offseBy() stuff can go in asmMergeWith() along with all the rest and you can remove the separate member/parameter from all the other places. ::: js/src/wasm/WasmGenerator.cpp:587 (Diff revision 12) > return false; > > + for (FuncDebugTraps debugTrap : debugTraps_) { > + if (!debugTrap.undefined()) > + masm_.patchFarJump(CodeOffset(debugTrap.farJumpOffset), debugTrapStub.begin); > + } nit: Could you put this loop at the end of patchFarJumps()? ::: js/src/wasm/WasmGenerator.cpp:1194 (Diff revision 12) > if (isAsmJS() && !metadata_->tables.resize(numTables_)) > return nullptr; > > + if (compileDebugInformation_) { > + metadata_->funcDebugTraps.swap(debugTraps_); > + } nit: no { } for single-line and could you `metadata_->funcDebugTraps = masm_.extractFuncDebugTraps();` (assuming the abovementioned change) right below the other `masm_.extrac*` calls above? ::: js/src/wasm/WasmStubs.cpp:1163 (Diff revision 12) > + masm.haltingAlign(CodeAlignment); > + > + Offsets offsets; > + offsets.begin = masm.currentOffset(); > + > + // TODO reduce copy'n'paste (copied from GenerateInterruptStub) Agreed, it would be really good to share code. I haven't compared closely, but I'm hoping it's just a matter of adding a SymbolicAddress parameter to GenerateInterruptExit? ::: js/src/wasm/WasmTypes.h:1014 (Diff revision 12) > typedef Vector<CallSiteAndTarget, 0, SystemAllocPolicy> CallSiteAndTargetVector; > > +struct DebugTrapAddress > +{ > + size_t trapOp; > + size_t returnOp; nit: For symmetry with some other similar-purposed structs, could these be named `trapOffset`/`returnOffset`? ::: js/src/wasm/WasmTypes.h:1032 (Diff revision 12) > +struct FuncDebugTraps > +{ > + static const uint32_t UNDEFINED_JUMP_OFFSET = 0xC0000000; > + > + DebugTrapAddress enterFrameTrap; > + DebugTrapAddress leaveFrameTrap; nit: I think the "Trap" suffix in these two field names isn't necessary and will shorten some lines elsewhere. ::: js/src/wasm/WasmTypes.cpp:304 (Diff revision 12) > case SymbolicAddress::HandleExecutionInterrupt: > return FuncCast(WasmHandleExecutionInterrupt, Args_General0); > + case SymbolicAddress::HandleDebugTrap: > + return FuncCast(WasmHandleDebugTrap, Args_General0); > + case SymbolicAddress::HandleDebugThrow: > + return FuncCast(WasmHandleDebugThrowTrap, Args_General0); nit: can you remove "Trap" at the end of the function name to match the SymbolicAddress name?

Actually, n/m comments 100/101. I started to refactor toward this and I pretty quickly hit a challenge that is not insurmountable but probably out of the scope of this bug: we write code pointers into Tables and imports so it's non-trivial to have multiple versions of the code: we'd have to update a bunch of memory locations to point to the right one or introduce a level of indirection. (See #error lines in attached patch for a first example.) So probably for now using debuggerObservesAsmJS() to control ion vs. baseline a priori is the way to go. Sorry for the noise!

Comment on attachment 8792075 [details] Bug 1286948 - Adds debug mode for WASM baseline compiler. https://reviewboard.mozilla.org/r/79304/#review95114 > While this is gloriously specific, I think 'debug' would be sufficient (here and elsewhere) and shorter. For WasmBaselineCompile.cpp, the "compileDebugInformation" was renamed to "debugInfo", and for the rest of the code -- "debug". The WasmBaselineCompile's debugInfo is specific to the compiler. The information needs to be returned via IonCompileTask, so I'm thinking to create an object similar to the FuncCompileResults and use presence of this object (instead of "debug") to detect debug mode > This will be true whenever devtools is open at all, including when the console or profiler is open, in which case we want to run full speed. I think instead we should use cx->compartment()->debuggerObservesAsmJS() which is only true when the debugger tab is actually open. But this also means that debuggee is a narrower predicate than saving bytecode (which makes sense: the minor size hit of saving bytecode doesn't affect perf) which breaks assumptions in other parts of this patch. > > Another idea: what if we synchronously recompiled Ion code into baseline code whenever we flip on the debug state (and there are no live activations, see wasm::Compartment::ensureProfilingState)? I think that would only take a few lines of code. Using debuggerObservesAsmJS() to detect of Code is in debug mode.

Comment on attachment 8784887 [details] Bug 1286948 - Adds prolog and epilog debug traps and handlers. https://reviewboard.mozilla.org/r/74202/#review95120 > IIUC, you don't need the out-of-line code at all. The only reason regular traps need it is because they use *jumps* inline (viz., conditional jumps, which you can do with calls) and we need a CallSite for stack-walking to work correctly. But if we can use *calls* inline (unconditionally), then I think you can just use a regular call(wasm::CallSiteDesc, wasm::CallSiteDesc::TrapExit (and then add a new wasm::Trap::Debug) and the existing machinery in MG::patchCallSites will handle the far jumps as necessary. All xxxWasmDebugToggableCalls related code was removed. > Would it be possible to instead model these after the nopPatachableToNearJump/patchNopToNearJump/patchNearJumpToNop trio in MacroAssembler.h (making a new nopPatchableToCall/patchNoToCall/patchCallToNop trio)? If you look at, in particular, the ARM impl, it's a good deal simpler. Introduced toggledPatchableCall/patchToggledCall/patchToggledToCall/patchToggledToNop that is using callSites. > Can you move FuncDebugTraps into AssemblerShared (in jit/shared/AssemblerShared.h), right under the trap fields? Then this offseBy() stuff can go in asmMergeWith() along with all the rest and you can remove the separate member/parameter from all the other places. FuncDebugTraps and DebugTrapAddress were removed in favor of CallSite handling.

Comment on attachment 8787441 [details] Bug 1286948 - Adds scope and environment for wasm calls. https://reviewboard.mozilla.org/r/76190/#review97806 This looks good, I'd just like to see a new version of getOrCreateFuncScope. ::: js/src/vm/EnvironmentObject.cpp:645 (Diff revision 13) > +{ > + RootedObjectGroup group(cx, ObjectGroup::defaultNewGroup(cx, &class_, TaggedProto(nullptr))); > + if (!group) > + return nullptr; > + > + MOZ_ASSERT(cx->isJSContext()); Why this assert? ::: js/src/vm/EnvironmentObject.cpp:662 (Diff revision 13) > + JSObject* obj; > + JS_TRY_VAR_OR_RETURN_NULL(cx, obj, JSObject::create(cx, kind, gc::DefaultHeap, shape, group)); > + > + Rooted<WasmFunctionCallObject*> callobj(cx, &obj->as<WasmFunctionCallObject>()); > + callobj->initFixedSlot(INSTANCE_SLOT, ObjectOrNullValue(instance)); > + callobj->initFixedSlot(FUNC_INDEX_SLOT, NumberValue(funcIndex)); Since funcIndex never escapes to script, you should use PrivateUint32Value here. ::: js/src/vm/EnvironmentObject.cpp:678 (Diff revision 13) > +} > + > +uint32_t > +WasmFunctionCallObject::funcIndex() const > +{ > + return getReservedSlot(FUNC_INDEX_SLOT).toInt32(); Similarly, use toPrivateUint32 here. ::: js/src/vm/Scope.h:883 (Diff revision 13) > JSScript* script() const; > > static Shape* getEmptyEnvironmentShape(ExclusiveContext* cx); > }; > > +// Scope corresponding to the wasm function. This comment should also say that these scopes are used by Debugger only, and not for wasm execution. ::: js/src/vm/Scope.h:899 (Diff revision 13) > + uint32_t length; > + uint32_t nextFrameSlot; > + > + // The wasm instance of the scope. > + GCPtr<WasmInstanceObject*> wasmInstance; > + uint32_t funcIndex; Nit: put the uint32_t fields next to each other. ::: js/src/vm/Scope.cpp:470 (Diff revision 13) > case ScopeKind::NonSyntactic: > return 0; > case ScopeKind::Module: > return si.scope()->as<ModuleScope>().nextFrameSlot(); > + case ScopeKind::WasmFunction: > + return 0; // si.scope()->as<WasmFunctionScope>().nextFrameSlot(); Add a TODO here. ::: js/src/vm/Scope.cpp:1148 (Diff revision 13) > { > return module()->script(); > } > > +static const uint32_t WasmFunctionEnvShapeFlags = > + BaseShape::NOT_EXTENSIBLE | BaseShape::QUALIFIED_VAROBJ | BaseShape::DELEGATE; Technically QUALIFIED_VAROBJ doesn't make sense here because wasm function locals aren't |var|s. Could you add a TODO here? We'd need to think through what Debugger behavior should be when it evaluates a var declaration. ::: js/src/vm/Scope.cpp:1162 (Diff revision 13) > + // GCPtr. Destruction of |copy| below may trigger calls into the GC. > + Rooted<WasmFunctionScope*> wasmFunctionScope(cx); > + > + { > + // The data that's passed in may be from the frontend and LifoAlloc'd. > + // Copy it now that we're creating a permanent VM scope. Is this comment true for WasmFunctionScopes? I don't think its Data is ever created by the frontend. Seems to me we can simplify this code. ::: js/src/vm/Scope.cpp:1168 (Diff revision 13) > + RootedShape envShape(cx); > + Rooted<UniquePtr<Data>> copy(cx, copyData(cx, data, &envShape)); > + if (!copy) > + return nullptr; > + > + // Modules always need an environment object for now. Nit: comment refers to modules instead of wasm functions. ::: js/src/vm/Scope.cpp:1196 (Diff revision 13) > +{ > + if (data) { > + BindingIter bi(*data); > + return CopyScopeData<WasmFunctionScope>(cx, bi, data, > + &WasmFunctionCallObject::class_, > + WasmFunctionEnvShapeFlags, envShape); Nit: extra leading space ::: js/src/wasm/WasmJS.h:204 (Diff revision 13) > const wasm::CodeRange& getExportedFunctionCodeRange(HandleFunction fun); > + > + Scope* funcScope(JSContext* cx, uint32_t funcIndex); > + Scope* innermostScope(JSContext* cx, void *pc); > + > +private: Nit: need 2 leading spaces ::: js/src/wasm/WasmJS.cpp:1037 (Diff revision 13) > const Metadata& metadata = instance().metadata(); > return metadata.codeRanges[metadata.lookupFuncExport(funcIndex).codeRangeIndex()]; > } > > +Scope* > +WasmInstanceObject::innermostScope(JSContext* cx, void *pc) Nit: void* pc ::: js/src/wasm/WasmJS.cpp:1062 (Diff revision 13) > + if (ScopeMap::Ptr p = instance->scopes().lookup(funcIndex)) > + return p->value(); > + > + // WasmFunctionScope::create uses WasmFunctionScope::Data fields, but > + // we should not destroy them (esp. GCPtr) -- temporary creating blob for > + // the data. I don't undertand this comment. ::: js/src/wasm/WasmJS.cpp:1075 (Diff revision 13) > + Rooted<Scope*> globalScope(cx, &instance->global().emptyGlobalScope()); > + > + JSAutoCompartment ac(cx, instance); > + Rooted<WasmFunctionScope*> funcScope(cx, WasmFunctionScope::create(cx, > + Handle<WasmFunctionScope::Data*>::fromMarkedLocation(&data), globalScope)); > + I know this is temporary, but this snippet still makes me uneasy. Since the initial Data is not actually LifoAlloc'd, you don't need to allocate something only to copy it. Seems like you could just use |create| with an empty Data* and have it create a default one and remove all this UniquePtr and fromMarkedLocation code.

Comment on attachment 8784889 [details] Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. https://reviewboard.mozilla.org/r/74206/#review97886 Most of it looks good, though this patch doesn't seem to do the DebugFrame lifetime management correctly. I also need to understand the big picture better with how traps are actually handled before I sign off on it. ::: js/src/gdb/mozilla/Interpreter.py:51 (Diff revision 14) > class AbstractFramePtr(object): > Tag_ScriptFrameIterData = 0x0 > Tag_InterpreterFrame = 0x1 > Tag_BaselineFrame = 0x2 > Tag_RematerializedFrame = 0x3 > - TagMask = 0x3 > + Tag_WasmShadowFrame = 0x4 Rename ShadowFrame to DebugFrame? ::: js/src/vm/EnvironmentObject.cpp:1280 (Diff revision 14) > EnvironmentIter::settle() > { > // Check for trying to iterate a function or eval frame before the prologue has > // created the CallObject, in which case we have to skip. > - if (frame_ && frame_.script()->initialEnvironmentShape() && !frame_.hasInitialEnvironment()) { > + if (frame_ && !frame_.isWasmDebugFrame() && > + frame_.script()->initialEnvironmentShape() && !frame_.hasInitialEnvironment()) { Nit: { on newline for multi-line conditionals ::: js/src/vm/EnvironmentObject.cpp:1297 (Diff revision 14) > } > } > > // Check if we have left the extent of the initial frame after we've > // settled on a static scope. > - if (frame_ && (!si_ || si_.scope() == frame_.script()->enclosingScope())) > + if (frame_ && !frame_.isWasmDebugFrame() && The else is redundant. This could just be if (frame_ && (frame_.isWasmDebugFrame() || (...)) ::: js/src/vm/EnvironmentObject.cpp:1298 (Diff revision 14) > } > > // Check if we have left the extent of the initial frame after we've > // settled on a static scope. > - if (frame_ && (!si_ || si_.scope() == frame_.script()->enclosingScope())) > + if (frame_ && !frame_.isWasmDebugFrame() && > + (!si_ || si_.scope() == frame_.script()->enclosingScope())) Nit: multi-line conditionals need { } ::: js/src/vm/GeneratorObject.cpp:51 (Diff revision 14) > obj = NewNativeObjectWithGivenProto(cx, &LegacyGeneratorObject::class_, proto); > } > if (!obj) > return nullptr; > > - GeneratorObject* genObj = &obj->as<GeneratorObject>(); > + Rooted<GeneratorObject*> genObj(cx, &obj->as<GeneratorObject>()); Was this Rooted needed for something? ::: js/src/vm/Stack.h:1739 (Diff revision 14) > void* resumePC() const { return resumePC_; } > + > + wasm::DebugFrame* getDebugFrame(wasm::FrameIterator& iter); > + bool getTrackedDebugFrames(FramePointerVector& fps); > + void removeDebugFrame(FramePointer fp); > + void clearDebugFrames(); removeDebugFrame and clearDebugFrames are unused. Are they used later? ::: js/src/vm/Stack.cpp:1055 (Diff revision 14) > + WasmActivation* activation = data_.activations_->asWasm(); > + > + data_.wasmFrames_ = wasm::FrameIterator(*activation); > + while (data_.wasmFrames_.fp() != fp) > + ++data_.wasmFrames_; > + When does the current iter's pc for wasm frames become stale? ::: js/src/vm/Stack.cpp:1714 (Diff revision 14) > + if (!p) { > + frame = wasm::DebugFrame::GetInitialized(iter); > + if (!frame) > + return nullptr; > + if (!debugFrames_->add(p, fp, frame) || > + !frame->ensureObservingState(cx_, true)) { Nit: { on newline ::: js/src/vm/Stack.cpp:1718 (Diff revision 14) > + if (!debugFrames_->add(p, fp, frame) || > + !frame->ensureObservingState(cx_, true)) { > + frame->~DebugFrame(); > + return nullptr; > + } > + } else Nit: { } around else ::: js/src/vm/Stack.cpp:1772 (Diff revision 14) > + return false; > + } > + FramePointerVector scratch(cx_); > + if (!scratch.resize(fps.length())) > + return false; > + MOZ_ALWAYS_TRUE(MergeSort(fps.begin(), fps.length(), scratch.begin(), SortComparatorFramePointer())); Why is this sorted? ::: js/src/wasm/WasmBaselineCompile.cpp:2067 (Diff revision 14) > } > > // The TLS pointer is always passed as a hidden argument in WasmTlsReg. > // Save it into its assigned local slot. > storeToFramePtr(WasmTlsReg, localInfo_[tlsSlot_].offs()); > + if (debugInfo_) { Where is debugInfo_ defined and how is it used? Is this one of the other patches in the series? ::: js/src/wasm/WasmDebugFrame.h:6 (Diff revision 14) > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- > + * vim: set ts=8 sts=4 et sw=4 tw=99: > + * > + * Copyright 2016 Mozilla Foundation > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); Huh, wasm isn't MPL? ::: js/src/wasm/WasmDebugFrame.h:37 (Diff revision 14) > + > +struct TlsData; > + > +struct DebugFrame > +{ > +private: Nit: 2-spaces before public/private labels. Also if you start with private, why not make DebugFrame a class? ::: js/src/wasm/WasmDebugFrame.h:54 (Diff revision 14) > + bool hasCachedSavedFrame_ : 1; > + }; > + struct > + { > + void* reserved1; > + }; Any reason the void* is in the struct instead of just by itself? I assume this is for alignment. ::: js/src/wasm/WasmDebugFrame.h:88 (Diff revision 14) > + > + bool ensureObservingState(JSContext* cx, bool observing); > + > + void trace(JSTracer* trc); > + > + static size_t sizeWithoutTlsData() { return offsetof(DebugFrame, tlsData_); } This assumes that tlsData_ will always be the last field in the struct. Add a static assert to that effect please. ::: js/src/wasm/WasmDebugFrame.h:92 (Diff revision 14) > + > + static size_t sizeWithoutTlsData() { return offsetof(DebugFrame, tlsData_); } > + static size_t offsetOfPc() { return offsetof(DebugFrame, pc_); } > +}; > + > +static_assert(sizeof(DebugFrame) % 8 == 0, "DebugFrame align"); Is DebugFrame supposed to be explicitly 8-byte aligned or uintptr-aligned? ::: js/src/wasm/WasmDebugFrame.cpp:49 (Diff revision 14) > + MOZ_ASSERT(fp); > + void* buf = fp - sizeof(DebugFrame); > + DebugFrame* maybeInitialized = static_cast<DebugFrame*>(buf); > + if (maybeInitialized->initialized()) > + return maybeInitialized; > + return new (buf) DebugFrame(iter, maybeInitialized->tlsData_); This method scares me. I'd like the layout assumptions here to be in the header and documented -- or wherever the layout for the wasm baseline frames is currently documented. By way of comparison, Ion frame layouts have overlay structs. ::: js/src/wasm/WasmDebugFrame.cpp:65 (Diff revision 14) > + MOZ_ASSERT(range); > + uint32_t funcIndex = range->funcIndex(); > + Rooted<WasmInstanceObject*> object(cx, instance()->object()); > + > + JSAutoCompartment ac(cx, object); > + cachedEnv_.init(WasmFunctionCallObject::create(cx, object, funcIndex)); Check for OOM. ::: js/src/wasm/WasmTypes.cpp:130 (Diff revision 14) > + for (auto p = fps.begin(); p != fps.end(); p++) { > + void* fp = *p; > + wasm::DebugFrame* frame = activation->lookupDebugFrame(fp); > + Code* code = activation->compartment()->wasm.lookupCode(frame->pc()); > + code->handleDebugThrowTrap(activation, fp); > + } I don't understand how this function works or why building this vector is necessary. I'll need an explanation of the baseline-compiled code side of things in person.

Comment on attachment 8784890 [details] Bug 1286948 - onEnterFrame/onLeaveFrame wasm events and callstack. https://reviewboard.mozilla.org/r/74208/#review97970 Please audit the Debugger.Frame methods and accessors that do not make sense for wasm referents, and: 1. Make them throw, like in Debugger.Scripts and Debugger.Environments with wasm referents. 2. Document them in js/src/doc/Debugger. Probably would be easier if you did those as a new patch instead of folding them into this patch. Additionally, I'm not totally clear on the 'isDebuggee-ness' of wasm modules. Can it be toggled from off->on? For the next version of the patch I'm mainly looking for a new version of the loop in |UpdateExecutionObservabilityOfScriptsInZone|. ::: js/src/vm/Debugger-inl.h:20 (Diff revision 14) > js::Debugger::onLeaveFrame(JSContext* cx, AbstractFramePtr frame, jsbytecode* pc, bool ok) > { > MOZ_ASSERT_IF(frame.isInterpreterFrame(), frame.asInterpreterFrame() == cx->interpreterFrame()); > - MOZ_ASSERT_IF(frame.script()->isDebuggee(), frame.isDebuggee()); > + MOZ_ASSERT_IF(frame.hasScript() && frame.script()->isDebuggee(), frame.isDebuggee()); > /* Traps must be cleared from eval frames, see slowPathOnLeaveFrame. */ > - mozilla::DebugOnly<bool> evalTraps = frame.isEvalFrame() && > + mozilla::DebugOnly<bool> evalTraps = frame.isEvalFrame() && frame.hasScript() && Is this frame.hasScript() needed? All eval frames should have scripts. ::: js/src/vm/Debugger.cpp:2611 (Diff revision 14) > + } > + if (!instance->code().ensureEnterFrameTrapsState(cx, enableTrap)) > + return false; > + } > + } > + You should be able to just use the |obs| set to check if the compartment you're iterating needs to be considered. Then, instead of iterating over all Debuggers on the compartment, you should consult |observing| because all compartments in |obs| are already being debugged. ::: js/src/vm/Debugger.cpp:2694 (Diff revision 14) > } > > /* static */ bool > Debugger::ensureExecutionObservabilityOfFrame(JSContext* cx, AbstractFramePtr frame) > { > - MOZ_ASSERT_IF(frame.script()->isDebuggee(), frame.isDebuggee()); > + MOZ_ASSERT_IF(frame.hasScript() && frame.script()->isDebuggee(), frame.isDebuggee()); We can't make on-stack wasm frames observable to the debugger at all right now. Maybe print a warning? ::: js/src/vm/Debugger.cpp:6322 (Diff revision 14) > } > > bool > Debugger::observesWasm(wasm::Instance* instance) const > { > if (!enabled || !instance->code().debugEnabled()) The call to debugEnabled() is strange because it's set once during compilation whether the Debugger is observing asm.js. What should happen if the Debugger stops observing asm.js between the time the module got compiled and now? ::: js/src/vm/Debugger.cpp:7863 (Diff revision 14) > return true; > } > > +/* static */ bool > +DebuggerFrame::getFrameIter(JSContext* cx, HandleDebuggerFrame frame, > + Maybe<FrameIter>& result) Nit: extra indent ::: js/src/vm/Debugger.cpp:7879 (Diff revision 14) > + if (!data) > + return false; > + frame->setPrivate(data.raw()); > + } > + return true; > +} Could you have a followup patch that removes getScriptFrameIter if we're going to be using getFrameIter going forward? ::: js/src/vm/Stack-inl.h:442 (Diff revision 14) > asBaselineFrame()->setReturnValue(rval); > return; > } > + if (isWasmDebugFrame()) { > + return; > + } Should setReturnValue ever be called on wasm debug frames, if we silently do nothing? I'd rather assert here. ::: js/src/vm/Stack-inl.h:703 (Diff revision 14) > } > > +inline bool > +AbstractFramePtr::hasScript() const { > + return !isWasmDebugFrame(); > +} I like this alias. In the previous patches there are some places that would read better if they used |hasScript()| instead of |!isWasmDebugFrame()|.

Comment on attachment 8784890 [details] Bug 1286948 - onEnterFrame/onLeaveFrame wasm events and callstack. https://reviewboard.mozilla.org/r/74208/#review97970 Reviewed Debugger.Frame methods and removed getScriptFrameIter. Some methods/properties will fail with TypeError. I made some doc modifications for type, implementation, and eval members, however I did not provide for members that are failing -- we may want to make them return some meaninful default for wasm value. (See delta changes from previous version at https://reviewboard.mozilla.org/r/74208/diff/14-15/ ) > We can't make on-stack wasm frames observable to the debugger at all right now. Maybe print a warning? Added assert for DebugFrame, so code is in debug mode. We still need to make frame observable to turn on/off onLeaveFrame traps. > The call to debugEnabled() is strange because it's set once during compilation whether the Debugger is observing asm.js. What should happen if the Debugger stops observing asm.js between the time the module got compiled and now? debugEnabled is not mutable -- once code is compiled with this flag it is stays in this mode. > Could you have a followup patch that removes getScriptFrameIter if we're going to be using getFrameIter going forward? It was simple, I just remove all usages of getScriptFrameIter after reviewing DebuggerFrame members. > Should setReturnValue ever be called on wasm debug frames, if we silently do nothing? I'd rather assert here. We cannot fail at the moment: current the return value is undefined (via DebugFrame), but it is set back during onPop handling.

Comment on attachment 8784889 [details] Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. https://reviewboard.mozilla.org/r/74206/#review97886 > Was this Rooted needed for something? It's needed for frame.environmentChain() call -- trying to root just that instead. > removeDebugFrame and clearDebugFrames are unused. Are they used later? clearDebugFrames is removed. removeDebugFrame is used (in the next patch for onLeaveFrame trap). > When does the current iter's pc for wasm frames become stale? Not sure I understand the question, but pc() and fp() are update with each iteration (++). > Why is this sorted? Added comment: to return frames in newer->older order. > Where is debugInfo_ defined and how is it used? Is this one of the other patches in the series? Yes, it's introduced by earlier patches. > Huh, wasm isn't MPL? All SM wasm code Apache 2.0 > This assumes that tlsData_ will always be the last field in the struct. Add a static assert to that effect please. Added wasm::Frame to the DebugFrame and added static asserts to ensure tlsData_ is previous field to the frame_. > Is DebugFrame supposed to be explicitly 8-byte aligned or uintptr-aligned? Added comment to the error message -- it's needed for AbstractFramePtr tag. > This method scares me. I'd like the layout assumptions here to be in the header and documented -- or wherever the layout for the wasm baseline frames is currently documented. > > By way of comparison, Ion frame layouts have overlay structs. Using wasm::Frame as a fp(), added its offset constant inside DebugFrame structure. > I don't understand how this function works or why building this vector is necessary. I'll need an explanation of the baseline-compiled code side of things in person. During exception we roll back stack. We need to call onLeaveFrame trap for each observed wasm frame on the exceptions. It's called from via wasm::GenerateThrowStub.

Comment on attachment 8787441 [details] Bug 1286948 - Adds scope and environment for wasm calls. https://reviewboard.mozilla.org/r/76190/#review98988 ::: js/src/gc/Policy.h:55 (Diff revision 14) > class ScriptSourceObject; > class Shape; > class SharedArrayBufferObject; > class StructTypeDescr; > class UnownedBaseShape; > +class WasmFunctionScope; This shouldn't be needed. Subclasses of js::Scope should just work. ::: js/src/gc/Policy.h:96 (Diff revision 14) > D(js::ScriptSourceObject*) \ > D(js::Shape*) \ > D(js::SharedArrayBufferObject*) \ > D(js::StructTypeDescr*) \ > D(js::UnownedBaseShape*) \ > + D(js::WasmFunctionScope*) \ This shouldn't be needed. Subclasses of js::Scope should just work. ::: js/src/vm/Scope.h:907 (Diff revision 14) > + BindingName names[1]; > + > + void trace(JSTracer* trc); > + }; > + > + static WasmFunctionScope* create(ExclusiveContext* cx, WasmInstanceObject* instance, uint32_t funcIndex, HandleScope enclosing); Might as well change create to take JSContext* instead of ExclusiveContext* for now, until such time all wasm stuff can be called using an ExclusiveContext*. ::: js/src/vm/Scope.cpp:1163 (Diff revision 14) > + // WasmFunctionScope::Data has GCManagedDeletePolicy because it contains a > + // GCPtr. Destruction of |data| below may trigger calls into the GC. > + Rooted<WasmFunctionScope*> wasmFunctionScope(cx); > + > + { > + Rooted<UniquePtr<Data>> data(cx, NewEmptyScopeData<WasmFunctionScope>(cx)); Add a TODO that you plan to pull the local variable names from wasm eventually.

Comment on attachment 8784889 [details] Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. https://reviewboard.mozilla.org/r/74206/#review98994 ::: js/src/gdb/mozilla/Interpreter.py:19 (Diff revision 15) > self.tJSOp = gdb.lookup_type('JSOp') > self.tScriptFrameIterData = gdb.lookup_type('js::ScriptFrameIter::Data') > self.tInterpreterFrame = gdb.lookup_type('js::InterpreterFrame') > self.tBaselineFrame = gdb.lookup_type('js::jit::BaselineFrame') > self.tRematerializedFrame = gdb.lookup_type('js::jit::RematerializedFrame') > + self.tShadowFrame = gdb.lookup_type('js::wasm::ShadowFrame') Still needs to be changed from ShadowFrame to DebugFrame. ::: js/src/gdb/mozilla/Interpreter.py:79 (Diff revision 15) > if tag == AbstractFramePtr.Tag_RematerializedFrame: > label = 'js::jit::RematerializedFrame' > ptr = ptr.cast(self.itc.tRematerializedFrame.pointer()) > + if tag == AbstractFramePtr.Tag_WasmShadowFrame: > + label = 'js::wasm::ShadowFrame' > + ptr = ptr.cast(self.itc.tShadowFrame.pointer()) ShadowFrame -> DebugFrame ::: js/src/vm/EnvironmentObject.cpp:1298 (Diff revision 15) > } > > // Check if we have left the extent of the initial frame after we've > // settled on a static scope. > - if (frame_ && (!si_ || si_.scope() == frame_.script()->enclosingScope())) > + if (frame_ && (frame_.isWasmDebugFrame() || > + (!si_ || si_.scope() == frame_.script()->enclosingScope()))) Style nit: the ( should be lined up to the 'f' in |frame_.isWasmDebugFrame| ::: js/src/vm/SavedStacks.cpp:1327 (Diff revision 15) > // be in the cache. > if (capture.is<JS::AllFrames>()) > parentIsInCache = iter.hasCachedSavedFrame(); > > auto principals = iter.compartment()->principals(); > - auto displayAtom = iter.isFunctionFrame() ? iter.functionDisplayAtom() : nullptr; > + auto displayAtom = iter.isWasm() || iter.isFunctionFrame() ? iter.functionDisplayAtom() : nullptr; Style nit: could you add () around |iter.isWasm() || iter.isFunctionFrame()|? ::: js/src/vm/Stack.h:1698 (Diff revision 15) > uint8_t* fp_; > wasm::ExitReason exitReason_; > const wasm::TlsData* tlsData_; > > public: > + typedef void* FramePointer; I think a void\* typedef here doesn't help. By overlay struct I meant use the overlay struct pointers here. For example, see things like JitFrameLayout. If there isn't such a struct readily available for wasm frames or one can be easily written, I'd rather go back to using raw void*. ::: js/src/vm/Stack.cpp:1055 (Diff revision 15) > + while (data_.wasmFrames_.fp() != fp) > + ++data_.wasmFrames_; > + > // Update the pc. > data_.pc_ = (jsbytecode*)data_.wasmFrames_.pc(); > - break; > + return; My previous question still stands: why do we need to implement updatePcQuadratic for wasm debug frames? I think I answered it though. For the record, wasm debug frames allows the same kind of re-entry into Debugger code where an existing Debugger.Frame is used. This kind of re-entry requires the pc to be updated. ::: js/src/vm/Stack.cpp:1711 (Diff revision 15) > + if (!p) { > + frame = wasm::DebugFrame::GetInitialized(iter); > + if (!frame) > + return nullptr; > + if (!debugFrames_->add(p, fp, frame) || > + !frame->ensureObservingState(cx_, true)) If debugFrames_->add succeeds and frame->ensureObservingState fails, you should remove the frame from debugFrames_. ::: js/src/vm/Stack.cpp:1714 (Diff revision 15) > + return nullptr; > + if (!debugFrames_->add(p, fp, frame) || > + !frame->ensureObservingState(cx_, true)) > + { > + frame->~DebugFrame(); > + return nullptr; Mixing assigning to |frame| and returning nullptr here is confusing. Either assign to |frame| unconditionally and return at the end of the method, or make |frame| local to the branches and return from there. ::: js/src/vm/Stack.cpp:1757 (Diff revision 15) > + if (!fps.append(e.front().key())) > + return false; > + } > + > + // Ensure order of the tracked frames from newer to older: a newer frame has > + // lesser frame pointer than older one. I assume wasm only runs on platforms where the stack grows downward, right? ::: js/src/vm/Stack.cpp:1761 (Diff revision 15) > + // Ensure order of the tracked frames from newer to older: a newer frame has > + // lesser frame pointer than older one. > + FramePointerVector scratch(cx_); > + if (!scratch.resize(fps.length())) > + return false; > + MOZ_ALWAYS_TRUE(MergeSort(fps.begin(), fps.length(), scratch.begin(), SortComparatorFramePointer())); Could you use a C++ lambda here instead of SortComparatorFramePointer? ::: js/src/wasm/WasmDebugFrame.h:45 (Diff revision 15) > + > + union > + { > + struct > + { > + bool observing_; What's the reason observing_ is not a bitfield? ::: js/src/wasm/WasmDebugFrame.h:82 (Diff revision 15) > + inline void unsetPrevUpToDate() { prevUpToDate_ = false; } > + > + inline bool hasCachedSavedFrame() const { return hasCachedSavedFrame_; } > + inline void setHasCachedSavedFrame() { hasCachedSavedFrame_ = true; } > + > + JSObject* environmentChain(); environmentChain() can do non-trivial work but just looks like a simple getter. I imagine this is because AbstractFramePtr::environmentChain() looks like that. But this is dangerous, because AFP::environmentChain() is infallible and its callers expect it to be infallible. On the other hand wasm::DebugFrame::environmentChain() is fallible. This kind of sucks, but for simplicity for now, I think you should eagerly create an environment in getDebugFrame, so this getter can remain infallible. ::: js/src/wasm/WasmDebugFrame.h:90 (Diff revision 15) > + > + void trace(JSTracer* trc); > + > + static constexpr size_t offsetOfFrame() { return offsetof(DebugFrame, frame_); } > + static constexpr size_t offsetOfTlsData() { return offsetof(DebugFrame, tlsData_); } > + static constexpr size_t offsetOfPc() { return offsetof(DebugFrame, pc_); } Great! ::: js/src/wasm/WasmDebugFrame.h:94 (Diff revision 15) > + static constexpr size_t offsetOfTlsData() { return offsetof(DebugFrame, tlsData_); } > + static constexpr size_t offsetOfPc() { return offsetof(DebugFrame, pc_); } > +}; > + > +static_assert(DebugFrame::offsetOfTlsData() + sizeof(TlsData*) == DebugFrame::offsetOfFrame(), "TLS pointer must be a field just before the wasm frame"); > +static_assert(sizeof(DebugFrame) % 8 == 0 && DebugFrame::offsetOfFrame() % 8 == 0, "DebugFrame and it's portion is 8-bytes aligned for AbstractFramePtr"); Typo: it's -> its Please put the strings on their own line for column width. ::: js/src/wasm/WasmTypes.cpp:121 (Diff revision 15) > static void > WasmHandleDebugThrow() > { > - // TODO call code->handleDebugThrowTrap for all tracked frames. > + WasmActivation* activation = JSRuntime::innermostWasmActivation(); > + > + // We are leaving activation, we need to handle all leave frame events. How about "When throwing from wasm, the entire activation and all its frames are unwound. So, we need to handle all onLeaveFrame events here."

Comment on attachment 8784890 [details] Bug 1286948 - onEnterFrame/onLeaveFrame wasm events and callstack. https://reviewboard.mozilla.org/r/74208/#review99010 This looks better, thanks for the fixes. The two big items remaining: 1. What about onExceptionUnwind? 2. Does JSTRAP_THROW work correctly? 3. What about JSTRAP_RETURN "forced return"? This case was fairly involved to handle in the regular JITs. Also need tests for these different continuation values that the Debugger handlers can return. ::: js/src/doc/Debugger/Debugger.Frame.md:132 (Diff revision 15) > * `"ion"`: a frame running in the optimizing JIT. > > + * `"wasm"`: a frame running in WebAssembly baseline JIT. > + > `this` > : The value of `this` for this frame (a debuggee value). `this` throws for wasm frames. Please update the documentation. ::: js/src/doc/Debugger/Debugger.Frame.md:155 (Diff revision 15) > code. On frames whose `callee` property is not null, this is equal to > `callee.script`. > > `offset` > : The offset of the bytecode instruction currently being executed in > `script`, or `undefined` if the frame's `script` property is `null`. `offset` throws for wasm frames. Please update the documentation. ::: js/src/doc/Debugger/Debugger.Frame.md:277 (Diff revision 15) > <code id="eval">eval(<i>code</i>, [<i>options</i>])</code> > : Evaluate <i>code</i> in the execution context of this frame, and return > a [completion value][cv] describing how it completed. <i>Code</i> is a > - string. If this frame's `environment` property is `null`, throw a > - `TypeError`. All extant handler methods, breakpoints, and > - so on remain active during the call. This function follows the > + string. If this frame's `environment` property is `null` or `type` property > + is `wasmcall`, throw a `TypeError`. All extant handler methods, breakpoints, > + and so on remain active during the call. This function follows the Need something like this for evalWithBindings as well. ::: js/src/jit-test/tests/debug/wasm-06.js:30 (Diff revision 15) > + if (frame.type !== "wasmcall" || frame.script !== wasmScript) > + return; > + onEnterFrameCalled = true; > + frame.environment; > + frame.older; > + frame.type; This test doesn't seem complete. Should there be asserts on frame.environment, frame.older, and frame.type? ::: js/src/vm/Debugger.cpp:2351 (Diff revision 15) > bool shouldRecompileOrInvalidate(JSScript* script) const { > return script->hasBaselineScript() && compartments_.has(script->compartment()); > } > - bool shouldMarkAsDebuggee(ScriptFrameIter& iter) const { > + bool shouldMarkAsDebuggee(FrameIter& iter) const { > // AbstractFramePtr can't refer to non-remateralized Ion frames, so if > // iter refers to one such, we know we don't match. This comment needs to be expanded to include non-debuggee wasm frames. ::: js/src/vm/Debugger.cpp:2412 (Diff revision 15) > script == frame_.asRematerializedFrame()->outerScript(); > } > > - bool shouldMarkAsDebuggee(ScriptFrameIter& iter) const { > + bool shouldMarkAsDebuggee(FrameIter& iter) const { > // AbstractFramePtr can't refer to non-remateralized Ion frames, so if > // iter refers to one such, we know we don't match. This comment also needs to be expanded to include non-debuggee wasm frames. ::: js/src/vm/Debugger.cpp:2443 (Diff revision 15) > return script->hasBaselineScript() && script == script_; > } > - bool shouldMarkAsDebuggee(ScriptFrameIter& iter) const { > + bool shouldMarkAsDebuggee(FrameIter& iter) const { > // AbstractFramePtr can't refer to non-remateralized Ion frames, and > // while a non-rematerialized Ion frame may indeed be running script_, > // we cannot mark them as debuggees until they bail out. This comment also needs to be expanded to include non-debuggee wasm frames. ::: js/src/vm/Debugger.cpp:2590 (Diff revision 15) > MOZ_ASSERT_IF(scripts[i]->isDebuggee(), observing); > FinishDiscardBaselineScript(fop, scripts[i]); > } > > + // Iterate through all wasm instances to find which are needed to be > + // updated. find which are needed to be updated -> find ones that need to be updated ::: js/src/vm/Debugger.cpp:2596 (Diff revision 15) > + for (JSCompartment* c : zone->compartments) { > + for (wasm::Instance* instance : c->wasm.instances()) { > + if (!instance->code().metadata().debugEnabled) > + continue; > + bool enableTrap = observing == Debugger::IsObserving::Observing; > + if (!instance->code().ensureEnterFrameTrapsState(cx, enableTrap)) Does this new patch only toggle once per |code|? I remember you saying that instance-to-code exists in a many-to-one relationship. Is there a problem with calling |ensureEnterFrameTrapState| multiple times on the same code? ::: js/src/vm/Debugger.cpp:7591 (Diff revision 15) > { > MOZ_ASSERT(frame->isLive()); > > AbstractFramePtr referent = DebuggerFrame::getReferent(frame); > + if (referent.isWasmDebugFrame()) { > + // TODO implement onStep support for wasm frames. If you are planning to land this TODO on central, I would rather you throw with a TODO exception here than to silently accept set the handler. ::: js/src/wasm/WasmCode.cpp:987 (Diff revision 15) > + if (!frame) > + return false; > + frame->setIsDebuggee(); > + JSTrapStatus status = Debugger::onEnterFrame(cx, frame); > + if (status != JSTRAP_CONTINUE) > + return false; What about forced returns, i.e. JSTRAP_RETURN. ::: js/src/wasm/WasmCode.cpp:988 (Diff revision 15) > + return false; > + frame->setIsDebuggee(); > + JSTrapStatus status = Debugger::onEnterFrame(cx, frame); > + if (status != JSTRAP_CONTINUE) > + return false; > + return true; Could do |return status == JSTRAP_CONTINUE| here. ::: js/src/wasm/WasmCode.cpp:1009 (Diff revision 15) > Code::handleDebugThrowTrap(WasmActivation* activation, void* fp) > { > - // TODO handle leave frame > + JSContext* cx = activation->cx(); > + > + DebugFrame* frame = activation->lookupDebugFrame(fp); > + Unused << Debugger::onLeaveFrame(cx, frame, nullptr, false); 1. onExceptionUnwind needs to be called as well. 2. Ignoring onLeaveFrame throwing exceptions would make wasm frames behave different than other frames. I don't think it's correct to ignore exceptions here.

Comment on attachment 8792075 [details] Bug 1286948 - Adds debug mode for WASM baseline compiler. https://reviewboard.mozilla.org/r/79304/#review99996 ::: js/src/wasm/WasmBaselineCompile.cpp:499 (Diff revision 10) > int32_t localSize_; // Size of local area in bytes (stable after beginFunction) > int32_t varLow_; // Low byte offset of local area for true locals (not parameters) > int32_t varHigh_; // High byte offset + 1 of local area for true locals > int32_t maxFramePushed_; // Max value of masm.framePushed() observed > bool deadCode_; // Flag indicating we should decode & discard the opcode > + bool debugInfo_; We currently name this bool debugInfo/debugEnabled/debug/debuggee at different points. It'd be nice to pick a single name, say, debugEnabled, and use it everywhere. ::: js/src/wasm/WasmCode.h:449 (Diff revision 10) > CodeRangeVector codeRanges; > CallSiteVector callSites; > CallThunkVector callThunks; > NameInBytecodeVector funcNames; > CacheableChars filename; > + bool debugEnabled; Initially I was going to suggest moving debugEnabled to MetadataCacheablePod but I remembered that we should never be serializing any debugEnabled module. Could you MOZ_ASSERT(!debugEnabled) at the end of Metadata::serialize and deserialize? ::: js/src/wasm/WasmCode.cpp:711 (Diff revision 10) > JSString* > Code::createText(JSContext* cx) > { > StringBuffer buffer(cx); > - if (maybeBytecode_) { > + if (metadata_->debugEnabled) { > + MOZ_ASSERT(maybeBytecode_); IIUC, debugEnabled will only be true when the debugger is open whereas bytecode will be present any time the devconsole is open so we should keep the original code here. ::: js/src/wasm/WasmGenerator.h:268 (Diff revision 10) > MOZ_MUST_USE bool startFuncDef(uint32_t lineOrBytecode, FunctionGenerator* fg); > MOZ_MUST_USE bool finishFuncDef(uint32_t funcIndex, FunctionGenerator* fg); > MOZ_MUST_USE bool finishFuncDefs(); > > + // Debug mode: > + void setDebug(bool enabled); This function seems to be dead. ::: js/src/wasm/WasmModule.cpp:886 (Diff revision 10) > // developer actually cares: when the compartment is debuggable (which is > // true when the web console is open) or a names section is present (since > // this going to be stripped for non-developer builds). > > const ShareableBytes* maybeBytecode = nullptr; > - if (cx->compartment()->isDebuggee() || !metadata_->funcNames.empty()) > + if (metadata_->debugEnabled || !metadata_->funcNames.empty()) I think we want to keep this as-is for the reason mentioned earlier.

(In reply to Luke Wagner [:luke] from comment #134) > > + bool debugEnabled; > > Initially I was going to suggest moving debugEnabled to MetadataCacheablePod > but I remembered that we should never be serializing any debugEnabled > module. Could you MOZ_ASSERT(!debugEnabled) at the end of > Metadata::serialize and deserialize? Actually, there is nothing preventing this assert from firing in this patch. I think what we need to do is: 1. in Module::serializedSize(), if debugEnabled, set *maybeCompiledSize to 0. 2. in Module::serialize(), if debugEnabled, write nothing This way, the compiled debug code is not saved, only the bytecode and, on deserialization, Module::assumptionsMatch() will return false and the module will be naturally recompiled from scratch by wasm::DeserializeModule(). That still leaves the problem that deserializing a Module will always produce non-debug code, but that fix requires a JSAPI tweak so that IDB can tell us whether the originating call was in a debuggerObservesAsmJS compartment.

Comment on attachment 8784887 [details] Bug 1286948 - Adds prolog and epilog debug traps and handlers. https://reviewboard.mozilla.org/r/74202/#review100004 Ah, looking much simpler, great! ::: js/src/jit/MacroAssembler.h:524 (Diff revision 15) > static void patchNearJumpToNop(uint8_t* jump) PER_SHARED_ARCH; > > + CodeOffset toggledPatchableCall(const wasm::CallSiteDesc& desc, bool enabled) PER_SHARED_ARCH; > + void patchToggledCall(uint32_t callOrNop, uint32_t targetOffset) PER_SHARED_ARCH; > + static void patchToggledToCall(uint8_t* call, uint8_t* target) PER_SHARED_ARCH; > + static void patchToggledToNop(uint8_t* call) PER_SHARED_ARCH; The 'bool enabled' parameter is always false, so it can be removed. With that change, patchToggledCall() can be removed b/c it will always be given a nop for which it does nothing. With those two changes, these methods are all quite symmetric with the above so could you rename to nopPatchableToCall/patchNopToCall/patchCallToNop? ::: js/src/wasm/WasmBaselineCompile.cpp:2091 (Diff revision 15) > } > + > + if (debugInfo_) { > + const bool beginDebugTrapEnabled = false; > + const uint32_t beginOffset = 0; > + CallSiteDesc desc(beginOffset, CallSiteDesc::EnterFrame); Here and below, can you pass iter\_.currentOffset() here instead of beginOffset/endOffset hardcoded to 0? ::: js/src/wasm/WasmCode.h:430 (Diff revision 15) > memoryUsage(MemoryUsage::None), > minMemoryLength(0) > {} > }; > > +typedef Vector<uint32_t, 0, SystemAllocPolicy> DebugTrapFarJumpsVector; nit: DebugTrapFarJumpVector (no 's') ::: js/src/wasm/WasmCode.h:454 (Diff revision 15) > CallSiteVector callSites; > CallThunkVector callThunks; > NameInBytecodeVector funcNames; > CacheableChars filename; > bool debugEnabled; > + DebugTrapFarJumpsVector debugTrapFarJumpOffsets; nit: can you realign columns to account for this new longer typename. As with the prev patch, could you also add asserts to Metadata::serialize/deserialize that debugTrapFarJumpOffsets.empty()? Also, now that we have two debugX fields, could you put a \n before debugEnabled and put a little comment like: // Debug-enabled code is not serialized. ::: js/src/wasm/WasmCode.h:557 (Diff revision 15) > // object will be shared between a module and all its instances. > > class Code > { > + typedef HashMap<uint32_t, uint32_t> LeaveFrameAddressCounters; > + typedef mozilla::Pair<const CallSite*, const CallSite*> CallSiteRange; Can you move this typedef to right under the definition of CallSite (next to the CallSiteVector decl) in WasmTypes.h? ::: js/src/wasm/WasmCode.h:568 (Diff revision 15) > CacheableCharsVector funcLabels_; > bool profilingEnabled_; > + bool enterFrameTrapsEnabled_; > + > + // Recording all debuggers requests to react on leave frames. > + UniquePtr<LeaveFrameAddressCounters> leaveFrameCounters_; I used to stuff as many class/typedefs inside classes as I could with asm.js but now I try to keep classes small by pulling these types out into their own little sections. Somewhere above class Code, could you make a section like: \n // The LeaveFrameAddressCounters records all ... \n typedef HashMap... typedef UniquePtr... \n ::: js/src/wasm/WasmCode.h:630 (Diff revision 15) > size_t* data) const; > > WASM_DECLARE_SERIALIZABLE(Code); > + > +private: > + void toggleDebugTrap(uint32_t offset, bool enabled); nit: could you move this decl up to the top of the class before the public: label? ::: js/src/wasm/WasmCode.cpp:645 (Diff revision 15) > +{ > + size_t lowerBound = 0; > + size_t upperBound = metadata_->callSites.length(); > + size_t beginMatch, endMatch; > + if (BinarySearch(CallSiteRetAddrOffset(metadata_->callSites), lowerBound, upperBound, range->begin(), &beginMatch)) > + beginMatch++; Because of prologues/epilogues, I think the BinarySearch() should never return true, so I think you can remove the if(). Also, I expect both lines are longer than 99 chars, so to fix that, could you hoist up a CallSiteRetAddrOffset matchRetAddr(metadata_->callSites); and reuse that? ::: js/src/wasm/WasmCode.cpp:648 (Diff revision 15) > + size_t beginMatch, endMatch; > + if (BinarySearch(CallSiteRetAddrOffset(metadata_->callSites), lowerBound, upperBound, range->begin(), &beginMatch)) > + beginMatch++; > + BinarySearch(CallSiteRetAddrOffset(metadata_->callSites), beginMatch, upperBound, range->end(), &endMatch); > + > + return Code::CallSiteRange(&metadata_->callSites[beginMatch], &metadata_->callSites[endMatch]); I think callSites[endMatch] might assert if endMatch == callSites.length() so I'd use pointer arith. To keep the line under 100 chars, I'd factor out metadata_.callSites.begin(). ::: js/src/wasm/WasmGenerator.cpp:368 (Diff revision 15) > break; > } > + case CallSiteDesc::EnterFrame: > + case CallSiteDesc::LeaveFrame: { > + DebugTrapFarJumpsVector& jumps = metadata_->debugTrapFarJumpOffsets; > + if (jumps.length() == 0 || jumps.empty() ::: js/src/wasm/WasmGenerator.cpp:369 (Diff revision 15) > } > + case CallSiteDesc::EnterFrame: > + case CallSiteDesc::LeaveFrame: { > + DebugTrapFarJumpsVector& jumps = metadata_->debugTrapFarJumpOffsets; > + if (jumps.length() == 0 || > + uint32_t(abs(int32_t(jumps[jumps.length() - 1]) - int32_t(callerOffset))) >= JumpRange()) { jumps.back() ::: js/src/wasm/WasmGenerator.cpp:384 (Diff revision 15) > + if (!debugTrapFarJumps_.emplaceBack(jumpOffset)) > + return false; > + if (!jumps.emplaceBack(offsets.begin)) > + return false; > + } > + uint32_t calleeOffset = jumps[jumps.length() - 1]; jumps.back() ::: js/src/wasm/WasmGenerator.cpp:1169 (Diff revision 15) > MOZ_ASSERT(codeRange.begin() >= lastEnd); > lastEnd = codeRange.end(); > } > #endif > > + // TODO Assert debugTrapFarJumpOffsets are sorted. (Other than a bunch in WasmBaseline) we try not to land TODOs in wasm/. ::: js/src/wasm/WasmStubs.cpp:1137 (Diff revision 15) > offsets.begin = masm.currentOffset(); > > + masm.andToStackPtr(Imm32(~(ABIStackAlignment - 1))); > + if (ShadowStackSpace) > + masm.subFromStackPtr(Imm32(ShadowStackSpace)); > + masm.call(SymbolicAddress::HandleDebugThrow); Could you remove SymbolicAddress::HandleDebugThrow (and all the associated) TODOs from this patch? ::: js/src/wasm/WasmTypes.cpp:109 (Diff revision 15) > + WasmActivation* activation = JSRuntime::innermostWasmActivation(); > + > + FrameIterator iter(*activation); > + void* pc = iter.pc(); > + MOZ_ASSERT(pc); > + Code* code = activation->compartment()->wasm.lookupCode(pc); Note that the Code returned by lookupCode is already in iter.code_. I'd really like to kill FrameIter::pc_/fp_ and just use more meaningful semantic results instead. ::: js/src/wasm/WasmTypes.cpp:111 (Diff revision 15) > + FrameIterator iter(*activation); > + void* pc = iter.pc(); > + MOZ_ASSERT(pc); > + Code* code = activation->compartment()->wasm.lookupCode(pc); > + bool result = code->handleDebugTrap(activation); > + activation->setResumePC(nullptr); I think setResumePC(nullptr) isn't necessary anymore?

Comment on attachment 8784888 [details] Bug 1286948 - Allows FrameIterator to return Instance and track actual fp. https://reviewboard.mozilla.org/r/74204/#review100342 ::: js/src/wasm/WasmInstance.cpp:650 (Diff revision 15) > // Push a WasmActivation to describe the wasm frames we're about to push > // when running this module. Additionally, push a JitActivation so that > // the optimized wasm-to-Ion FFI call path (which we want to be very > // fast) can avoid doing so. The JitActivation is marked as inactive so > // stack iteration will skip over it. > - WasmActivation activation(cx); > + WasmActivation activation(cx, &tlsData_); Unfortunately, a single WasmActivation can contain a chain of calls between multiple (same-compartment) Instances which can thus have different tlsData_s. This can happen via call_indirect and calls to imports. I'm afraid what is necessary instead is saving the TlsData\* in the wasm::Frame. I actually think we should do this for *all* wasm::Frames (for a number of reasons) so the right place might be GenerateFunctionPrologue.

Comment on attachment 8787441 [details] Bug 1286948 - Adds scope and environment for wasm calls. https://reviewboard.mozilla.org/r/76190/#review98988 > This shouldn't be needed. Subclasses of js::Scope should just work. Linker fails with ```text Undefined symbols for architecture x86_64: "bool js::gc::IsAboutToBeFinalized<js::WasmFunctionScope*>(js::ReadBarrieredBase<js::WasmFunctionScope*>*)", referenced from: JS::GCPolicy<js::ReadBarriered<js::WasmFunctionScope*> >::needsSweep(js::ReadBarriered<js::WasmFunctionScope*>*) in Unified_cpp_js_src39.o ```

Comment on attachment 8784889 [details] Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. https://reviewboard.mozilla.org/r/74206/#review98994 > I think a void\* typedef here doesn't help. By overlay struct I meant use the overlay struct pointers here. For example, see things like JitFrameLayout. > > If there isn't such a struct readily available for wasm frames or one can be easily written, I'd rather go back to using raw void*. I changed the code to not map frames the same way. Now we just store initialized DebugFrame in the `debugFrames_` set for faster GC trace. > I assume wasm only runs on platforms where the stack grows downward, right? The code was modified to not require any sorting. > environmentChain() can do non-trivial work but just looks like a simple getter. I imagine this is because AbstractFramePtr::environmentChain() looks like that. > > But this is dangerous, because AFP::environmentChain() is infallible and its callers expect it to be infallible. On the other hand wasm::DebugFrame::environmentChain() is fallible. > > This kind of sucks, but for simplicity for now, I think you should eagerly create an environment in getDebugFrame, so this getter can remain infallible. The DebugFrame::Create now creates/initializes the environmentChain. > How about "When throwing from wasm, the entire activation and all its frames are unwound. So, we need to handle all onLeaveFrame events here." Code changes in the WasmHandleDebugThrow() -- see "Add prolog and elipog traps..." patch

Comment on attachment 8784887 [details] Bug 1286948 - Adds prolog and epilog debug traps and handlers. https://reviewboard.mozilla.org/r/74202/#review100004 > nit: can you realign columns to account for this new longer typename. > > As with the prev patch, could you also add asserts to Metadata::serialize/deserialize that debugTrapFarJumpOffsets.empty()? > > Also, now that we have two debugX fields, could you put a \n before debugEnabled and put a little comment like: > // Debug-enabled code is not serialized. Realigned only names in the the sections with `debugEnabled` and `debugTrapFarJumpOffsets`. > (Other than a bunch in WasmBaseline) we try not to land TODOs in wasm/. Implemented "Assert debugTrapFarJumpOffsets are sorted." > Could you remove SymbolicAddress::HandleDebugThrow (and all the associated) TODOs from this patch? HandleDebugThrow is needed to properly handle onLeaveFrame and onExceptionUnwind debugger events. > Note that the Code returned by lookupCode is already in iter.code_. I'd really like to kill FrameIter::pc_/fp_ and just use more meaningful semantic results instead. The code was rafactored to just use callSite instead.

Comment on attachment 8784890 [details] Bug 1286948 - onEnterFrame/onLeaveFrame wasm events and callstack. https://reviewboard.mozilla.org/r/74208/#review99010 Added code to handle onExceptionUnwind and folder Code::handleDebugTrap into WasmHandleDebugTrap so JSTRAP_ERROR/JSTRAP_THROW will work as well. Defering implementation of JSTRAP_RETURN for future work (as well as setReturnValue) -- currently it's raising an error. Additional tests are added to test different continuation values.

(In reply to Yury Delendik (:yury) from comment #143) > Comment on attachment 8787441 [details] > Bug 1286948 - Adds scope and environment for wasm calls. > > https://reviewboard.mozilla.org/r/76190/#review98988 > > > This shouldn't be needed. Subclasses of js::Scope should just work. > > Linker fails with > > ```text > Undefined symbols for architecture x86_64: > "bool > js::gc::IsAboutToBeFinalized<js::WasmFunctionScope*>(js:: > ReadBarrieredBase<js::WasmFunctionScope*>*)", referenced from: > JS::GCPolicy<js::ReadBarriered<js::WasmFunctionScope*> > >::needsSweep(js::ReadBarriered<js::WasmFunctionScope*>*) in > Unified_cpp_js_src39.o > ``` That's odd... do you know which file that is? Is it not including "vm/Scope.h"?

Comment on attachment 8784889 [details] Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. https://reviewboard.mozilla.org/r/74206/#review101416 ::: js/src/vm/Stack.h:1689 (Diff revision 17) > // all kinds of jit code. > class WasmActivation : public Activation > { > + friend void TraceWasmActivations(JSRuntime* rt, JSTracer* trc); > + > + typedef HashSet<wasm::DebugFrame*> DebugFrameSet; Style nit: I think we prefer using new "using" style typedefs now, like using DebugFrameSet = HashSet<wasm::DebugFrame*> ::: js/src/vm/Stack.cpp:13 (Diff revision 17) > > #include "mozilla/PodOperations.h" > > #include "jscntxt.h" > > +#include "ds/Sort.h" Don't need to include Sort.h anymore. ::: js/src/vm/Stack.cpp:1689 (Diff revision 17) > + > +wasm::DebugFrame* > +WasmActivation::getDebugFrame(const wasm::FrameIterator& iter) > +{ > + MOZ_ASSERT(!iter.done() && iter.isDebuggee()); > + if (wasm::DebugFrame* frame = wasm::DebugFrame::MaybeInitialized(iter)) Please assert that if you get an initialized frame, that frame is in debugFrames_. Also add a comment stating the invariant that initialized <=> in debugFrames_. ::: js/src/vm/Stack.cpp:1734 (Diff revision 17) > + if (!debugFrames_) > + return; > + if (wasm::DebugFrame* frame = wasm::DebugFrame::MaybeInitialized(iter)) { > + MOZ_ALWAYS_TRUE(frame->ensureObservingState(cx_, false)); > + frame->~DebugFrame(); > + debugFrames_->remove(frame); Similarly, please assert that frame is actually in debugFrames_.

Comment on attachment 8784890 [details] Bug 1286948 - onEnterFrame/onLeaveFrame wasm events and callstack. https://reviewboard.mozilla.org/r/74208/#review101426 I'd add a bunch of TODOs in the comments around JSTRAP_RETURN so they're easier to grep later. ::: js/src/doc/Debugger/Debugger.Frame.md:132 (Diff revisions 15 - 17) > * `"ion"`: a frame running in the optimizing JIT. > > * `"wasm"`: a frame running in WebAssembly baseline JIT. > > `this` > -: The value of `this` for this frame (a debuggee value). > +: The value of `this` for this frame (a debuggee value). For `wasmcall` frame, Nit: For a `wasmcall` frame ::: js/src/doc/Debugger/Debugger.Frame.md:157 (Diff revisions 15 - 17) > `callee.script`. > > `offset` > : The offset of the bytecode instruction currently being executed in > `script`, or `undefined` if the frame's `script` property is `null`. > + For `wasmcall` frame, this property throws a `TypeError`. Nit: For a `wasmcall` frame ::: js/src/doc/Debugger/Debugger.Frame.md:335 (Diff revisions 15 - 17) > prevent SpiderMonkey from extending this frame's environment as > requested, this call throws an `Error` exception. > > The <i>options</i> argument is as for > [`Debugger.Frame.prototype.eval`][fr eval], described above. > + Also like `eval`, if this frame's `environment` property is `null` or Nit: trailing whitespace ::: js/src/jit-test/tests/debug/wasm-06.js:167 (Diff revisions 15 - 17) > + } > +); > + > +// Checking if onEnterFrame/onExceptionUnwind work during exceptions. > +runWasmWithDebugger( > + '(module (func (unreachable)) (export "test" 0))', undefined, What causes an error to be thrown here? ::: js/src/jit-test/tests/debug/wasm-06.js:284 (Diff revisions 15 - 17) > + } > +); > > -assertEq(onEnterFrameCalled, true); > -assertEq(onLeaveFrameCalled, true); > -assertEq(wasException, false); > + > +// Checking resumption values for JS_RETURN (not implemented by wasm baseline). > +runWasmWithDebugger( The usual thing to do would be to write the test as if it were going to pass, but assert that it fails with a message saying it is NYI, and that if it's passing because you have implemented the feature, invert the condition. ::: js/src/vm/Debugger.cpp:2351 (Diff revisions 15 - 17) > bool shouldRecompileOrInvalidate(JSScript* script) const { > return script->hasBaselineScript() && compartments_.has(script->compartment()); > } > bool shouldMarkAsDebuggee(FrameIter& iter) const { > - // AbstractFramePtr can't refer to non-remateralized Ion frames, so if > - // iter refers to one such, we know we don't match. > + // AbstractFramePtr can't refer to non-remateralized Ion frames or > + // non-debuggee wasm frame, so if iter refers to one such, we know we Typo: wasm frames ::: js/src/vm/Debugger.cpp:2413 (Diff revisions 15 - 17) > script == frame_.asRematerializedFrame()->outerScript(); > } > > bool shouldMarkAsDebuggee(FrameIter& iter) const { > - // AbstractFramePtr can't refer to non-remateralized Ion frames, so if > - // iter refers to one such, we know we don't match. > + // AbstractFramePtr can't refer to non-remateralized Ion frames or > + // non-debuggee wasm frame, so if iter refers to one such, we know we Typo: wasm frames ::: js/src/jit-test/tests/debug/wasm-06-onEnterFrame-null.js:7 (Diff revision 17) > +// Checking resumption values for 'null' at onEnterFrame. > + > +load(libdir + "asserts.js"); > + > +if (!wasmIsSupported()) > + quit(); This test will fail if !wasmIsSupported(), since the exit status won't be 3. Also I'm not 100% that Windows will give you 3? Should check. ::: js/src/jit-test/tests/debug/wasm-06-onPop-null.js:7 (Diff revision 17) > +// Checking resumption values for 'null' at frame's onPop. > + > +load(libdir + "asserts.js"); > + > +if (!wasmIsSupported()) > + quit(); This test will also fail if !wasmIsSupported(), since the exit status won't be 3. ::: js/src/wasm/WasmTypes.cpp:120 (Diff revision 17) > frame->setIsDebuggee(); > - // TODO call onEnterFrame > - return true; > + JSTrapStatus status = Debugger::onEnterFrame(cx, frame); > + if (status == JSTRAP_RETURN) { > + // Ignoring forced return (JSTRAP_RETURN) -- changing code execution > + // order is not yet implemented in the wasm baseline. > + JS_ReportErrorASCII(cx, "Unexpected resumption value from onEnterFrame"); Are you planning to implement this after the MVP, or in a later patch? ::: js/src/wasm/WasmTypes.cpp:144 (Diff revision 17) > { > WasmActivation* activation = JSRuntime::innermostWasmActivation(); > + JSContext* cx = activation->cx(); > > for (FrameIterator iter(*activation); !iter.done(); ++iter) { > if (!iter.isDebuggee()) As we talked about on IRC, iter.isDebuggee confused me. Maybe iter.codeHasDebugInstrumentation. ::: js/src/wasm/WasmTypes.cpp:149 (Diff revision 17) > if (!iter.isDebuggee()) > continue; > > DebugFrame* frame = activation->getDebugFrame(iter); > if (!frame) > continue; // continue unwind when debug frame construction fails. Style nit: capitalize and move // comment above if (!frame) ::: js/src/wasm/WasmTypes.cpp:158 (Diff revision 17) > + // is not yet implemented in the wasm baseline. > + JS_ReportErrorASCII(cx, "Unexpected resumption value from onExceptionUnwind"); > + } > + bool ok = Debugger::onLeaveFrame(cx, frame, nullptr, false); > + if (ok) { > + // Unexpected success from the handler onLeaveFrame -- rasing error Typo: raising

Comment on attachment 8792075 [details] Bug 1286948 - Adds debug mode for WASM baseline compiler. https://reviewboard.mozilla.org/r/79304/#review101718 Looks good, thanks! ::: js/src/wasm/WasmBaselineCompile.cpp:7759 (Diff revision 12) > > JitContext jitContext(&results.alloc()); > > // One-pass baseline compilation. > > - BaseCompiler f(task->env(), d, func, locals, results); > + BaseCompiler f(task->env(), d, func, locals, debugEnabled, results); Since there's only one use, I'd just pass task->debugEnabled() directly. ::: js/src/wasm/WasmCompile.cpp:106 (Diff revision 12) > alwaysBaseline = cx->options().wasmAlwaysBaseline(); > + > + // Debug information such as source view or debug traps will require > + // additional memory, so we try to only enabled it when a developer > + // actually cares: when the compartment is debuggable (which is true when > + // the web console is open at debugger tab). s/enabled/enable/ Also, I'd add to the first sentence "... require additional memory and permanently stay in baseline code, ..." and change "when the compartment is debuggable (...)" to "when the debugger tab is open". ::: js/src/wasm/WasmGenerator.cpp:1118 (Diff revision 12) > metadata_->boundsChecks.podResizeToFit(); > metadata_->codeRanges.podResizeToFit(); > metadata_->callSites.podResizeToFit(); > metadata_->callThunks.podResizeToFit(); > > + metadata_->debugEnabled = debugEnabled_; I'd move this after the tables.resize() below (to keep all the "shrink to fit" operations together).

Comment on attachment 8787441 [details] Bug 1286948 - Adds scope and environment for wasm calls. https://reviewboard.mozilla.org/r/76190/#review98988 > Linker fails with > > ```text > Undefined symbols for architecture x86_64: > "bool js::gc::IsAboutToBeFinalized<js::WasmFunctionScope*>(js::ReadBarrieredBase<js::WasmFunctionScope*>*)", referenced from: > JS::GCPolicy<js::ReadBarriered<js::WasmFunctionScope*> >::needsSweep(js::ReadBarriered<js::WasmFunctionScope*>*) in Unified_cpp_js_src39.o > ``` WasmJS.h uses WasmFunctionScope as `ReadBarriered<WasmFunctionScope*>`. Adding #include "vm/Scope.h" does not fix the issue.

Comment on attachment 8784890 [details] Bug 1286948 - onEnterFrame/onLeaveFrame wasm events and callstack. https://reviewboard.mozilla.org/r/74208/#review101426 > Are you planning to implement this after the MVP, or in a later patch? I guess the answer will be "in a later patch". There is no specific time for this feature, however I assume we want to make Debugger API complete for wasm. We need to modify wasm baseline compiler to handle this kind (JS_RETURN) of execution order change.

Comment on attachment 8787441 [details] Bug 1286948 - Adds scope and environment for wasm calls. https://reviewboard.mozilla.org/r/76190/#review101744 I was scanning this patch to understand its interaction with the others and had a few small nits to WasmJS.(h,cpp): ::: js/src/wasm/WasmJS.h:202 (Diff revision 17) > uint32_t funcIndex, > MutableHandleFunction fun); > > const wasm::CodeRange& getExportedFunctionCodeRange(HandleFunction fun); > + > + Scope* funcScope(JSContext* cx, uint32_t funcIndex); Following the style of `getExportedFunction`, could `funcScope` be made static, taking a `HandleWasmInstanceObject` (using typedef in WasmTypes.h) and renamed to `getFunctionScope`? This removes the hazard of a stale `this` value. Then `getOrCreateFuncScope` would be removed. ::: js/src/wasm/WasmJS.cpp:1100 (Diff revision 17) > const Metadata& metadata = instance().metadata(); > return metadata.codeRanges[metadata.lookupFuncExport(funcIndex).codeRangeIndex()]; > } > > +Scope* > +WasmInstanceObject::innermostScope(JSContext* cx, void* pc) I think a more accurate name would be `getScopeForFunctionContainingPC`. ::: js/src/wasm/WasmJS.cpp:1128 (Diff revision 17) > + Rooted<Scope*> globalScope(cx, &instance->global().emptyGlobalScope()); > + > + JSAutoCompartment ac(cx, instance); > + Rooted<WasmFunctionScope*> funcScope(cx, WasmFunctionScope::create(cx, instance, funcIndex, globalScope)); > + > + if (!funcScope) nit: can you remove the \n between alloc and null-check?

Comment on attachment 8784889 [details] Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. https://reviewboard.mozilla.org/r/74206/#review101748 Scanning this patch also for background, I had a few comments: ::: js/src/vm/Stack.cpp:1759 (Diff revision 19) > + if (!act->isWasm()) > + continue; > + WasmActivation* wasmActivation = act->asWasm(); > + if (!wasmActivation->debugFrames_) > + continue; > + for (WasmActivation::DebugFrameSet::Enum e(*wasmActivation->debugFrames_); !e.empty(); e.popFront()) { I'd much prefer not to need a DebugFrameSet and instead use "normal" stack walking via FrameIter. Specifically: \* move the tlsData field into wasm::Frame and use this to mark the WasmInstanceObject (instead of the current wasm::Compartment::trace() hack) \* have wasm::Frame::isDebug() const \{ return tlsData->instance().metadata().debugEnabled(); \} and wasm::DebugFrame\* wasm::Frame::asDebug() \{ MOZ_ASSERT(isDebug()); ...\} \* use these helpers to conditionally mark the additional GC fields of wasm::DebugFrame Would this work? I can write a separate patch for the first bullet. ::: js/src/wasm/WasmDebugFrame.h:59 (Diff revision 19) > + int64_t resultI64_; > + float resultF32_; > + double resultF64_; > + }; > + > + HeapPtr<JSObject*> env_; Since this is on the stack, I don't think we need any barriers, we can store a raw JSObject* and call TraceRoot() (like BaselineFrame::trace() etc). Given that, what I'd like is if wasm::DebugFrame was a POD (no dtor) that was initialized to a valid initial state when entering the frame, so there was no Create()/MaybeInitialized(), but rather member functions of wasm::DebugFrame::isX()/isY() that queried the state of the (existing) wasm::DebugFrame.

Comment on attachment 8784887 [details] Bug 1286948 - Adds prolog and epilog debug traps and handlers. https://reviewboard.mozilla.org/r/74202/#review101720 Thanks for all the work on this, r+! (I'd still like to discuss some of the other patches commented on above to make sure I understand it all.) ::: js/src/jit/MacroAssembler.h:529 (Diff revision 17) > static void patchNopToNearJump(uint8_t* jump, uint8_t* target) PER_SHARED_ARCH; > static void patchNearJumpToNop(uint8_t* jump) PER_SHARED_ARCH; > > + CodeOffset nopPatchableToCall(const wasm::CallSiteDesc& desc) PER_SHARED_ARCH; > + static void patchNopToCall(uint8_t* call, uint8_t* target) PER_SHARED_ARCH; > + static void patchCallToNop(uint8_t* call) PER_SHARED_ARCH; Can you add a comment to this group of functions symmetric to the one above nopPatchableToNearJump()? ::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:748 (Diff revision 17) > > +CodeOffset > +MacroAssembler::nopPatchableToCall(const wasm::CallSiteDesc& desc) > +{ > + CodeOffset offset(size()); > + JmpSrc src = masm.cmp_eax(); Could you use masm.nop_five()? It's the official Intel-recommended nop sequence (see comment and Table 4-9 in Intel manual) and thus might be cheaper deep-down in the microarchitecture. For symmetry, could you then add Assembler::patchFiveByteNopToCall/patchFiveByteNopToCall to BaseAssembler-x86-shared.h right below the TwoByte versions? ::: js/src/wasm/WasmBaselineCompile.cpp:2109 (Diff revision 17) > } > + > + if (debugEnabled_) { > + const uint32_t beginOffset = iter_.currentOffset(); > + CallSiteDesc desc(beginOffset, CallSiteDesc::EnterFrame); > + masm.nopPatchableToCall(desc); Could you factor out these 3 lines into some insertBreakablePoint(CallSiteDesc::Kind) helper function used here and below? ::: js/src/wasm/WasmBaselineCompile.cpp:2109 (Diff revision 17) > } > + > + if (debugEnabled_) { > + const uint32_t beginOffset = iter_.currentOffset(); > + CallSiteDesc desc(beginOffset, CallSiteDesc::EnterFrame); > + masm.nopPatchableToCall(desc); ::: js/src/wasm/WasmBaselineCompile.cpp:2118 (Diff revision 17) > + void saveResult() { > + MOZ_ASSERT(debugEnabled_); > + size_t debugFrameOffset = masm.framePushed() - DebugFrame::offsetOfFrame(); > + Address resultsAddress(StackPointer, debugFrameOffset + DebugFrame::offsetOfResults()); > + ExprType type = func_.sig().ret(); > + switch (type) { nit: switch (func\_.sig().ret()) (here and below) ::: js/src/wasm/WasmBaselineCompile.cpp:2140 (Diff revision 17) > + masm.storeFloat32(RegF32(ReturnFloat32Reg), resultsAddress); > + break; > + } > + default: { > + MOZ_CRASH("Function return type"); > + } nit (here and below): none of the cases require {} so can you remove all the {}s? ::: js/src/wasm/WasmBaselineCompile.cpp:2218 (Diff revision 17) > + CallSiteDesc desc(endOffset, CallSiteDesc::LeaveFrame); > + masm.nopPatchableToCall(desc); > + > + // Restoring return value into the registers just after the leave > + // frame debug trap. > + restoreResult(); nit: Given the abovementioned factoring, how about grouping the remaining three lines into one commented paragraph: ``` // Store and reload the return value from DebugFrame::return so that // it can be clobbered, and/or modified by the debug trap. saveReturnRegister(); insertBreakablePoint() restoreReturnRegister(); ``` ::: js/src/wasm/WasmCode.h:478 (Diff revision 17) > CustomSectionVector customSections; > CacheableChars filename; > > // Debug-enabled code is not serialized. > - bool debugEnabled; > + bool debugEnabled; > + DebugTrapFarJumpVector debugTrapFarJumpOffsets; nit: Since the field name contains the key "Offsets" noun, might as well just use the common Uint32Vector typedef instead of adding a new one. ::: js/src/wasm/WasmCode.h:575 (Diff revision 17) > > typedef UniquePtr<GeneratedSourceMap> UniqueGeneratedSourceMap; > > +// The LeaveFrameAddressCounters records all requests to enable leave frame > +// debug traps. > +typedef HashMap<uint32_t, uint32_t> LeaveFrameAddressCounters; nit: \n between comment and typedef for symmetry with top-level style of this file ::: js/src/wasm/WasmCode.h:590 (Diff revision 17) > const SharedMetadata metadata_; > const SharedBytes maybeBytecode_; > UniqueGeneratedSourceMap maybeSourceMap_; > CacheableCharsVector funcLabels_; > + uint32_t enterFrameTrapsCounter_; > + UniquePtr<LeaveFrameAddressCounters> leaveFrameCounters_; Instead of a UniquePtr, can you store the LeaveFrameAddressCounters by value and use leaveFrameCounters_.initialized() as the "has this map been initialized yet" test? ::: js/src/wasm/WasmStubs.cpp:1172 (Diff revision 17) > + masm.setFramePushed(0); > + > + ProfilingOffsets offsets; > + GenerateExitPrologue(masm, 0, ExitReason::None, &offsets); > + > + // Saving all registers used between baseline compiler operations. nit: /Saving/Save/ ::: js/src/wasm/WasmCode.cpp:866 (Diff revision 20) > > void > +Code::toggleDebugTrap(uint32_t offset, bool enabled) > +{ > + MOZ_ASSERT(offset); > + uint8_t* debugTrapOffset = segment_->base() + offset; nit: this variable holds the pointer, not an offset, so I'd name it `trap`. ::: js/src/wasm/WasmCode.cpp:871 (Diff revision 20) > + uint8_t* debugTrapOffset = segment_->base() + offset; > + if (enabled) { > + MOZ_ASSERT(metadata_->debugTrapFarJumpOffsets.length() > 0); > + size_t i = 0; > + while (i < metadata_->debugTrapFarJumpOffsets.length() && > + offset < metadata_->debugTrapFarJumpOffsets[i]) i++; Can you hoist a `Uint32Vector& farJumpOffsets = metadata_->debugTrapFarJumpOffsets` at the top of this function and use it everywhere? Then this while-condition and the below if-condition should fit on one line (which conforms with SM style: if you have a multi-line condition the body needs to be in curlies). ::: js/src/wasm/WasmCode.cpp:875 (Diff revision 20) > + while (i < metadata_->debugTrapFarJumpOffsets.length() && > + offset < metadata_->debugTrapFarJumpOffsets[i]) i++; > + if (i >= metadata_->debugTrapFarJumpOffsets.length() || > + (i > 0 && offset - metadata_->debugTrapFarJumpOffsets[i - 1] < metadata_->debugTrapFarJumpOffsets[i] - offset)) > + i--; > + uint8_t* debugTrapJump = segment_->base() + metadata_->debugTrapFarJumpOffsets[i]; I'd name this one `farJump` ::: js/src/wasm/WasmCode.cpp:923 (Diff revision 20) > +Code::incrementLeaveFrameTrap(JSContext* cx, uint8_t* pc) > +{ > + MOZ_ASSERT(metadata_->debugEnabled); > + const CodeRange* codeRange = lookupRange(pc); > + if (!codeRange) > + return false; In what case could the pc not be in in a CodeRange? Seems like that shouldn't happen. Alternatively, it'd be nice if we could pass CodeRanges around instead of pcs. ::: js/src/wasm/WasmCode.cpp:927 (Diff revision 20) > + if (!codeRange) > + return false; > + MOZ_ASSERT(codeRange->isFunction()); > + > + uint32_t trapAddress = 0; > + auto callSitesRange = lookupCallSiteRangeFor(codeRange); How about naming this `range` so that the following line fits within the 100-char limit. ::: js/src/wasm/WasmCode.cpp:930 (Diff revision 20) > + > + uint32_t trapAddress = 0; > + auto callSitesRange = lookupCallSiteRangeFor(codeRange); > + for (const CallSite* callSite = callSitesRange.first(); callSite != callSitesRange.second(); callSite++) { > + if (callSite->kind() == CallSite::LeaveFrame) > + trapAddress = callSite->returnAddressOffset(); Can you either MOZ_ASSERT(!trapAddress) or break? ::: js/src/wasm/WasmCode.cpp:937 (Diff revision 20) > + MOZ_ASSERT(trapAddress); > + > + if (!leaveFrameCounters_) { > + leaveFrameCounters_.reset(cx->runtime()->new_<LeaveFrameAddressCounters>(cx)); > + if (!leaveFrameCounters_->init()) > + return false; If init() fails, you need to ReportOutOfMemory(cx). ::: js/src/wasm/WasmCode.cpp:945 (Diff revision 20) > + bool alreadyEnabled = !!p; > + if (!p) { > + if (!leaveFrameCounters_->add(p, trapAddress, 1)) > + return false; > + } else > + p->value()++; nit: when then-branch has curlies, else branch needs too. ::: js/src/wasm/WasmCode.cpp:960 (Diff revision 20) > + > + return true; > +} > + > +bool > +Code::decrementLeaveFrameTrap(JSContext* cx, uint8_t* pc) Could you factor out an adjustLeaveFrameTrap() analogous to adjustEnterFrameTrap()? It seems like most of the code is shared. ::: js/src/wasm/WasmGenerator.h:200 (Diff revision 20) > jit::MacroAssembler masm_; > Uint32Vector funcToCodeRange_; > Uint32Set exportedFuncs_; > uint32_t lastPatchedCallsite_; > uint32_t startOfUnpatchedCallsites_; > + DebugTrapFarJumpArray debugTrapFarJumps_; nit: could this also be a Uint32Vector? ::: js/src/wasm/WasmGenerator.cpp:375 (Diff revision 20) > } > + case CallSiteDesc::EnterFrame: > + case CallSiteDesc::LeaveFrame: { > + DebugTrapFarJumpVector& jumps = metadata_->debugTrapFarJumpOffsets; > + if (jumps.empty() || > + uint32_t(abs(int32_t(jumps.back()) - int32_t(callerOffset))) >= JumpRange()) { nit: multi-line condition requires { on new line ::: js/src/wasm/WasmGenerator.cpp:605 (Diff revision 20) > throwStub.offsetBy(offsetInWhole); > if (!metadata_->codeRanges.emplaceBack(CodeRange::Inline, throwStub)) > return false; > > + debugTrapStub.offsetBy(offsetInWhole); > + if (!metadata_->codeRanges.emplaceBack(CodeRange::Inline, debugTrapStub)) Could you add a CodeRange::DebugTrap? This will involve updating wasm/profiling.js and complements ExitReason::DebugTrap. ::: js/src/wasm/WasmInstance.cpp:848 (Diff revision 20) > +Instance::ensureEnterFrameTrapsState(JSContext* cx, bool enabled) > +{ > + if (enterTrapsEnabled_ == enabled) > + return true; > + > + enterTrapsEnabled_ = enabled; It seems like wasm::Code's enterFrameTrapsCounter_ subsumes wasm::Instance's enterTrapsEnabled_. To be symmetric with the leave-frame traps, could you remove enterTrapsEnabled_ and this method and have the caller call wasm::Code directly? ::: js/src/wasm/WasmInstance.cpp:853 (Diff revision 20) > + enterTrapsEnabled_ = enabled; > + > + if (enabled) > + return code_->incrementEnterFrameTraps(cx); > + else > + return code_->decrementEnterFrameTraps(cx); nit: no else after return in then-branch ::: js/src/wasm/WasmStubs.cpp:1170 (Diff revision 20) > + masm.haltingAlign(CodeAlignment); > + > + masm.setFramePushed(0); > + > + ProfilingOffsets offsets; > + GenerateExitPrologue(masm, 0, ExitReason::None, &offsets); Could you add an ExitReason::DebugTrap and use it here (and in the Epilogue). This will allow self-time in the debug traps to be accounted for (which will help people diagnose why their code is running slow if they happen to be accidentally activating debug-mode). ::: js/src/wasm/WasmStubs.cpp:1186 (Diff revision 20) > + masm.moveStackPtrTo(scratch); > + masm.moveStackPtrTo(scratch2); > + masm.subPtr(Imm32(sizeof(intptr_t)), scratch2); > + masm.andPtr(Imm32(~(ABIStackAlignment - 1)), scratch2); > + masm.moveToStackPtr(scratch2); > + masm.storePtr(scratch, Address(scratch2, 0)); I think you only need one scratch reg; you can use masm.andToStackPtr/subFromStackPtr to operate on the stack poitner directly.