Closed Bug 847605 Opened 11 years ago Closed 11 years ago

Frequent jit-test TEST-UNEXPECTED-FAIL | jit_test.py --no-jm | auto-regress/bug755564.js: Assertion failure: GeneralRegisterSet(Registers::SingleByteRegs).has(lhs), at ../../../js/src/ion/shared/Assembler-x86-shared.h:726 and more

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: RyanVM, Unassigned)

References

Details

(Keywords: assertion, intermittent-failure)

Attachments

(1 file)

I don't know exactly when this regressed because we hadn't been running these since moz.build landed last week. However, inbound is now closed for this due to the high frequency with which it's hitting.

https://tbpl.mozilla.org/php/getParsedLog.php?id=20309752&tree=Mozilla-Inbound

Linux mozilla-inbound leak test build on 2013-03-04 11:51:20 PST for push a3d0d869bd50
slave: bld-linux64-ec2-623

TEST-PASS | jit_test.py --no-ion --no-jm --no-ti| /builds/slave/m-in-lx-d-00000000000000000000/build/js/src/jit-test/tests/auto-regress/bug755564.js
TEST-PASS | jit_test.py --no-ion --no-ti| /builds/slave/m-in-lx-d-00000000000000000000/build/js/src/jit-test/tests/auto-regress/bug755564.js
FAIL - /builds/slave/m-in-lx-d-00000000000000000000/build/js/src/jit-test/tests/auto-regress/bug755564.js
TEST-UNEXPECTED-FAIL | jit_test.py --no-jm        | /builds/slave/m-in-lx-d-00000000000000000000/build/js/src/jit-test/tests/auto-regress/bug755564.js: Assertion failure: GeneralRegisterSet(Registers::SingleByteRegs).has(lhs), at ../../../js/src/ion/shared/Assembler-x86-shared.h:726
TEST-PASS | jit_test.py --no-ion --no-ti --always-mjit --debugjit| /builds/slave/m-in-lx-d-00000000000000000000/build/js/src/jit-test/tests/auto-regress/bug755564.js

and others like it...
OS: All → Linux
Hardware: All → x86
This bug is coming from Bug 807853, by adding a second use of testb which does not ensure that operands can be encoded.

--- a/js/src/ion/shared/MacroAssembler-x86-shared.h
+++ b/js/src/ion/shared/MacroAssembler-x86-shared.h
@@ -140,6 +143,10 @@ class MacroAssemblerX86Shared : public Assembler
         testl(Operand(address), imm);
         j(cond, label);
     }
+    void branchTestBool(Condition cond, const Register &lhs, const Register &rhs, Label *label) {
+        testb(lhs, rhs);
+        j(cond, label);
+    }
 
     // The following functions are exposed for use in platform-shared code.
     template <typename T>
Blocks: 807853
I am pushing a fix right now,

The problem might be coming from:

  LIRGenerator::visitParCheckOverRecursed(MParCheckOverRecursed *ins)
  LIRGenerator::visitParCheckInterrupt(MParCheckInterrupt *ins)

which are using "temp()" for the register used in branchTestBool.

The current work-around makes sure that we fallback on a 32 bits compare instead of a using the right registers.

Shu, Nikos, any constraints at changing these to be fixed temps?
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> I am pushing a fix right now,
> 
> The problem might be coming from:
> 
>   LIRGenerator::visitParCheckOverRecursed(MParCheckOverRecursed *ins)
>   LIRGenerator::visitParCheckInterrupt(MParCheckInterrupt *ins)
> 
> which are using "temp()" for the register used in branchTestBool.
> 
> The current work-around makes sure that we fallback on a 32 bits compare
> instead of a using the right registers.
> 
> Shu, Nikos, any constraints at changing these to be fixed temps?

Its sounds that the 2 out of line paths used for these instructions should be oolVMCall instead of callWithABI.
Ok, this fix does not seems to be needed after all as all bug failures went away with the backout of Bug 829602 from the tree.

https://hg.mozilla.org/integration/mozilla-inbound/rev/37f8e72f5124
Blocks: 829602
https://hg.mozilla.org/mozilla-central/rev/6af9594ac0d4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
It's worth noting that this patch changes the semantics of branchTestBool() in an incorrect way (it now tests the entire word, not just the least significant byte).  I will post a compensating patch as part of bug 829602, either by using tempFixed() or rewrite the relevant code to mask away the insignificant bits. (The problem here is that the functions in question test the return value from C functions that return |bool|, which is typically just one byte in size)
Given that it is taking me some time to nail down the last transient failure from bug 829602, and that there are new users for |branchTestBool()| being added in the meantime, I want to land my revised patch independently.  Hence I am reopening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Not sure about the name of the function, but it seems to fulfill a need.
Attachment #721810 - Flags: review?(nicolas.b.pierron)
Attachment #721810 - Flags: review?(nicolas.b.pierron) → review+
Try run for 1add3f03e255 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1add3f03e255
Results (out of 19 total builds):
    success: 17
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nmatsakis@mozilla.com-1add3f03e255
https://hg.mozilla.org/mozilla-central/rev/ba7e066d215c
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
No longer blocks: 829602
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: