Last Comment Bug 531888 - Overzealous compilers don't like Assembler::intersectRegisterState
: Overzealous compilers don't like Assembler::intersectRegisterState
Status: RESOLVED FIXED
fixed-in-nanojit, fixed-in-tracemonke...
:
Product: Core Graveyard
Classification: Graveyard
Component: Nanojit (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Future
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-30 10:48 PST by Steven Johnson
Modified: 2014-03-17 08:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.21 KB, patch)
2009-11-30 10:59 PST, Steven Johnson
rreitmai: review+
n.nethercote: review+
Details | Diff | Splinter Review

Description Steven Johnson 2009-11-30 10:48:30 PST
This funtion contains the statement

        for (Register r=LastReg; 
             r >= FirstReg && r <= LastReg;
             r = prevreg(r))

which generates

    error: comparison is always true due to limited range of data type

on current android versions of gcc, causing spurious build failures.
Comment 1 Rick Reitmaier 2009-11-30 10:54:21 PST
I guess we could put NanoAssert(r <= LastReg); in the body and remove the condition from the check.
Comment 2 Steven Johnson 2009-11-30 10:59:10 PST
Created attachment 415184 [details] [diff] [review]
Patch

One possible patch (fixes it). Open for more elegant solutions.
Comment 3 Rick Reitmaier 2009-11-30 12:29:30 PST
Comment on attachment 415184 [details] [diff] [review]
Patch

Fix looks fine (albeit ugly), but do we know the source of the warning?  Can we just tweak the comparison(s) and add any appropriate asserts.
Comment 4 Steven Johnson 2009-11-30 12:47:24 PST
(In reply to comment #3)
> (From update of attachment 415184 [details] [diff] [review])
> Fix looks fine (albeit ugly), but do we know the source of the warning?  Can we
> just tweak the comparison(s) and add any appropriate asserts.

Source is that Register can't (legally) ever be outside that range; prevreg/nextreg hack things to make that possible, but the compiler can't tell that.

Should I get a mozillan to review this too?
Comment 5 Rick Reitmaier 2009-11-30 12:50:31 PST
I see and yes having Nick look at it would be worthwhile.  

I'd prefer not to push this change (maybe I should have marked it '-'?) unless we are truly forced.
Comment 6 Steven Johnson 2009-11-30 12:56:05 PST
Comment on attachment 415184 [details] [diff] [review]
Patch

Adding Nick (or do you prefer Nicholas?) ... it's an ugly fix I'm not particularly happy with, so I'm open for better suggestions.
Comment 7 Nicholas Nethercote [:njn] 2009-11-30 20:09:44 PST
Comment on attachment 415184 [details] [diff] [review]
Patch

"Nick" is fine.

This is ugly but at least it has a comment.  Although I'd appreciate a capital letter at the start of the comment and a full stop at the end :)
Comment 8 Steven Johnson 2009-12-23 09:56:49 PST
Pretty sure that this has been pushed to all relevant repos, shall we close it?

Note You need to log in before you can comment on or make changes to this bug.