443294 - Assert on allocation failure; bigger trampolines needed?

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

The Fragmento::pagesGrow() function "returns" NULL when the delta is out of the 16MB range.  It seems code doesn't really check that, so we got a crash deep in LirBufWriter.

Maybe it should assert instead? (Patch attached.)

This exposes perhaps a worse problem though; with the new exponential growth allocator, it's more likely addresses will be out of the 16MB range before a 16MB limit is reached (I think we hit it at 4MB).  Andreas thinks a wider trampoline opcode may be necessary.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: adds assertion when pagesGrow() fails

Comment 2

[Edwin Smith]

a wider tramp would certianly work, although this is probably why the code region was preallocated to 16MB, just to get pages to be contiguous.

any other clever ideas?  oprnd1() is already annoyingly expensive, a second kind of tramp would add to the trouble.  

we could make LIR_tramp and LIR_skip take a full pointer, always -- keeps oprnd1() simple at the cost of a small increase in LIR size.

another idea would be to bite the bullet and make all LIR instructions 8 bytes wide.  this would:
* buy room for many more high level opcodes
* avoid tramps entirely, usually
* cost about 25% increase in size of LIR memory required (assuming average bytes/LIR for a long trace is ~6).
* eliminate need for LIR_short
* lift 256 entry restriction for Assembler Reservations (altho that's not the only way to get that restriction lifted)

Comment 3

[Edwin Smith]

Comment on attachment 327889 [details] [diff] [review]
adds assertion when pagesGrow() fails 

we actually need to fail gracefully here even if we cant reach the full range of contiguous memory.

asserts are reserved for real bugs, they should never fire even for scarce or fragmented memory

Comment 4

[Andreas Gal :gal]

With 64-bit LIR ops you could definitively cleanup the LIR_call/LIR_arg representation. That might actually save some space. Traversal might get faster, too. Calls could be represented inline as special LIR ops. Otoh, this sounds like an invasive change touching a ton of code.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

I like the idea of widening the instruction.  Btw I added the assert because it seemed pretty pervasive that NULL pointers weren't checked, so that sounds like an invasive change as well.

Comment 6

[Edwin Smith]

Attachment: Eliminate 16MB restriction, allow long tramp/skips, eliminate LIR_arg family, reduce LIR_tramps

This patch lifts the short-range restrictions to LIR_tramp and LIR_skip, and adds a few optimizations to further reduce LIR size:

- LIR_arg, farg, ref are removed, the CallInfo struct used by LIR_call has enough information to replace these args.
- the most common LIR_tramp's are from rp and sp references.  Added smarts to LirBuffer to reuse those tramps.

- removed insImm8() pipeline function, added insParam() in its place.

Comment 7

[Edwin Smith]

pushed changeset:   513:0f626f9187a4

Comment 8

[Andreas Gal :gal]

This unblocks the amd64 nanojit port. We will try to get that in today, assuming there are no major collisions that need a 2nd review.