Verifier's use of offset-from-code_start is too fragile

RESOLVED FIXED in Q3 11 - Serrano

Status

P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: edwsmith, Assigned: edwsmith)

Tracking

(Blocks: 1 bug)

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

Details

(Whiteboard: has-patch)

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
Tracking exception ranges and block labels using (int) offsets from code_start is fragile in methods that have synthetic init code followed by OP_abs_jump.  In the past, it has required careful coding to work around the fragileness (witness tryFrom/tryTo ranges and careful timing of parseExceptionHandlers(), for example).

The problem now blocks OSR, for which the JIT which must look up saved FrameState information when called from CodeWriter::writeProloge().  However at that point, code_start points to init code; int offsets offsets to loop labels and exception ranges are not valid until writeOp2(OP_abs_jump) is called.

general fix: Use byte* pc's instead since they are exact.
(Assignee)

Updated

8 years ago
Blocks: 539094
Target Milestone: --- → Future
(Assignee)

Updated

8 years ago
Blocks: 565184
(Assignee)

Updated

8 years ago
Assignee: nobody → edwsmith
Priority: -- → P2
Target Milestone: Future → flash10.2
(Assignee)

Comment 1

8 years ago
assigned, targeted, and raised priority since it also blocks a valuable bugfix and cleanup for exception handling (bug 565184)
(Assignee)

Comment 2

8 years ago
Created attachment 446294 [details] [diff] [review]
Use byte* pc's everywhere except in debug output and exception handler table

In the exception handler table we still need to store int offsets, so we explicitly capture the byte* pointer to use as a base.  (tryBase).  Everywhere else we now use byte* pc.

To do this, FrameState->pc (int) was replaced with abc_pc (byte*), CodegenLIR's blockStates table was modified to use byte* as a key, and the changes in api were propagated around.

it is tempting to go all the way, and change the ExceptionHandler table as well, but I haven't done that.
Attachment #446294 - Flags: review?(wmaddox)

Comment 3

8 years ago
Comment on attachment 446294 [details] [diff] [review]
Use byte* pc's everywhere except in debug output and exception handler table

R- due to the following issue: Build fails with #define FEATURE_CFGWRITER due to multiple references to undeclared member FrameState::pc in class CFGWriter.

nits:
'bool hasFrameState(int pc_off)' is no longer used.  I would recommend simply deleting it.  If there is any need for its functionality, using a different name would be appropriate, as overloading in this case is likely to create confusion.

    // emit a relative branch to the given ABC pc-offset by mapping pc_off
    // to a corresponding CodegenLabel, and creating a new one if necessary

In the following, the comment should refer to the renamed parameter 'pc, not 'pc_off':
    
    // emit a relative branch to the given ABC pc-offset by mapping pc_off
    // to a corresponding CodegenLabel, and creating a new one if necessary
    void CodegenLIR::branchToAbcPos(LOpcode op, LIns *cond, const byte* pc) {
Attachment #446294 - Flags: review?(wmaddox) → review-
(Assignee)

Updated

8 years ago
Whiteboard: has-patch
(Assignee)

Comment 4

8 years ago
Created attachment 447349 [details] [diff] [review]
(v2) Use byte* pc's everywhere except in debug output and exception handler table

Changes since previous patch:
* rebased
* removed CFGWriter
* removed Verifier::hasFrameState(int pc_off)
* updated comment
Attachment #447349 - Flags: review?(wmaddox)
(Assignee)

Comment 5

8 years ago
(In reply to comment #4)
> * removed CFGWriter

Clarification: I removed CFGWriter in tamarin-redux, then rebased.  So the CFGWriter issue no longer applies.

Updated

8 years ago
Attachment #447349 - Flags: review?(wmaddox) → review+
(Assignee)

Comment 6

8 years ago
TR: http://hg.mozilla.org/tamarin-redux/rev/e567cbf274a4
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

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