499672 - Verifier produces garbage and uses GCAlloc when FixedAlloc would be fine

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

Several memory management improvements are desired:

* used fixed allocation instead of GCAlloc for blockStates, FrameState
* only the jit code path is protected by try/catch, all uses of the verifier should be.
* verifier produces two un-freed FrameState's per invocation of verify() and one of them is entirely unused.

Coder & LIR need cleanup as well, will use separate bugs.

Comment 1

[Edwin Smith]

Attachment: Convert verifier blockStates & FrameState & Value to fixed mem

In this patch:

* FrameState, blockStates, and Value allocated with FixedMem
* Value[] array is a separate allocation from FrameState
* TRY/CATCH around verifier handles interp-only invocation too
* removed redundant initialState instance of FrameState
* explicitly free verifier->state in addition to blockStates

The only GC mem used by the verifier now is objects that get attached
into the type system and live longer than the verifier:
 - ExceptionHandlerTable
 - ScopeTypeChain
 - things allocated by resolveTraits & resolveSignatures

Comment 2

[Tom Harwood]

Comment on attachment 384392 [details] [diff] [review]
Convert verifier blockStates & FrameState & Value to fixed mem

Overall comment: new memory handling regime has lost some OOM checks.
Are they done elsewhere?

- scopeValue(i) could use an assertion to verify input as was done for value(i).
  scope overflow not very likely due to other checks?

- The "// pass 2 - data flow" comment @ MethodInfo.cpp line 291 is obsolete?

- Verifier::cleanup() could delete blockStates.at(i) -- delete NULL port issues?

- Verifier::verify() passes self to FrameState ctor, but still has logic copying
  frame info into the new FrameState.  Move into ctor?
  Unless NULL isn't binary zero on a platform, this loop looks redundant
  b/c the ctor does initialize the new FrameState's locals to 0.

Comment 3

[Lars T Hansen]

Note, out of memory errors are no longer possible unless specific APIs (FixedMalloc::PleaseAlloc) are used.  NULL checks are entirely redundant.

Comment 4

[Edwin Smith]

http://hg.mozilla.org/tamarin-redux/rev/bd9cc869b195