526029 - CallStackNode::enumerateScopeChainAtoms is wrong for jitted methods

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

The dynamic part of the scope chain is always spaced at 8-byte intervals for jitted methods, regardless of 32/64 arch. enumerateScopeChainAtoms needs to account for this.

Comment 1

[Steven Johnson]

Attachment: Patch

Fix.

Comment 2

[Edwin Smith]

Rick; CodegenLIR only maintains Traits info for local_count variables in the stack frame; if the debugger (or any tool) directly accesses the scope chain on the stack frame as well, then we ought to be tracking Traits for scope chain entries too, right?  (all the debugger-specific logic, like saving vars before calls, loading them after calls, etc, is all only done for locals but it seems like for safety we must do all this for scope chain items too, right?)

Comment 3

[Edwin Smith]

Comment on attachment 409804 [details] [diff] [review]
Patch

StackTrace.cpp:158: encapsulate checking of jit flag in class MethodInfo

StackTrace.cpp:167: scope values can be string/namespace/int/uint/double/bool; this code is unsafe.  

per the previous comment, the jit needs to save the traits for any stack frame slots accessed by the debugger.  currently it only does this for locals, but it needs to do it for scopes too, and this code here needs to use the saved traits to correctly decode scope values.

even the old code was unsafe... relying on "kUnusedTag" to detect jit vs interpreter is unsound.  (ints could have arbitrary bits 0:2, for example).

Comment 4

[Steven Johnson]

(In reply to comment #3)
> (From update of attachment 409804 [details] [diff] [review])
> StackTrace.cpp:158: encapsulate checking of jit flag in class MethodInfo

will do, but I'd still like to keep it private with friend access to prevent embedders from being able to peek at it as they used to.
 
> StackTrace.cpp:167: scope values can be string/namespace/int/uint/double/bool;
> this code is unsafe.  

line 167 is an #ifdef. what are you referring to? AFAICT, the interpreter always uses tagged atom values, and the jit uses untagged values only for activation objects (please correct me if I'm wrong). this is also the logic that the MIR-based code used (except for the 8-byte spacing) so please let me know what I'm missing... does nanojit behave differently?
 
> per the previous comment, the jit needs to save the traits for any stack frame
> slots accessed by the debugger.  currently it only does this for locals, 
> but it needs to do it for scopes too, and this code here needs to use 
> the saved traits to correctly decode scope values.

That sounds more robust.

> even the old code was unsafe... relying on "kUnusedTag" to detect jit vs
> interpreter is unsound.  (ints could have arbitrary bits 0:2, for example).

Ah, so we were just getting lucky before, then? Gotcha...

Comment 5

[Rick Reitmaier]

re comment #2 we save the Traits for locals since they get spilled to the stack in their native form and debugger would not be able to interpret the encoding without some adjunct information.

If the values are always Atoms then we don't need the Traits, but for scope I don't believe this is the case; so yes we should store the Traits.

Removing the < local_count in localSet() and allocating enough space in Traits array should be a good start, in other words we shouldn't need to add much logic ot make it work.

Comment 6

[Edwin Smith]

in this case, the values that the debugger is trying to access are *not* Atoms.

Comment 7

[Steven Johnson]

(In reply to comment #6)
> in this case, the values that the debugger is trying to access are *not* Atoms.

Yep. Working on a more complete fix now.

Comment 8

[Steven Johnson]

Attachment: Patch #2

Changes per Ed's suggestions. Note, this patch is not yet tested (!) and obviously will be prior to landing, but conceptually should be pretty close.

Comment 9

[Rick Reitmaier]

Comment on attachment 409994 [details] [diff] [review]
Patch #2

nice cleanup; mind to add/change the prologue comment regarding 'varTraits'.

Comment 10

[Edwin Smith]

Comment on attachment 409994 [details] [diff] [review]
Patch #2

provisional R+

* class DebuggerCheck also needs to be initialized with local_count + scope_depth

Comment 11

[Steven Johnson]

pushed as changeset:   2992:ca3d83ef436b

Comment 12

[Steven Johnson]

Attachment: Patch

Refinement on top of previous patch: consolidate redundant code between boxArgs and boxOneLocal

Comment 13

[Edwin Smith]

MethodInfo.cpp:809:  it would be cleaner not to ifdef:
   ap += (bt == BUILTIN_number ? sizeof(double) : sizeof(Atom)) / sizeof(int32_t)

Comment 14

[Steven Johnson]

pushed with ed's revision as changeset:   3083:e152c1d808bf

Comment 15

[Rick Reitmaier]

Comment on attachment 410684 [details] [diff] [review]
Patch

After the fact r+

Comment 16

[Chris Peyer]

Engineering work item.  Marking as verified.