Closed Bug 558406 Opened 14 years ago Closed 14 years ago

Debugger could use JIT SST tags instead of explicit Traits* pointers

Categories

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q3 11 - Serrano

People

(Reporter: edwsmith, Assigned: edwsmith)

References

Details

(Whiteboard: has-patch)

Attachments

(1 file, 3 obsolete files)

This would streamline debuggable jit code, because SST tag values are small integer constants, and traits pointers are, well, pointers.  Since we generate LIR for the SST tags anyway, we have less code.

To do this correctly, deadvars() must be aware of the points where the tag values can be read, so the stores are not removed.  Doing this for tags[] and vars[] at the same time will also fix Bug 484909.
Attached patch use tags instead of varTraits (obsolete) — Splinter Review
This bug will only work on nanojit backends that support byte-sized load/store instructions.

options:
1. add byte load/store to the remaining backends (ppc, sparc, maybe mips)
2. add a tag typedef for the debugger api's instead of hardcoding uint8_t.
Assignee: nobody → edwsmith
Status: NEW → ASSIGNED
Attachment #438130 - Flags: feedback?(rreitmai)
Attachment #438130 - Flags: feedback?(mmoreart)
excluded .cproject noise
Attachment #438130 - Attachment is obsolete: true
Attachment #438150 - Flags: feedback?(rreitmai)
Attachment #438150 - Flags: feedback?(mmoreart)
Attachment #438130 - Flags: feedback?(rreitmai)
Attachment #438130 - Flags: feedback?(mmoreart)
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Comment on attachment 438150 [details] [diff] [review]
(v2) use tags instead of varTraits

Looks good to me, I don't see any bugs.  Trivial indentation problems in StackTrace.cpp: Three of the lines that assign a value to m_frame_sst have an incorrect number of tabs after "m_frame_sst" and before "=".
Attachment #438150 - Flags: feedback?(mmoreart) → feedback+
(In reply to comment #1)
> Created an attachment (id=438130) [details]
> use tags instead of varTraits
> 
> This bug will only work on nanojit backends that support byte-sized load/store
> instructions.
> 
> options:
> 1. add byte load/store to the remaining backends (ppc, sparc, maybe mips)
> 2. add a tag typedef for the debugger api's instead of hardcoding uint8_t.

Haven't looked at the patch yet, but is it feasible to use 32b instead...which is probably what (2) is referring to.
(In reply to comment #4)
> > options:
> > 1. add byte load/store to the remaining backends (ppc, sparc, maybe mips)
> > 2. add a tag typedef for the debugger api's instead of hardcoding uint8_t.
> 
> Haven't looked at the patch yet, but is it feasible to use 32b instead...which
> is probably what (2) is referring to.

#2 means defining a typedef something like this:

#if NJ_EXPANDED_LOADSTORE_SUPPORTED == 1
typedef uint8_t JitSstTag;
#else
typedef uint32_t JitSstTag;
#endif

and then using JitSstTag instead of uint8_t.  Or, factoring the code more so that knowlege of the tag layout is encapsulated in the jit code, somehow (nicer, but needs a bit more thought).

certianly is feasible, just a bit annoying to work around the lack of a couple of fairly easy opcodes to implement.  On the other hand its more annoying to hold up a debugger cleanup patch while the opcodes are implemented.
Attachment #438150 - Flags: feedback?(rreitmai)
> On the other hand its more annoying to
> hold up a debugger cleanup patch while the opcodes are implemented.

It turns out the only missing support is LIR_stb on PPC and Sparc.  All other backends implement LIR_stb, and all backends already implement LIR_ldzb.
Depends on: 558600
- nice cleanup and good way to resolve debugger deadvars issue too! 

- Also Mike probably already watched for this , but if the player code relies on the Traits* being available then we'll have to have a parallel patch land for the player debugger.
Blocks: 484909
From what I could tell tracing the code, the debugger only used the Traits* pointers to determine the representation of the value.  As long as we know the value's representation, we can always get the value's Traits* after the fact, although we cannot reconstruct the compile-time Traits*.

If the debugger has a feature that checks the compile-time Traits* of a variable to ensure we don't assign illegal values to a variable while stopped at a breakpoint, then this patch could be a problem.

and, if the debugger *doesn't* have such a feature, then it probably should, and it will need to know the compile-time constraints on each variable slot, at each point in a function.   This includes its staticly determined type (which is what varTraits was), as well as its nullability flag.  See Bug 544238.

Mike, thoughts?
FYI, this patch doesn't quite work in Flash -- compiles but ATS hangs.
No longer blocks: 484909
Depends on: 484909
Whiteboard: has-patch
Rebased, factored out deadvars() changes that already were submitted, fixed indentation.
Attachment #438150 - Attachment is obsolete: true
this one is rebased, and uses const uint8_t* everywhere.
Attachment #438735 - Attachment is obsolete: true
Attachment #439046 - Flags: review?(mmoreart)
(In reply to comment #8)
> From what I could tell tracing the code, the debugger only used the Traits*
> pointers to determine the representation of the value.  As long as we know the
> value's representation, we can always get the value's Traits* after the fact,
> although we cannot reconstruct the compile-time Traits*.
> 
> If the debugger has a feature that checks the compile-time Traits* of a
> variable to ensure we don't assign illegal values to a variable while stopped
> at a breakpoint, then this patch could be a problem.
> 
> and, if the debugger *doesn't* have such a feature, then it probably should,
> and it will need to know the compile-time constraints on each variable slot, at
> each point in a function.   This includes its staticly determined type (which
> is what varTraits was), as well as its nullability flag.  See Bug 544238.
> 
> Mike, thoughts?

Edwin and I discussed this on the phone.  The debugger currently has no such feature, and it probably should.  But this change doesn't negatively impact that future possibility, because, although moving from Traits pointers to SST tags does cause some loss of information, it turns out that even Traits aren't sufficient for the debugger to do validation of values being assigned to variables, so we aren't losing anything by giving up the Traits pointers.
Attachment #439046 - Flags: review?(mmoreart) → review+
Blocks: 559420
pushed to TR
http://hg.mozilla.org/tamarin-redux/rev/2d3c06669582
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: