486410 - AS Debugger fixes

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Rick Reitmaier]

This bug is a holding pot for various debugger issues encountered during integration.

Comment 1

[Rick Reitmaier]

Attachment: ver1 - applies to steven's sandbox

placing here to keep a record of what changes were needed, will formally rearrange for submit once all issues are resolved.

(1) GC::CleanStack needs to be disabled for debugger, due to threading issues.  The debugger executes code on multiple threads and in so doing rememberedStackTop can get set to multiple thread local stack values; boom.

(2) Added DebuggerCheck filter for debug debugger builds to verifier that the LIR being generated for traits is correct. 

(3) bug in call to localSet() needed to use 'loc' not 'i'

(4) In MethodEnv:debugEnter() frameTraits should not be cleared out.

(5) StackTrace.scopeBase() disabled until bug 484039 is fixed

(6) LIR_live now extends the lifetime of LIR_alloc's instructions to the point of the live reference.  Other expressions continue to work as they did before, i.e. their lifetime is extended to the bottom of loops.

Comment 2

[Edwin Smith]

can you generate the patch with hg diff -- all the files in that patch seem mashed together?

Comment 3

[Edwin Smith]

even better, rebase to tamarin-redux too

Comment 4

[Rick Reitmaier]

Attachment: ver 1 - reposting patch rebased against redux

Poated a redux based version of the patch for others to use - Will pull it apart for review shortly.

Comment 5

[Rick Reitmaier]

Attachment: ver 1 - live extension for LIR_alloc

LIR_live now extends the lifetime of LIR_alloc's instructions to the point
of the live reference.  Other expressions continue to work as they did before,
i.e. their lifetime is extended to the bottom of loops.

Comment 6

[Rick Reitmaier]

Attachment: ver 1 - cgen fixes and filter

Fixes a bug in call to localSet() needed to use 'loc' not 'i' and introduces DebuggerCheck filter for debug debugger builds to verifier that the
LIR being generated for traits is correct.

Comment 7

[Rick Reitmaier]

Attachment: ver 1 - clean stack disabled for debugger builds

GC::CleanStack needs to be disabled for debugger, due to threading issues. 
The debugger executes code on multiple threads and in so doing
rememberedStackTop can get set to multiple thread local stack values; boom.

Also note: Steven's comment :

"ok, but keep in mind that AIR always runs with SCRIPT_DEBUGGER — if the code doesn’t check for presence of a debuuger at runtime (vs compiletime), maybe it should"

which implies this is not the correct solution. Can you suggest otherwise?

Comment 8

[Rick Reitmaier]

Attachment: ver 1 - disable scopeBase() call for debugger

StackTrace.scopeBase() disabled until bug 484039 is fixed

Comment 9

[Edwin Smith]

Comment on attachment 370916 [details] [diff] [review]
ver 1 - live extension for LIR_alloc

adding commentage such as "... because LIR_alloca memory is presumed referenced indirectly until the point of LIR_live"

Comment 10

[Edwin Smith]

Comment on attachment 370917 [details] [diff] [review]
ver 1 - cgen fixes and filter

Please add comments explaining what DebuggerCheck is and how it works.

Comment 11

[Tommy Reilly]

Comment on attachment 370919 [details] [diff] [review]
ver 1 - clean stack disabled for debugger builds

This won't work b/c AIR always defines DEBUGGER.

Comment 12

[Rick Reitmaier]

Comment on attachment 370919 [details] [diff] [review]
ver 1 - clean stack disabled for debugger builds

Obsolete see https://bugzilla.mozilla.org/show_bug.cgi?id=487032

Comment 13

[Rick Reitmaier]

99e9cf0046a2: Fixes for bug 486410 - AS debugging (r=stejohns/edwsmith) - rev 1679