Closed Bug 608105 Opened 10 years ago Closed 2 years ago

Improve underrunProtect() usage on non X86 platforms


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

Not set


(Not tracked)



(Reporter: wmaddox, Assigned: ginnchen+exoracle)




(1 file)

On X86, underrunProtect() calls are generally pushed down into the low-level emitters for single machine instructions.  Calls in the Assembler:asm_* family can then freely emit instructions without worrying about buffer space calculations.

On Sparc, MIPS, PPC, and SH4, the underrunProtect() calls appear at a higher level, in asm_* routines, and reserve space for multiple instructions at a time.  This leads to difficulty in computing the needed reservation when instructions are emitted conditionally, particularly when emitted in subsidiary routines.  Several cases of omitted underrunProtect() calls have surfaced recently, and inspection of NativeSparc.cpp reveals cases where it appears that the reservations are conservative.

Presumably, one motivation for this convention is to allow for sequences of instructions that must remain contiguous, and cannot be interrupted by the jump to another chunk of code.  Perhaps we could make underrunProtect() more robust by giving it a scope, e.g., using RAII, and verifying that no more code was actually generated than space was reserved.

   // save current _nIns in new object of class 'underrunProtect'
   underrunProtect(this, 32);

   ... emit code here ...

}  // destructor checks against declared reservation
Depends on: 608098
Attached patch patch for SPARCSplinter Review
1) The old underrunProtect code is wrong about (eip - n < codeStart),
so actually we protect 4 times larger.

2) Use UnderrunProtectCheck class to do the protect and check if we underestimate the size. Usually UnderrunProtectCheck should be used after findRegFor().

3) Not all uses of UnderrunProtectCheck are really needed, but I don't want to spend time on fix this.

4) Combined fix for Bug 535708.

5) Several code styling fixes.

Tested with make check for debug and release build on nanojit-central and tracemonkey.
Assignee: nobody → ginn.chen
Attachment #496737 - Flags: review?(wmaddox)
No longer depends on: 608098
Duplicate of this bug: 608098
Duplicate of this bug: 535708
Attachment #496737 - Attachment description: patch → patch for SPARC
Comment on attachment 496737 [details] [diff] [review]
patch for SPARC

HI, Ginn.  Sorry I sat on this for so long and didn't get it reviewed.

Overall, these are worthwhile changes that will improve the code, and is good work that should be committed.  I've given the patch an R- though, because I have a few questions about correctness, and the areas in question at least need to be be better commented if in fact they are correct.  Also, there are several unrelated cleanups being made here, and these need to be broken out into separate patches:

1) The UnderrunProtectCheck class and changes elsewhere to use it.
2) Usage of prepareResultReg() and removal of deprecated regalloc calls.
3) Many cosmetic fixes to indentation style and '*'-placement conventions.

The UnderrunProtectCheck class is a nice improvement, and should make the debug builds much more useful in finding errors.  It would be worthwhile to explain it in comments, though.  Further suggestions:
  - Declare ctor and dtor as inline, in which case no extra call overhead will be introduced in the release builds.
  - Stylistically, it may be preferable to introduce a new, dedicated block for each declaration of an UnderrunProtectCheck instance even if the enclosing block happens to have the right scope.  This makes it a bit clearer that the RAII idiom is being used, and that the block+declaration is really functioning as a special kind of statement protecting an invariant within its body rather being a real declaration of a variable to be used as such.

In most cases, you've preserved the same reservation in an UnderrunProtectCheck declaration as previously existed in the naked underrunProtect() call.  In a few, however, the allocation as been increased.  It is not at all obvious where instructions are being generated and how many -- many calls that look like single instruction emitters, and follow the all-caps naming convention, in fact may expand into multiple instructions.  It seems like the only way to make this clearly transparent to the reader would be to use a naming convention that tagged each emitter with the maximum number of byte that it might generate, including subsidiary calls.  The correctness of reservations needs to be made obvious by a purely local inspection of each emitter.

On the other hand, it's not at all clear why there are so many underrunProtect calls.  In general, it should be sufficient simply to reserve sufficient space for each individual machine instruction immediately before emitting the actual instruction bits.  It is only necessary to reserve more space than for a single instruction when those instructions must be physically adjacent, or when a jump to the another chunk would otherwise corrupt machine state that must be preserved.  On the face of it, the only cases I can think of where an underrun protect must span multiple instructions involve emitters that generate internal branches or skips.  Can you explain why such multiple-instruction reservations are used so pervasively?  At a minimum, the reasoning behind these needs to explained in commentary.  I'd be much happier if the SPARC code generator looked a lot more like the i386 one in this respect. Is there some SPARC-specific issue that I'm missing here?
Attachment #496737 - Flags: review?(wmaddox) → review-
(In reply to comment #1)
> Created attachment 496737 [details] [diff] [review]
> patch for SPARC

> 3) Not all uses of UnderrunProtectCheck are really needed, but I don't want to
> spend time on fix this.

I mentioned this in review feedback, but it looks like you were specifically leaving the unecessary calls out of the scope of this patch.  That's fair enough if the code is correct.  My concern is not so much that there are unnecessary calls, but that it's not clear enough that all of the necessary ones are present, or that the allocations are sufficient.  I'm looking for a combination of design, naming and coding conventions, and documentation that would permit a reliable audit of this.
Added Rick and Edwin to the CC list.  SPARC isn't the only platform that makes extensive use of underrunProtect calls reserving multiple instructions at once.  Browsing through the NativePPC.cpp and NativeMIPS.cpp, however, there is quite a bit of commentary, and much easier to see what is going on.  Usually, there's stated reason why instructions must be contiguous -- filling delay slots, keeping a branch target near, etc.  NativeSPARC.cpp seems much more opaque to me, but it's not clear whether there's anything more to it than the lack of commentary.  I'd like to open this up to a wider discussion, since much of what is going on in NativeSPARC.cpp is following precedent established elsewhere, and was perhaps already hashed out in the distant past.
I agree with your comments.

I think I should split the patch into several patches in another bug and leave this bug open for further improvements on all platforms.

I don't want to block the work of NativeSPARC.cpp with this big patch.
Actually I do have some fixes / improvements of NativeSPARC.cpp after this one.

Flags: flashplayer-qrb+
Target Milestone: --- → Future
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.