Closed Bug 614907 Opened 9 years ago Closed 9 years ago

The shape guard in BindName is not linked to the exit sequence.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

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

People

(Reporter: jbramley, Assigned: jbramley)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

http://hg.mozilla.org/tracemonkey/file/2000c21b7910/js/src/methodjit/PolyIC.cpp#l1551

The 'shapeTest' jump is not linked, so the shape guard in the BindName stub never actually does anything. On x86 and amd64, an unpatched branch points to the next instruction, so this fails silently. On ARM, an unpatched branch jumps to an invalid address.

I have produced a patch for this as part of the PIC porting effort [bug 588021]: http://hg.mozilla.org/users/cleary_mozilla.com/tm-arm-pic-mq/file/32a00b4bdfc9/fix-bindname-stub-shape-jump

It might be worth pushing the fix before PICs are ready, so I'm raising a separate bug. (Note that I don't have a test-case that catches this problem.)

We should probably also consider making the default jump on x86 and amd64 branch to an invalid address, so we can catch this kind of error more easily. Is this feasible? The jumps are relative and the code location isn't known at assemble-time, so making the jumps fail reliably might be tricky. A jump-to-self might work. Infinite loops can be a nuisance but they're obvious in a debugger, at least.
Attachment #493344 - Flags: review?(dvander)
Comment on attachment 493344 [details] [diff] [review]
Link the bindname stub shape guards to the exit sequence.

Nice catch. Yeah, I think that's a great idea, we've made these mistakes a few times and they're always a pain to track down.

It'd be really nice if a Jump's destructor could assert that it was patched. It might not be too hard if in debug mode a Jump also stored its assembler.
Attachment #493344 - Flags: review?(dvander) → review+
blocking2.0: --- → final+
http://hg.mozilla.org/mozilla-central/rev/0aad954c48ed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.