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)

defect

Tracking

(Not tracked)

VERIFIED FIXED
flash10.1

People

(Reporter: stejohns, Assigned: stejohns)

References

Details

Attachments

(1 file)

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.)
Depends on: 481230
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)
(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)
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.
We can hold off on this change for now, but it is required prior to shipping FP10.
Target Milestone: --- → flash10.x
Depends on: 486410
No longer depends on: 481230
Flags: flashplayer-qrb+
Priority: -- → P2
Poaching from Rick
Assignee: rreitmai → stejohns
Attached patch PatchSplinter Review
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 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+
(In reply to comment #7) > (From update of attachment 408151 [details] [diff] [review]) > nit both fair complaints, will address before landing
Attachment #408151 - Flags: superreview?(edwsmith) → superreview+
pushed as changeset: 2880:90caf1c61bc6
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Engineering work item. Marking as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: