Assert on allocation failure; bigger trampolines needed?

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dvander, Unassigned)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 327889 [details] [diff] [review]
adds assertion when pagesGrow() fails
Attachment #327889 - Flags: review?(edwsmith)

Comment 2

10 years ago
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

10 years ago
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
Attachment #327889 - Flags: review?(edwsmith) → review-

Comment 4

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

10 years ago
Created attachment 329676 [details] [diff] [review]
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.
Attachment #327889 - Attachment is obsolete: true
Attachment #329676 - Flags: review?

Updated

10 years ago
Attachment #329676 - Flags: review?(stejohns)
Attachment #329676 - Flags: review?(danderson)
Attachment #329676 - Flags: review?
Attachment #329676 - Flags: review?(danderson) → review+

Updated

10 years ago
Attachment #329676 - Flags: review?(stejohns) → review+

Comment 7

10 years ago
pushed changeset:   513:0f626f9187a4
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 8

10 years ago
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.
You need to log in before you can comment on or make changes to this bug.