539454 - In methods with try/catch state->insideTryBlock is never set back to false

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

Description

[Edwin Smith]

Which causes excessive JIT code to be generated which tracks the current PC in case of exceptions.

Comment 1

[Edwin Smith]

It's a bug to set the current pc too often, but it also a bug to *only* set it when we are in a try block.  if that happens then the rethrow in a finally block will reinvoke the catch block, since the last update to _save_eip was still inside the try block.

the test as3/Statements/Exceptions/NestedTryWithMultipleCatchInsideFinallyExceptionBubbling.as

will demonstrate this by looping forever.

Comment 2

[Edwin Smith]

Attachment: Fixed by removing insideTryBlock entirely and setting pc cleanly at each CodeWriter entry point

The proper fix here is to always set PC in methods with try blocks, whether or not we're inside the try block.  This patch does so by:

1. moving all "CodegenLIR::state = state" assignments to the CodeWriter entry points, and removing all other assignments.

2. renaming emitPrep() to emitSetPc() since that's really what it does.

3. calling emitSetPc() at each CodeWriter entry point, and remove all other emitPrep calls.   Some were redundant, and it was not easy to tell if one was missing.

Comment 3

[Rick Reitmaier]

I'd really like to see this patch split into 2 pieces, with (1) and (2) together, followed by a 2nd patch with only (3).

That aside, I'll have to revisit it tomorrow since I've effectively +'d 1 and 2, but need to squint a little harder to validate (3).

Comment 4

[Rick Reitmaier]

Comment on attachment 428265 [details] [diff] [review]
Fixed by removing insideTryBlock entirely and setting pc cleanly at each CodeWriter entry point

Could we end up in a situation where 'writeOpcodeVerified' is called, clearing the state, then in the info->exceptions section of the Verifier we call emitCoerce() i.e. see the comment 'FIXME: bug 538639' ? 


There doesn't seem to be any intervening coder-> calls, except the writeBlockStart() which does not set state.

Also not in this patch but 'fixExceptionsAndLabels' probably should be 'writeFixExceptionsAndLabels'

+'d otherwise.

Comment 5

[Edwin Smith]

All of the other CodeWriter entry points re-set this->state to a valid pointer.  Calling writeOpcodeVerified() at the top of the catch block is safe; each instruction in the catch block will call write() or writeOp1() (etc).

setting this->state to NULL is intended to cause a NPE crash if we accidentally refactor away the assignment in any of the other entry points.

The fixExceptionAndLabels rename already was pushed in one of the patches on bug 546408.

The other way to go about dealing with stale FrameState* in CodegenLIR is to get rid of the field entirely and pass it around as a paramter.  Its a much bigger patch but might be preferred in the long run.

opinions?

Comment 6

[Edwin Smith]

pushed http://hg.mozilla.org/tamarin-redux/rev/33bcfe704926

Comment 7

[Brent Baker]

Verified that the as3/Statements/Exceptions/NestedTryWithMultipleCatchInsideFinallyExceptionBubbling.as executes properly. This was also covered in bug 521879, removing skip from testconfig