Closed
Bug 730108
Opened 12 years ago
Closed 12 years ago
IonMonkey: Pool bug
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Assigned: mjrosenb)
Details
Attachments
(1 file, 1 obsolete file)
21.83 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
The bug is kind of silly, and easily fixable, but there is a similar bug that will require a bit more effort to fix. What goes wrong: 1) choose a perforation point (unconditional branch) before making any pool references 2) generate a whole bunch of code without any unconditional branches 3) generate a pool reference At this point, there are several thousand bytes between the perforation point and the reference to the pool. Eventually, the pool is flushed, and we attempt to use the perforation point, so all of the constants are (correctly) placed into the back-reference half of the pool, at which point they are out of range of the actual pool. When we go to generate the references, an assertion is thrown. The easiest fix for now is to not select an unconditional branch as a perforation point if there are no pool entries. A more general fix will be to monitor when an element moves out of range of the backwards pool while it is being filled from the forwards pool, and to immediately start a new forwards pool
Assignee | ||
Comment 1•12 years ago
|
||
Unfortunately, I am a bad person, and while writing this patch, also managed to include the fix for bug 728850. Since there isn't a good file-level separation between the patches, and I was planning on having Jacob review both patches in any case, I decided to not split them.
Attachment #601579 -
Flags: review?(Jacob.Bramley)
Comment 2•12 years ago
|
||
Comment on attachment 601579 [details] [diff] [review] /home/mrosenberg/patches/more_pool_fixes-r0.patch Review of attachment 601579 [details] [diff] [review]: ----------------------------------------------------------------- The only major issue is a pointer aliasing violation, but this could be fixed with a minor modification. ::: js/src/ion/arm/Assembler-arm.cpp @@ +1969,5 @@ > new (i) InstBLImm(BOffImm(offset), cond); > JSC::ExecutableAllocator::cacheFlush(i, 4); > } > +struct PoolHeader { > + // size should take into account the pool header. You should also mention that the units are sizeof(Instruction), not bytes. @@ +2053,5 @@ > +Assembler::nextInstruction(uint8 *inst_, uint32 *count) > +{ > + Instruction *inst = reinterpret_cast<Instruction*>(inst_); > + if (count != NULL) > + *count += 4; a: 'sizeof(Instruction)' might be better than '4'. b: The way this is called, I expected *count to be an offset between inst_ and the next instruction. That is, 'inst->next() - inst_'. The caller could just as well use a 'for' loop to calculate the count value as it's implemented here. @@ +2058,5 @@ > + return reinterpret_cast<uint8*>(inst->next()); > +} > +bool extractPoolHeader(Instruction *inst, PoolHeader **ph) > +{ > + PoolHeader *tmp = reinterpret_cast<PoolHeader *>(inst); Pointer aliasing is hard to avoid at times, but it's dangerous and can cause bugs that are obscure and hard to debug. This line of code violates C/C++ pointer aliasing rules, since PoolHeader and Instruction are not compatible types. @@ +2070,5 @@ > +// Cases to be handled: > +// 1) no pools in sight => return this+1 > +// 2) this+1 is an artificial guard for a pool => return first instruction after the pool > +// 3) this+1 is a natural guard => return the branch > +// 4) this is a branch, right before a pool => return first instruction after the pool This is a helpful comment. @@ +2077,5 @@ > +{ > + Assembler::Condition c; > + Instruction *ret = this+1; > + bool thisIsGuard = false; > + bool nextIsGuard = false; These booleans are unused. @@ +2085,5 @@ > + // If it isn't a guard, then start looking ahead. > + if (c == Assembler::Always && > + (this->is<InstBXReg>() || this->is<InstBImm>()) && > + extractPoolHeader(ret, &ph)) { > + return ret+ph->size; Spaces around the '+'. @@ +2090,5 @@ > + } else { > + ret->extractCond(&c); > + if (c == Assembler::Always && (ret->is<InstBXReg>() || ret->is<InstBImm>())) { > + if (extractPoolHeader(ret + 1, &ph) && !ph->isNatural) > + return ret + 1 + ph->size; This could be much clearer if you made helper functions to check for guards. To illustrate: if (this->isPoolGuard(&ph)) { return ret + ph->size; } else if (ret->isArtificialPoolGuard(&ph)) { return ret + 1 + ph->size; } return ret; ::: js/src/ion/arm/Assembler-arm.h @@ +1683,5 @@ > *c = (Assembler::Condition)(data & 0xf0000000); > } > // Get the next instruction in the instruction stream. > // This will likely eventually do neat things like ignore > // constant pools and their guards. You did that! The comment needs updating. ::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h @@ +204,5 @@ > } > int getAlignment() { > return alignment; > } > + uint32 addPool(uint32 start) { The name of this method is a little misleading. I'm not sure that I can think of a better one, though. What do you think of "addPoolSize"?
Attachment #601579 -
Flags: review?(Jacob.Bramley) → review+
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Jacob Bramley [:jbramley] from comment #2) > a: 'sizeof(Instruction)' might be better than '4'. > > b: The way this is called, I expected *count to be an offset between inst_ > and the next instruction. That is, 'inst->next() - inst_'. The caller could > just as well use a 'for' loop to calculate the count value as it's > implemented here. > Possibly, but |inst->next() - inst| is going to return the number of bytes between them, rather than the number of bytes worth of instructions between them, the important difference being pools. If there were a pool between inst and inst->next(), the value assumed would be huge, and wrong. > Pointer aliasing is hard to avoid at times, but it's dangerous and can cause > bugs that are obscure and hard to debug. This line of code violates C/C++ > pointer aliasing rules, since PoolHeader and Instruction are not compatible > types. > I've decided to go a slightly more complex route, and make PoolHeader inherit from Instruction. I'll probably do the same for pool hints, eventually. > @@ +2090,5 @@ > > + } else { > > + ret->extractCond(&c); > > + if (c == Assembler::Always && (ret->is<InstBXReg>() || ret->is<InstBImm>())) { > > + if (extractPoolHeader(ret + 1, &ph) && !ph->isNatural) > > + return ret + 1 + ph->size; > > This could be much clearer if you made helper functions to check for guards. > To illustrate: > > if (this->isPoolGuard(&ph)) { > return ret + ph->size; > } else if (ret->isArtificialPoolGuard(&ph)) { > return ret + 1 + ph->size; > } > return ret; > Indeed it is!
Assignee | ||
Comment 4•12 years ago
|
||
normally fixing nits isn't cause for re-review, but I did make some pretty serious changes to PoolHeader.
Attachment #601579 -
Attachment is obsolete: true
Attachment #602274 -
Flags: review?(Jacob.Bramley)
Comment 5•12 years ago
|
||
Comment on attachment 602274 [details] [diff] [review] /home/mrosenberg/patches/more_pool_fixes-r1.patch Review of attachment 602274 [details] [diff] [review]: ----------------------------------------------------------------- Good stuff! ::: js/src/ion/arm/Assembler-arm.cpp @@ +1985,5 @@ > + Header(const Instruction *i) { > + JS_STATIC_ASSERT(sizeof(Header) == sizeof(uint32)); > + memcpy(this, i, sizeof(Header)); > + JS_ASSERT(ONES == 0xffff); > + } The indentation is wrong here. @@ +2003,5 @@ > + Header tmp(this); > + return tmp.isNatural; > + } > + static bool isTHIS(const Instruction &i) { > + return (*i.raw() & 0xffff0000) == 0xffff0000; This same mechanism could be used for size() and isNatural() too, and then you could do away with the whole Header struct and its bitfields. Maybe I'm too used to manually masking bits off in C! The Header allows you to use this 4-byte header even if sizeof(Instruction) is not 4, but isTHIS makes that assumption anyway. @@ +2128,5 @@ > + // If it isn't a guard, then start looking ahead. > + if (instIsGuard(this, &ph)) { > + return ret + ph->size(); > + } else { > + if (instIsArtificialGuard(ret, &ph)) I'd squash that onto a single 'else if' line. ::: js/src/ion/arm/Assembler-arm.h @@ +1689,3 @@ > // Sometimes, an api wants a uint32 (or a pointer to it) rather than > // an instruction. raw() just coerces this into a pointer to a uint32 > + uint32 *raw() const { return (uint32*)this; } Actually, now I look, the raw() method violates pointer aliasing rules too. I think you could fix it by returning &data (and then no cast is required anyway).
Attachment #602274 -
Flags: review?(Jacob.Bramley) → review+
Assignee | ||
Comment 6•12 years ago
|
||
landed: http://hg.mozilla.org/projects/ionmonkey/rev/af7981fd0ca4
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•