Improve underrunProtect() usage on non X86 platforms

ASSIGNED
Assigned to

Status

Tamarin
Baseline JIT (CodegenLIR)
ASSIGNED
7 years ago
6 years ago

People

(Reporter: William Maddox, Assigned: Ginn Chen)

Tracking

unspecified
Future
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

7 years ago
Depends on: 608098
(Assignee)

Comment 1

7 years ago
Created attachment 496737 [details] [diff] [review]
patch for SPARC

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
Status: NEW → ASSIGNED
Attachment #496737 - Flags: review?(wmaddox)
(Assignee)

Updated

7 years ago
No longer depends on: 608098
Duplicate of this bug: 608098
(Assignee)

Updated

7 years ago
Duplicate of this bug: 535708
(Assignee)

Updated

7 years ago
Attachment #496737 - Attachment description: patch → patch for SPARC
(Reporter)

Comment 4

7 years ago
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-
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

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

Thanks!

Updated

6 years ago
Flags: flashplayer-qrb+
Target Milestone: --- → Future
You need to log in before you can comment on or make changes to this bug.