481230 - LIR doesn't properly update the traits of locals (breaks debugger)

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

LIR attempts to track the traits of locals (when debugger() is non-null) so that a debugger can properly box the locals for presentation to the user. But, it's broken, because Verifier doesn't update the framestate until after the relevant local stores are emitted. (This used to work in MIR because MIR didn't update the local-traits until the end of a basic block, by which time the framestate had been updated.)

To fix, we either need to refactor Verifier to update the framestate traits sooner, or modify LIR to update the local-traits the same way MIR did.

Comment 1

[Rick Reitmaier]

assigned to rickr.  Most likely have dependencies on Verifier work see 481275 and 478115

Comment 2

[Rick Reitmaier]

Attachment: ver 1 - gets DebugCLI working

Comment 3

[Rick Reitmaier]

Attachment: ver 1 - trims DebugMethodInfo structure to only needed fields

Comment 4

[Rick Reitmaier]

Attachment: ver 1 - call stack node cleanup

Comment 5

[Rick Reitmaier]

Attachment: ver 2 - get DebugCLI working again

Comment 6

[Rick Reitmaier]

Attachment: ver 1 - debugger functional once again

Comment 7

[Steven Johnson]

Comment on attachment 366423 [details] [diff] [review]
ver 1 - call stack node cleanup

review- only for two concerns, otherwise I like it:

(1) I think the call to debugEnter in CodegenLIR.cpp needs to be updated (since you changed the args) and I don't see that here. Did I miss it?

(2) We need to double-check to ensure that the pruned fields aren't in use by the Flash/AIR debuggers -- if not then let's prune away

Comment 8

[Rick Reitmaier]

re: comment #7

(1) you're correct and that change landed in patch #4.  I was separating the patches manually and missed it.  Have a look there, please.

(2) they are not used by flash/air and if they are we need to fix it!  The fields were added in the early days when we were playing with the jit and debugging and just never got trimmed.

Comment 9

[Steven Johnson]

Comment on attachment 366426 [details] [diff] [review]
ver 1 - debugger functional once again

okey dokey then.

Comment 10

[Edwin Smith]

Comment on attachment 366424 [details] [diff] [review]
ver 2 - get DebugCLI working again

Generally I like the direction and i'm eager to have a working standalone debugger.

issues with the patch:
* I can't get a working build / debugger given the info in the bug.  what starting rev in redux should be used?  (hg diff provides this, yet it's not here in these patches).

* i tried applying each patch in succession, the fourth didn't apply cleanly, and i can't find the base rev that lets the first patch even compile cleanly.

* applying the DebugCLI patch by itself doesn't result in a working debugger (avmplus -d foo.abc) doesn't enter the debugger unless the abc is compiled with debug symbols.  (need an error!)

also in reviewing the code, the diagnostics for a missing source file were commented out, but no replacement was found.  a useful error message could save substantial hair pulling.

Comment 11

[Edwin Smith]

Comment on attachment 366426 [details] [diff] [review]
ver 1 - debugger functional once again

(generally) I can't get this to apply cleanly: recommend using hg diff to create patches (not git style!)

(specifically)

CodegenLIR:2559, localSet() traits parameter is commented out.  the actual type could be null, void, String, or Namespace, correct?

Since localSet() requires a valid traits parameter for debugging to work, its best to make it a non-optional parameter.

line 2870: ClassClosure::newInstance() returns ScriptObject*, so the traits for the result pointer should be something specific.  OBJECT_TYPE is incorrect because it implies the value is an Atom, which it isn't.

Comment 12

[Rick Reitmaier]

I'll rebase the patches and supply the needed info.

Comment #10

Regarding the debugcli changes, seeing as it wasn't functioning at all and post-patch at least works, I'd consider that an improvement.  Is it all that needs to be done, no; but its a start.  

With regards to source-file-missing messages; the logic for locating the source file is weak and was causing issues for me; for instance repeated messages per debugger command.  I agree it needs to be cleaned up.

Comment #11

* CodegenLIR:2559 : good catch.  yes that needs fixing
* Agreed will make traits ptr non-optional.
* I'll mark it as CLASS_TYPE.  That way it will go through the boxing logic correctly; see boxLocals.  BTW, that section of code should also get fixed but that's for another day/bug.

Comment 13

[Rick Reitmaier]

Attachment: ver 2 - call stack node cleanup

the debugger patch must be laid down first.

Comment 14

[Rick Reitmaier]

Attachment: ver 2 - debugger functional once again

Comment 15

[Rick Reitmaier]

Attachment: ver 2 - gets DebugCLI working

Comment 16

[Edwin Smith]

Comment on attachment 367128 [details] [diff] [review]
ver 2 - debugger functional once again

* OP_pushundefined should be emitPtrConst(..., VOID_TYPE)

* in emitCall(), case OP_construct, when we call newInstance(), we know the result type, it's passed in as "result".  ie s/CLASS_TYPE/result, i think.

* in epilogue(), localSet(..., exAtom, INT_TYPE) should be NULL (the exception atom is an atom == type * == NULL).

Comment 17

[Rick Reitmaier]

3 pts above addressed and change pushed 73f6fe8a9258: bug 481230 - AS debugging of variables - rev 1594

Comment 18

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.