Closed
Bug 484039
Opened 16 years ago
Closed 15 years ago
CallStackNode::scopeDepth() was recently removed, but is used in Flash Debugger code
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P2)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
VERIFIED
FIXED
flash10.1
People
(Reporter: stejohns, Assigned: stejohns)
References
Details
Attachments
(1 file)
6.14 KB,
patch
|
rreitmai
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
Rick thinks that info()->max_scopes() may be equivalent in this case, but we need to check to be sure. (Note that this is used only inside PlayerAvmDebugger::AppendScopeChain, which sorely needs to be moved into the VM itself as it makes many assumptions that we don't want exposed to embedders.)
Comment 1•16 years ago
|
||
what's the easiest way to make DebugCLI do the same thing so we dont have depend on Flash debugger for testing? (what missing DebugCLI command should we implement)
Assignee | ||
Comment 2•16 years ago
|
||
(In reply to comment #1)
> what's the easiest way to make DebugCLI do the same thing so we dont have
> depend on Flash debugger for testing? (what missing DebugCLI command should we
> implement)
(1) move implementation of PlayerAvmDebugger::AppendScopeChain into the VM, where it should live anyway
(2) have DebugCLI use it (eg to show the frame information)
Comment 3•16 years ago
|
||
I'm gathering requirements from MikeM about why/how this code is used and will come back with a recommendation. In the end we'll need to expose whatever code gets added to the vm via debugCLI.
Comment 4•16 years ago
|
||
We can hold off on this change for now, but it is required prior to shipping FP10.
Updated•16 years ago
|
Assignee | ||
Comment 6•15 years ago
|
||
So this solves the problem by essentially moving the guts of PlayerAvmDebugger::AppendScopeChain into avmplus itself (with a callback).
One twist here is that I tweaked the interpreter to actually null out inactive scopes in the active scope chain, so that the code can handle the jit and interpreter case more uniformly, and so that we don't have to track scopeDepth in the CallStackNode like we used to.
Also removed the now-finally-no-longer-needed isJitImpl() call.
Also rearranged on field in CallStackNode to reduce padding on 64-bit.
(Note: I need to do some torture-testing of this in various AS3 debuggers, so I can't land it just yet, but conceptually I think it's pretty close)
Attachment #408151 -
Flags: superreview?(edwsmith)
Attachment #408151 -
Flags: review?(rreitmai)
Comment 7•15 years ago
|
||
Comment on attachment 408151 [details] [diff] [review]
Patch
nit, but nothing to prevent landing :
scope += kObjectType <= addition here might be efficient but its not clear what is intended.
Also, I wonder whether the debugger wants to be informed of 'holes' in the list of scopes. The check isNullOrUndefined() will result in those scopes not showing up in a listing.
I'd be more inclined to perform the callback for all scopes and let the enumerator implementer decide to filter or not.
Attachment #408151 -
Flags: review?(rreitmai) → review+
Assignee | ||
Comment 8•15 years ago
|
||
(In reply to comment #7)
> (From update of attachment 408151 [details] [diff] [review])
> nit
both fair complaints, will address before landing
Updated•15 years ago
|
Attachment #408151 -
Flags: superreview?(edwsmith) → superreview+
Assignee | ||
Comment 9•15 years ago
|
||
pushed as changeset: 2880:90caf1c61bc6
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•