Verifier produces garbage and uses GCAlloc when FixedAlloc would be fine

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: edwsmith, Assigned: edwsmith)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 384392 [details] [diff] [review]
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
Assignee: nobody → edwsmith
Attachment #384392 - Flags: review?(lhansen)
(Assignee)

Updated

10 years ago
Attachment #384392 - Flags: superreview?(lhansen)
Attachment #384392 - Flags: review?(tharwood)
Attachment #384392 - Flags: review?(lhansen)

Comment 2

10 years ago
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.
Attachment #384392 - Flags: review?(tharwood) → review+

Comment 3

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

Updated

10 years ago
Attachment #384392 - Flags: superreview?(lhansen) → superreview+
(Assignee)

Updated

10 years ago
Blocks: 499980
(Assignee)

Comment 4

10 years ago
http://hg.mozilla.org/tamarin-redux/rev/bd9cc869b195
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.