Closed Bug 659782 Opened 13 years ago Closed 13 years ago

IonMonkey: Infinite loop in IR generator

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: dvander)

References

Details

Attachments

(3 files)

Attached file Test case.
The attached test case loops infinitely and quickly OOMs on IM tip. Backtrace:

(gdb) bt
#0  0x000000000070f4a7 in js::ion::MBasicBlock::MBasicBlock (this=0x0, gen=0x7fffffffbe50, pc=0xb6cd08 "") at /home/adrake/moz/ionmonkey/js/src/ion/MIRGraph.cpp:91
#1  0x000000000070f3d5 in js::ion::MBasicBlock::New (gen=0x7fffffffbe50, pred=0x5023770, entryPc=0xb6cd08 "") at /home/adrake/moz/ionmonkey/js/src/ion/MIRGraph.cpp:67
#2  0x000000000070f479 in js::ion::MBasicBlock::NewLoopHeader (gen=0x7fffffffbe50, pred=0x5023770, entryPc=0xb6cd08 "") at /home/adrake/moz/ionmonkey/js/src/ion/MIRGraph.cpp:80
#3  0x000000000070c2c1 in js::ion::IonBuilder::newLoopHeader (this=0x7fffffffbe50, predecessor=0x5023770, pc=0xb6cd08 "") at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:1217
#4  0x000000000070b696 in js::ion::IonBuilder::doWhileLoop (this=0x7fffffffbe50, op=JSOP_NOP, sn=0xb6cd1d " \020!\017\263\v\263\263") at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:937
#5  0x000000000070b44a in js::ion::IonBuilder::maybeLoop (this=0x7fffffffbe50, op=JSOP_NOP, sn=0xb6cd1d " \020!\017\263\v\263\263") at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:877
#6  0x0000000000709d54 in js::ion::IonBuilder::snoopControlFlow (this=0x7fffffffbe50, op=JSOP_NOP) at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:308
#7  0x0000000000709c0c in js::ion::IonBuilder::traverseBytecode (this=0x7fffffffbe50) at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:284
#8  0x0000000000709b0f in js::ion::IonBuilder::analyze (this=0x7fffffffbe50) at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:215
#9  0x00000000007093fc in js::ion::Go (cx=0xb0e150, script=0xb6cc50, fp=0x7ffff17be0c0) at /home/adrake/moz/ionmonkey/js/src/ion/IonBuilder.cpp:72
#10 0x00000000006e8668 in js::Interpret (cx=0xb0e150, entryFrame=0x7ffff17be048, inlineCallCount=1, interpMode=js::JSINTERP_NORMAL) at /home/adrake/moz/ionmonkey/js/src/jsinterp.cpp:4660
#11 0x00000000004c207f in js::RunScript (cx=0xb0e150, script=0xb6c660, fp=0x7ffff17be048) at /home/adrake/moz/ionmonkey/js/src/jsinterp.cpp:614
#12 0x00000000004c344b in js::Execute (cx=0xb0e150, chain=..., script=0xb6c660, prev=0x0, flags=0, result=0x0) at /home/adrake/moz/ionmonkey/js/src/jsinterp.cpp:975
#13 0x0000000000430866 in JS_ExecuteScript (cx=0xb0e150, obj=0x7ffff1602048, scriptObj=0x7ffff1602240, rval=0x0) at /home/adrake/moz/ionmonkey/js/src/jsapi.cpp:4942
#14 0x0000000000404f33 in Process (cx=0xb0e150, obj=0x7ffff1602048, filename=0x7fffffffe53c "/home/adrake/loop.js", forceTTY=0) at /home/adrake/moz/ionmonkey/js/src/shell/js.cpp:452
#15 0x0000000000405f69 in ProcessArgs (cx=0xb0e150, obj=0x7ffff1602048, argv=0x7fffffffe220, argc=1) at /home/adrake/moz/ionmonkey/js/src/shell/js.cpp:975
#16 0x0000000000410edf in Shell (cx=0xb0e150, argc=1, argv=0x7fffffffe220, envp=0x7fffffffe230) at /home/adrake/moz/ionmonkey/js/src/shell/js.cpp:5918
#17 0x00000000004111ce in main (argc=1, argv=0x7fffffffe220, envp=0x7fffffffe230) at /home/adrake/moz/ionmonkey/js/src/shell/js.cpp:6051
There were a few bugs here, most likely fallout from for-loop refactoring:

 (1) Do-while did not update the pc to the successor block
 (2) If-no-else did not update the pc to the successor block
 (3) Do-while did not create a successor block
 (4) Do-while did not pass a parameter to finalizeLoop that would insert MTest
 (5) finalizeLoop() used the wrong successor
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #535246 - Flags: review?(adrake)
The other major bug is that we expected operator |new| to not call constructors if the overload returned NULL. This patch removes OOM checks for small allocations, and like TM/JM we'll rely on a ballast instead (which is left unimplemented for now).
Attachment #535252 - Flags: review?(adrake)
Comment on attachment 535252 [details] [diff] [review]
get rid of OOM checks

One thing that worries which stood out while I was reading this patch is that there's no clear way to know what functions /should/ be null-checked at a glance. For example, newBlock can fail because MBasicBlock::New can fail because it makes a variable sized allocation. But, at a glance, I would not bother to check it because virtually every other function of that form doesn't need to be checked. I'm worried we're going to end up with tons of OOM crash bugs because we just forgot to check the functions which called fallible functions ten layers down. There's not really an easy way that I see to fix this without resorting to __attribute__((warn_unused_result)) or some such, but that has to be manually propagated or else it does nothing. Even then, it doesn't cover return NULL.

I don't see anything technically wrong, but if we find a solution to this it might be good to apply it before putting this into the tree. 

Thoughts?
Attachment #535252 - Flags: review?(adrake) → review+
Attachment #535246 - Flags: review?(adrake) → review+
Yeah, that's a good point. The annotation sounds decent. We could have a policy of no OOM on "tiny structures" or something. We could also account for bigger structures in the ballast. At most we're going to allocate one snapshot, or two basic blocks per opcode.
Just another idea: in addition to warn_unused (which I do think is a good idea), it might be useful to employ Dehydra to do statically verify our proposed ballasting, or even give some tight bounds.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
warn_unused_result works much better if you have signatures like this:

  bool create(Thing **thing)  // return |false| on failure

as opposed to:

  Thing *create()  // return |NULL| on failure

because with the latter you always use the result somehow (unless the call is truly dead, which is the rare case).

The return-bool version is a little more awkward to use, but it might be worth it for safety.  Plus it'll work now, as opposed to some vaporware Dehydra analysis that'll never get implemented :)
You need to log in before you can comment on or make changes to this bug.