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

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Edwin Smith]

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.

Comment 1

[Edwin Smith]

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

Comment 2

[Edwin Smith]

Attachment: 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.

Comment 3

[William Maddox]

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) {

Comment 4

[Edwin Smith]

Attachment: (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

Comment 5

[Edwin Smith]

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

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

Comment 6

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/e567cbf274a4