WordcodeEmitter label placement is slightly too restrictive

RESOLVED FIXED in Q3 11 - Serrano

Status

P3
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: edwsmith, Assigned: edwsmith)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-qrb +
flashplayer-bug +

Details

(Whiteboard: verifier-cleanup)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Currently we create a label record for each OP_label, however the verifier's notion of a label is based on the existence of a FrameState* at a particular location, which can be due to a forward jump, a label, or a catch block.

Bug 481171 was due to the WordcodeEmitter only placing an implicit label when it sees OP_label, and crashing in a situation that would later trigger a VerifyError anyway.  The fix to 481171 was thus a pre-emptive VerifyError.

An alternate fix is proposed here:  add a label_info record in writeBlockStart(), which matches the verifier's notions of labels.  This fixes 481171, but also is consistent with how the JIT interacts with the verifier, and protects WordcodeEmitter from changes in the Verifier's logic for identifying blocks.
(Assignee)

Updated

9 years ago
Blocks: 413522

Updated

9 years ago
Assignee: nobody → edwsmith
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
Priority: -- → P2
Whiteboard: verifier
Target Milestone: --- → flash10.1

Updated

9 years ago
Whiteboard: verifier → verifier-cleanup
(Assignee)

Comment 1

9 years ago
Created attachment 427890 [details] [diff] [review]
moves emitLabel() to writeBlockStart, and removes extra verify error

This cannot land yet, because the Verifier calls coder->writeOp1() (for branches), before calling checkTarget(), even though checkTarget is the natural place for the illegal back-edge check.
(Assignee)

Updated

9 years ago
No longer blocks: 413522
Depends on: 413522
(Assignee)

Comment 2

9 years ago
Retargeted to 10.2 since 10.1 wont use WC interpreter.
Priority: P2 → P3
Target Milestone: flash10.1 → flash10.2
(Assignee)

Comment 3

9 years ago
Created attachment 433143 [details] [diff] [review]
(v2, rebased) moves emitLabel() to writeBlockStart, and removes extra verify error
Attachment #427890 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
Created attachment 433462 [details] [diff] [review]
Fixes exeception_fixup logic in WordcodeEmitter

The verifier upgrade injected a bug in WordcodeEmitter, in the case when you have init code followed by OP_abc_jump, in a method that has try blocks.  I.e. in a constructor containing try blocks.  The bug causes "AvmAssert(!exceptions_consumed);" to fire in emitAbsJump().

WordcodeEmitter relied on the fact that MethodInfo::abc_exceptions() returned null, initially, then later returned a valid [non-null] exception table at the point of OP_abs_jump.  This way, the exception_fixes list wasn't initialized until we reached the "real" code with try blocks.

With the verifier upgrade, phase 1 initializes the exception table, and MethodInfo::abc_exceptions returns non-null before OP_abs_jump is reached.  WCEmitter computed exception_fixes too early, causing them to be based on the wrong address.

The fix: Verifier::tryFrom and tryTo indicate the real boundaries of exception handlers.  If they don't fall inside [code_start::code_length], then we defer computeExceptionFixups().  emitAbsJump() will call computeExceptionFixups() again, resulting in valid exception_fixes.
Attachment #433462 - Flags: review?(lhansen)
(Assignee)

Comment 5

9 years ago
Comment on attachment 433143 [details] [diff] [review]
(v2, rebased) moves emitLabel() to writeBlockStart, and removes extra verify error

WordCode emitter caused a VerifyError if a back-edge did not land on an OP_label instruction.

Verifier rules are that back edges must land on a known block boundary, which can be either a target of an earlier forward edge, or an OP_label instruction.

The verifier upgrade subsumed the wordcode emitter's logic, and does the check during phase 1, making the WC emitter check redundant.

Moving the call to WCEmitter::emitLabel() to CodeWriter::writeBlockStart() makes it compatible with the verifier's notion of block boundaries.
Attachment #433143 - Flags: review?(lhansen)

Updated

9 years ago
Attachment #433143 - Flags: review?(lhansen) → review+

Comment 6

9 years ago
Comment on attachment 433462 [details] [diff] [review]
Fixes exeception_fixup logic in WordcodeEmitter

More or less taking your word for it.
Attachment #433462 - Flags: review?(lhansen) → review+
(Assignee)

Comment 7

9 years ago
pushed
http://hg.mozilla.org/tamarin-redux/rev/fbae6347ab73 (exception_fixes)
http://hg.mozilla.org/tamarin-redux/rev/b8089741e1cd (emitLabel)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.