Closed Bug 949171 Opened 6 years ago Closed 6 years ago

Add a bunch of asserts, fix some bugs

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(7 files, 2 obsolete files)

The following patch series adds a bunch of JS_ASSERTs, some of which that exposed otherwise hidden or latent bugs, fixes those bugs.
Attached patch padded-local-slot-count.patch (obsolete) — Splinter Review
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)
Attached patch eaa-block.patchSplinter Review
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)
Attached patch asserts.patchSplinter Review
This patch adds a bunch of assorted asserts.
Attachment #8346165 - Flags: review?(nicolas.b.pierron)
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)
Attached patch bitset-unfudge.patch (obsolete) — Splinter Review
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)
This patch makes AssertBasicGraphCoherency check more stuff.
Attachment #8346187 - Flags: review?(nicolas.b.pierron)
This patch verifies that register allocators keep safepoints more consistent earlier.
Attachment #8346189 - Flags: review?(nicolas.b.pierron)
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)
Attachment #8346164 - Flags: review?(nicolas.b.pierron) → review+
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+
Attachment #8346168 - Flags: review?(nicolas.b.pierron) → review+
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 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+
Attachment #8346189 - Flags: review?(nicolas.b.pierron) → review+
(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)
Attachment #8346158 - Attachment is obsolete: true
(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 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 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+
(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
You need to log in before you can comment on or make changes to this bug.