Closed Bug 596823 Opened 14 years ago Closed 14 years ago

TM: "Assertion failure: "Constantly false guard detected": 0"

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: gkw, Assigned: n.nethercote)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(1 file, 1 obsolete file)

for (a = 0; a < 9; a++) {
  "".charAt(3 / 0)
}

asserts js debug shell on TM changeset a409054e1395 with -j at Assertion failure: "Constantly false guard detected": 0
blocking2.0: --- → ?
Summary: "Assertion failure: "Constantly false guard detected": 0" → TM: "Assertion failure: "Constantly false guard detected": 0"
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   48671:fd0411f5ce7f
user:        Andreas Gal
date:        Thu Aug 05 22:54:34 2010 -0700
summary:     Optimize string[idx] on trace (584499, r=lw).
Blocks: 584499
blocking2.0: ? → beta8+
Attached patch Proposed patch (obsolete) — Splinter Review
The same assertion occurs with this testcase:
for (a = 0; a < 9; a++) {
  "abc".charCodeAt(1.5);
}

It's a result of getCharAt/getCharCodeAt calling makeNumberInt32 with a double that is not integral.

I've uploaded a patch that seems to fix it. Though my understanding of the this part of the code base is quite limited (to say the least), so it probably isn't the best solution.
Comment on attachment 480413 [details] [diff] [review]
Proposed patch

Switching to gal for feedback (I hope I'm correct) - also AIUI a test should also be provided..
Attachment #480413 - Flags: feedback?(gal)
Comment on attachment 480413 [details] [diff] [review]
Proposed patch

Stealing the review, I've been looking at this stuff lately.
Attachment #480413 - Flags: feedback?(gal) → feedback?(nnethercote)
Changing the makeNumberInt32() calls to demote() doesn't seem right.  I think demote() is only supposed to be used on values for which isPromoteInt() succeeds.  If the test was changed to this:

  for (a = 0; a < 9; a++) {
    "....".charAt(1 / 3)
  }

demote() would be called on an immediate with value 1/3.
The bounds check looks good, though, because it matches the charCodeAt case.  Just adding the bounds check is enough to make the test in comment 0 pass, but the one in comment 5 asserts.
Attached patch Proposed patch 2Splinter Review
Attachment #480413 - Attachment is obsolete: true
Attachment #480872 - Flags: feedback?(nnethercote)
Attachment #480413 - Flags: feedback?(nnethercote)
Comment on attachment 480872 [details] [diff] [review]
Proposed patch 2

I'm pretty sure that isn't right.  The 'i' you see at record-time may be different when the code is executed again later.

Bug 601009 comment 3 has a proposal for a more general solution to avoiding this assertion, which will fix this bug.  I'll work on it tomorrow.  Thanks for the patches!
Attachment #480872 - Flags: feedback?(nnethercote) → feedback-
(In reply to comment #8)
> 
> Bug 601009 comment 3 has a proposal for a more general solution to avoiding
> this assertion, which will fix this bug.  I'll work on it tomorrow.

This patch is now up, it fixes the test case in comment 0.
Depends on: 601009
Setting assignee field so this doesn't look like an unassigned blocker.
Assignee: general → nnethercote
Fixed by bug 601009.
Severity: critical → normal
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: