Closed
Bug 949171
Opened 10 years ago
Closed 10 years ago
Add a bunch of asserts, fix some bugs
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: sunfish, Assigned: sunfish)
Details
Attachments
(7 files, 2 obsolete files)
1.97 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
9.46 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
6.82 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
8.44 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
4.81 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
4.22 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
The following patch series adds a bunch of JS_ASSERTs, some of which that exposed otherwise hidden or latent bugs, fixes those bugs.
Assignee | ||
Comment 1•10 years ago
|
||
StackOffsetOfPassedArg aligns its offset to be a multiple of 8. This caused it to sometimes return a stack offset which StackOffsetToSlot would convert to a slot beyond the bound implied by the graph.totalSlotCount() value. This patch moves the padding out of localSlotCount() into paddedLocalSlotCount() for clarity, and to allow assertion checks which don't need the padding to be more precise, and to fix the bug, it increases the alignment to be at least sizeof(Value).
Attachment #8346158 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•10 years ago
|
||
The AnalyzeLsh function starts at an lsh and walks down the def-use graph. At the end, it inserts an instruction after the last instruction it examined, however that instruction may not be in the same block as lsh, so in this statement: block->insertAfter(last, eaddr); last was sometimes an instruction in a different block. This patch fixes it to insert the instruction using the block containing last.
Attachment #8346164 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•10 years ago
|
||
This patch adds a bunch of assorted asserts.
Attachment #8346165 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 4•10 years ago
|
||
Since bitset is now properly enforcing its upper bounds with < instead of <=, it makes sense to rename its "max" member to "numBits".
Attachment #8346168 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 5•10 years ago
|
||
Since bitset is now properly enforcing its upper bounds with < instead of <=, it no longer needs to allocate an extra word when the number of bits it contains is a multiple of 32. This patch changes it to use a regular ceiling divide instead of a fudge factor. This also fixes the safepoint reader, which was using the same computation, and reading an extra word beyond the ostensible end of the set.
Attachment #8346169 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 6•10 years ago
|
||
This patch makes AssertBasicGraphCoherency check more stuff.
Attachment #8346187 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 7•10 years ago
|
||
This patch verifies that register allocators keep safepoints more consistent earlier.
Attachment #8346189 -
Flags: review?(nicolas.b.pierron)
Comment 8•10 years ago
|
||
Comment on attachment 8346158 [details] [diff] [review] padded-local-slot-count.patch Review of attachment 8346158 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/LIR.h @@ +1414,5 @@ > uint32_t argumentSlotCount() const { > return argumentSlotCount_; > } > uint32_t totalSlotCount() const { > + return paddedLocalSlotCount() + (argumentSlotCount() * sizeof(Value) / STACK_SLOT_SIZE); Add ArgumentsDepth() (or ArgumentsLength / ArgumentsSize / …) = argumentSlotCount() * sizeof(Value) With a JS_STATIC_ASSERT(sizeof(Value) >= STACK_SLOT_SIZE) in it. and use it here. ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +48,5 @@ > lastOsiPointOffset_(0), > sps_(&GetIonContext()->runtime->spsProfiler(), &lastPC_), > osrEntryOffset_(0), > skipArgCheckEntryOffset_(0), > + frameDepth_(graph->paddedLocalSlotCount() * sizeof(STACK_SLOT_SIZE) + Add LocalSlotsDepth() (or LocalSlotsLength / LocalSlotsSize / …) which is equal to paddedLocalSlotCount() * STACK_SLOT_SIZE And get rid of this sizeof ! ::: js/src/jit/shared/CodeGenerator-shared.h @@ +184,2 @@ > offset &= ~7; > JS_ASSERT(offset >= 0); Assert that the offset is already aligned on sizeof(Value) instead of doing "offset &= ~7;".
Attachment #8346158 -
Flags: review?(nicolas.b.pierron)
Updated•10 years ago
|
Attachment #8346164 -
Flags: review?(nicolas.b.pierron) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8346165 [details] [diff] [review] asserts.patch Review of attachment 8346165 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +1138,5 @@ > { > const LAllocation *arg = lir->getArgument(); > MIRType argType = lir->mir()->getArgument()->type(); > uint32_t argslot = lir->argslot(); > + JS_ASSERT(argslot - 1u < graph.argumentSlotCount()); Why not using <= instead of "- 1u <" ? ::: js/src/jit/LIR.cpp @@ +72,5 @@ > LBlock::lastId() > { > LInstruction *last = *instructions_.rbegin(); > JS_ASSERT(last->id()); > + JS_ASSERT(last->numDefs() == 0); nit: Add a comment to mention that the last instruction is a control flow instruction which does not have any output. ::: js/src/jit/LIR.h @@ +1404,5 @@ > uint32_t paddedLocalSlotCount() const { > // Round to StackAlignment, but also round to at least sizeof(Value) in > // case that's greater, because StackOffsetOfPassedArg rounds argument > // slots to 8-byte boundaries. > + size_t Alignment = Max(size_t(StackAlignment), sizeof(Value)); Move this change as part of the first patch. ::: js/src/jit/MIR.h @@ +2854,5 @@ > > // Set by the MCall. > void setArgnum(uint32_t argnum) { > argnum_ = argnum; > + JS_ASSERT(argnum_ >= 0); I think this is useless because we cannot declare a function with more than 2^31 arguments. ::: js/src/jit/MIRGraph.cpp @@ +772,5 @@ > > void > MBasicBlock::insertAfter(MInstruction *at, MInstruction *ins) > { > + JS_ASSERT(at->block() == this); :)
Attachment #8346165 -
Flags: review?(nicolas.b.pierron) → review+
Updated•10 years ago
|
Attachment #8346168 -
Flags: review?(nicolas.b.pierron) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8346169 [details] [diff] [review] bitset-unfudge.patch Review of attachment 8346169 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BitSet.h @@ +21,5 @@ > class BitSet : private TempObject > { > public: > static size_t RawLengthForBits(size_t bits) { > + return (bits + 32 - 1) / 32; define a static constant for "8 * sizeof(uint32_t)" and use it here instead of the 2 appearance of "32". ::: js/src/jit/Safepoints.cpp @@ +385,2 @@ > // Are there any more chunks to read? > + if (nextSlotChunkNumber_ == BitSet::RawLengthForBits(frameSlots_)) We are not using a bit set here, and the representation depends on what is encoded in the compact buffer. I'll suggest to remove the reference to RawLengthForBits as it depends on the internal representation of the bit set, and replace this condition by: nextSlotChunkNumber * bitsPerChunk >= frameSlots_ where we have declared: const size_t bitsPerChunk = 8 * sizeof(uint32_t); @@ +397,5 @@ > currentSlotChunk_ &= ~(1 << bit); > > // Return the slot, taking care to add 1 back in since it was subtracted > // when added in the original bitset. > + *slot = ((nextSlotChunkNumber_ - 1) * sizeof(uint32_t) * 8) + bit + 1; use the above bitsPerChunk here.
Attachment #8346169 -
Flags: review?(nicolas.b.pierron) → feedback+
Comment 11•10 years ago
|
||
Comment on attachment 8346187 [details] [diff] [review] basic-graph-coherency-assertions.patch Review of attachment 8346187 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonAnalysis.cpp @@ +1262,5 @@ > } > + for (MResumePointIterator iter(block->resumePointsBegin()); iter != block->resumePointsEnd(); iter++) { > + for (uint32_t i = 0, e = iter->numOperands(); i < e; i++) > + JS_ASSERT_IF(iter->getUseFor(i)->hasProducer(), > + CheckOperandImpliesUse(*iter, iter->getOperand(i))); we are in a giant "#ifdef DEBUG", I think we can use an if(…) JS_ASSERT() instead of JS_ASSERT_IF.
Attachment #8346187 -
Flags: review?(nicolas.b.pierron) → review+
Updated•10 years ago
|
Attachment #8346189 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #8) > Comment on attachment 8346158 [details] [diff] [review] > padded-local-slot-count.patch > > Review of attachment 8346158 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/LIR.h > @@ +1414,5 @@ > > uint32_t argumentSlotCount() const { > > return argumentSlotCount_; > > } > > uint32_t totalSlotCount() const { > > + return paddedLocalSlotCount() + (argumentSlotCount() * sizeof(Value) / STACK_SLOT_SIZE); > > Add ArgumentsDepth() (or ArgumentsLength / ArgumentsSize / …) = > argumentSlotCount() * sizeof(Value) > With a JS_STATIC_ASSERT(sizeof(Value) >= STACK_SLOT_SIZE) in it. > and use it here. Done. > ::: js/src/jit/shared/CodeGenerator-shared.cpp > @@ +48,5 @@ > > lastOsiPointOffset_(0), > > sps_(&GetIonContext()->runtime->spsProfiler(), &lastPC_), > > osrEntryOffset_(0), > > skipArgCheckEntryOffset_(0), > > + frameDepth_(graph->paddedLocalSlotCount() * sizeof(STACK_SLOT_SIZE) + > > Add LocalSlotsDepth() (or LocalSlotsLength / LocalSlotsSize / …) > which is equal to paddedLocalSlotCount() * STACK_SLOT_SIZE Done. > And get rid of this sizeof ! Oddly enough, it was actually returning the right value! Done. > ::: js/src/jit/shared/CodeGenerator-shared.h > @@ +184,2 @@ > > offset &= ~7; > > JS_ASSERT(offset >= 0); > > Assert that the offset is already aligned on sizeof(Value) instead of doing > "offset &= ~7;". Good notice. Done. I also changed it to use sizeof(Value) instead of a magic number.
Attachment #8346811 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•10 years ago
|
Attachment #8346158 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #10) > Comment on attachment 8346169 [details] [diff] [review] > bitset-unfudge.patch > > Review of attachment 8346169 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/BitSet.h > @@ +21,5 @@ > > class BitSet : private TempObject > > { > > public: > > static size_t RawLengthForBits(size_t bits) { > > + return (bits + 32 - 1) / 32; > > define a static constant for "8 * sizeof(uint32_t)" and use it here instead > of the 2 appearance of "32". Done. > ::: js/src/jit/Safepoints.cpp > @@ +385,2 @@ > > // Are there any more chunks to read? > > + if (nextSlotChunkNumber_ == BitSet::RawLengthForBits(frameSlots_)) > > We are not using a bit set here, and the representation depends on what is > encoded in the compact buffer. I'll suggest to remove the reference to > RawLengthForBits as it depends on the internal representation of the bit > set, and replace this condition by: > > nextSlotChunkNumber * bitsPerChunk >= frameSlots_ > > where we have declared: > > const size_t bitsPerChunk = 8 * sizeof(uint32_t); I added bitsPerChunk, but I left RawLengthForBits. It may be an implementation detail of BitSet, but the safepoint reading/writing code is already heavily tied to the implementation of BitSet. The writer uses BitSet's raw() function and just writes out the data verbatim, and the reader assumes it knows how to read that data. It might be nice to generalize the BitSetIterator class to work on raw data; this would be a nice cleanup in general, and it would provide better encapsulation. I'll work on a separate patch for that. > @@ +397,5 @@ > > currentSlotChunk_ &= ~(1 << bit); > > > > // Return the slot, taking care to add 1 back in since it was subtracted > > // when added in the original bitset. > > + *slot = ((nextSlotChunkNumber_ - 1) * sizeof(uint32_t) * 8) + bit + 1; > > use the above bitsPerChunk here. Done.
Attachment #8346169 -
Attachment is obsolete: true
Attachment #8346825 -
Flags: review?(nicolas.b.pierron)
Comment 14•10 years ago
|
||
Comment on attachment 8346811 [details] [diff] [review] padded-local-slot-count.patch Review of attachment 8346811 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/LIR.h @@ +1415,5 @@ > uint32_t argumentSlotCount() const { > return argumentSlotCount_; > } > + size_t argumentsSize() const { > + JS_STATIC_ASSERT(sizeof(Value) >= size_t(STACK_SLOT_SIZE)); nit: >= STACK_SLOT_SIZE
Attachment #8346811 -
Flags: review?(nicolas.b.pierron) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8346825 [details] [diff] [review] bitset-unfudge.patch Review of attachment 8346825 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BitSet.h @@ +19,5 @@ > // N.B. All set operations must be performed on sets with the same number > // of bits. > class BitSet : private TempObject > { > + static const size_t bitsPerWord = 8 * sizeof(uint32_t); nit: static const are capitalized. @@ +35,5 @@ > unsigned int numBits_; > uint32_t *bits_; > > static inline uint32_t bitForValue(unsigned int value) { > + return 1l << (uint32_t)(value % bitsPerWord); nit: use C++ cast style ::: js/src/jit/Safepoints.cpp @@ +387,2 @@ > // Are there any more chunks to read? > + if (nextSlotChunkNumber_ == BitSet::RawLengthForBits(frameSlots_)) If you want to keep RawLengthForBits here, then assert that bitsPerChunk == BitSet::BitsPerWord, as bitsPerChunk refers to the stream_.readUnsigned().
Attachment #8346825 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #14) > Comment on attachment 8346811 [details] [diff] [review] > padded-local-slot-count.patch > > Review of attachment 8346811 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/LIR.h > @@ +1415,5 @@ > > uint32_t argumentSlotCount() const { > > return argumentSlotCount_; > > } > > + size_t argumentsSize() const { > > + JS_STATIC_ASSERT(sizeof(Value) >= size_t(STACK_SLOT_SIZE)); > > nit: >= STACK_SLOT_SIZE STACK_SLOT_SIZE is a ptrdiff_t, so the cast avoids a signed vs unsigned comparison warning. (In reply to Nicolas B. Pierron [:nbp] from comment #15) > Comment on attachment 8346825 [details] [diff] [review] > bitset-unfudge.patch > > Review of attachment 8346825 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/BitSet.h > @@ +19,5 @@ > > // N.B. All set operations must be performed on sets with the same number > > // of bits. > > class BitSet : private TempObject > > { > > + static const size_t bitsPerWord = 8 * sizeof(uint32_t); > > nit: static const are capitalized. Fixed. > @@ +35,5 @@ > > unsigned int numBits_; > > uint32_t *bits_; > > > > static inline uint32_t bitForValue(unsigned int value) { > > + return 1l << (uint32_t)(value % bitsPerWord); > > nit: use C++ cast style Done. > ::: js/src/jit/Safepoints.cpp > @@ +387,2 @@ > > // Are there any more chunks to read? > > + if (nextSlotChunkNumber_ == BitSet::RawLengthForBits(frameSlots_)) > > If you want to keep RawLengthForBits here, then assert that bitsPerChunk == > BitSet::BitsPerWord, as bitsPerChunk refers to the stream_.readUnsigned(). If we're going to make BitsPerWord public for now, we may as well just use it directly. https://hg.mozilla.org/integration/mozilla-inbound/rev/45e5f66557d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc169da07fe https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc494d2d940 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a14262db708 https://hg.mozilla.org/integration/mozilla-inbound/rev/54a30297e0bc https://hg.mozilla.org/integration/mozilla-inbound/rev/1a154e47c1c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/e420aeeb7d30
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/45e5f66557d8 https://hg.mozilla.org/mozilla-central/rev/cbc169da07fe https://hg.mozilla.org/mozilla-central/rev/cdc494d2d940 https://hg.mozilla.org/mozilla-central/rev/0a14262db708 https://hg.mozilla.org/mozilla-central/rev/54a30297e0bc https://hg.mozilla.org/mozilla-central/rev/1a154e47c1c9 https://hg.mozilla.org/mozilla-central/rev/e420aeeb7d30
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•