532906 - VTable::resolveImtSlot calls itself recursively, only bounded by inheritance depth

Status: VERIFIED FIXED

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

Description

[Edwin Smith]

Code in VTable::resolveImtSlot() calls base->resolveImtSlot() to get the IMT entry from the base class to copy down.  This is unchecked recursion that is only bounded by inheritance depth.

Comment 1

[Erik Tierney]

Attachment: Eliminate recursion in VTable::resolveImtSlot

When we need to copy the imt thunk from the base VTable, walk up the inheritance chain, pushing each base onto a stack.  Then, walk through the stack backwards, copying the imt thunk down.

Comment 2

[Edwin Smith]

see also: the code in Traits::resolveSignatures() builds a list of base classes and interfaces in top-down order.

Comment 3

[Steven Johnson]

Comment on attachment 416445 [details] [diff] [review]
Eliminate recursion in VTable::resolveImtSlot

Looks right, but we should probably run thru some Flex tests to torture-test it before landing... the avmshell acceptance tests don't exercise this code as much as it should.

Comment 4

[Erik Tierney]

I will do that - do we have a list of flex apps for testing somewhere?

Comment 5

[Erik Tierney]

(In reply to comment #2)
> see also: the code in Traits::resolveSignatures() builds a list of base classes
> and interfaces in top-down order.

I saw that.  This case is a simpler one, as we only have to walk up the base classes, so we don't need to insert the interfaces into the stack.

Comment 6

[Erik Tierney]

I ran Phosphate, DataGridScroll, Insurance App, and TextInputForm_Load for flex tests - everything worked ok, and there was no performance impact from these changes.

Comment 7

[Edwin Smith]

nothing is obviously wrong.  We probably could reduce the allocation overhead of top-down traversal both here, and in Traits::resolveSignatures but its not a clear problem yet.

small nit to clean up before submitting: in VTable.cpp:227-232, this code:

		ImtThunkEnv* ite;
		// copy down imt stub from base class
		AvmAssert(base->imt[slot] != NULL);
		ite = base->imt[slot];

could be simplified to:

		// copy down imt stub from base class
		ImtThunkEnv* ite = base->imt[slot];
		AvmAssert(ite != NULL);

Comment 8

[Erik Tierney]

Cleaned up the part ed was talking about.

pushed:  
http://hg.mozilla.org/tamarin-redux/rev/294937a422f8

Comment 9

[Chris Peyer]

Engineering work item.  Marking as verified.