544878 - WordcodeEmitter label placement is slightly too restrictive

Status: RESOLVED FIXED

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

Description

[Edwin Smith]

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.

Comment 1

[Edwin Smith]

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

Comment 2

[Edwin Smith]

Retargeted to 10.2 since 10.1 wont use WC interpreter.

Comment 3

[Edwin Smith]

Attachment: (v2, rebased) moves emitLabel() to writeBlockStart, and removes extra verify error

Comment 4

[Edwin Smith]

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

Comment 5

[Edwin Smith]

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.

Comment 6

[Lars T Hansen]

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

More or less taking your word for it.

Comment 7

[Edwin Smith]

pushed
http://hg.mozilla.org/tamarin-redux/rev/fbae6347ab73 (exception_fixes)
http://hg.mozilla.org/tamarin-redux/rev/b8089741e1cd (emitLabel)