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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

Attachment: Link the bindname stub shape guards to the exit sequence.

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.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/0aad954c48ed

Comment 3

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0aad954c48ed