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

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Baseline JIT (CodegenLIR)
P3
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Edwin Smith, Assigned: Edwin Smith)

Tracking

(Blocks: 1 bug)

unspecified
Q3 11 - Serrano
Dependency tree / graph
Bug Flags:
flashplayer-qrb +
flashplayer-bug +

Details

(Whiteboard: has-patch)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 438130 [details] [diff] [review]
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.
Assignee: nobody → edwsmith
Status: NEW → ASSIGNED
Attachment #438130 - Flags: feedback?(rreitmai)
(Assignee)

Updated

8 years ago
Attachment #438130 - Flags: feedback?(mmoreart)
(Assignee)

Comment 2

8 years ago
Created attachment 438150 [details] [diff] [review]
(v2) use tags instead of varTraits

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)

Updated

8 years ago
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2

Comment 3

8 years ago
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+

Comment 4

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #438150 - Flags: feedback?(rreitmai)
(Assignee)

Comment 6

8 years ago
> 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

Comment 7

8 years ago
- 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.
(Assignee)

Updated

8 years ago
Blocks: 484909
(Assignee)

Comment 8

8 years ago
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?

Comment 9

8 years ago
FYI, this patch doesn't quite work in Flash -- compiles but ATS hangs.
(Assignee)

Updated

8 years ago
No longer blocks: 484909
Depends on: 484909
(Assignee)

Updated

8 years ago
Whiteboard: has-patch
(Assignee)

Comment 10

8 years ago
Created attachment 438735 [details] [diff] [review]
(v3) use tags instead of varTraits

Rebased, factored out deadvars() changes that already were submitted, fixed indentation.
(Assignee)

Updated

8 years ago
Attachment #438150 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Created attachment 439046 [details] [diff] [review]
(v4) use tags instead of varTraits

this one is rebased, and uses const uint8_t* everywhere.
Attachment #438735 - Attachment is obsolete: true
Attachment #439046 - Flags: review?(mmoreart)

Comment 12

8 years ago
(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.

Updated

8 years ago
Attachment #439046 - Flags: review?(mmoreart) → review+
(Assignee)

Updated

8 years ago
Blocks: 559420
(Assignee)

Comment 13

8 years ago
pushed to TR
http://hg.mozilla.org/tamarin-redux/rev/2d3c06669582
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.