The default bug view has changed. See this FAQ.

Overzealous compilers don't like Assembler::intersectRegisterState

RESOLVED FIXED in Future

Status

Core Graveyard
Nanojit
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Steven Johnson, Unassigned)

Tracking

unspecified
Future

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(1 attachment)

(Reporter)

Description

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

7 years ago
I guess we could put NanoAssert(r <= LastReg); in the body and remove the condition from the check.
(Reporter)

Comment 2

7 years ago
Created attachment 415184 [details] [diff] [review]
Patch

One possible patch (fixes it). Open for more elegant solutions.
Attachment #415184 - Flags: review?(rreitmai)

Comment 3

7 years ago
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.
Attachment #415184 - Flags: review?(rreitmai) → review+
(Reporter)

Comment 4

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

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

Comment 6

7 years ago
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.
Attachment #415184 - Flags: review?(nnethercote)
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 :)
Attachment #415184 - Flags: review?(nnethercote) → review+
(Reporter)

Comment 8

7 years ago
Pretty sure that this has been pushed to all relevant repos, shall we close it?

Updated

7 years ago
Component: JIT Compiler (NanoJIT) → Nanojit
Product: Tamarin → Core
QA Contact: nanojit → nanojit
Target Milestone: --- → Future

Updated

7 years ago
OS: Android → All
Hardware: ARM → All
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.