603576 - findSpecificRegFor should include assertion for allocated register

Status: RESOLVED WORKSFORME

(Core Graveyard :: Nanojit, enhancement)

Description

[Tero Koskinen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fi-FI; rv:1.9.2.10) Gecko/20100915 Ubuntu/10.04 (lucid) Firefox/3.6.10
Build Identifier: 

Inspired by comment https://bugzilla.mozilla.org/show_bug.cgi?id=602834#c4 , function findSpecificRegFor in nanojit/Assembler.cpp should have an assertion which checks that we really got the register we wanted.



Reproducible: Always

Steps to Reproduce:
1. No steps.

Comment 1

[Tero Koskinen]

Attachment: Fix proposal.

Attached patch adds the assertion.

Comment 2

[Jacob Bramley [:jbramley]]

Comment on attachment 482484 [details] [diff] [review]
Fix proposal.

Looks good.

Just as a sanity check, we should test this on x86. It'll be tested on ARM as part of bug 602834. The Tamarin acceptance suite takes ages on ARM, so testing in batches is convenient.

Comment 3

[Tero Koskinen]

Attachment: Fix proposal, v2.

Oops, the first version of the patch didn't compile with assertions turned off:
Assembler.cpp:426: error: unused variable `r'

New version of the patch should fix that.

tamarin-redux compiles cleanly (debug and release builds) with the patch, but I haven't completely run the test suite yet.

Comment 4

[Nicholas Nethercote [inactive]]

The patch is fine.  Did you submit it because you've seen a case where it is violated, or are you just being cautious?

Comment 5

[Tero Koskinen]

I am just cautious. Comments didn't say that it is guaranteed to get the register you want and the code was not clear enough for me, so I though that I make it sure by placing an assertion there.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Regallocs have these kinds of assertions too and Nanojit is long gone.