1215555 - Improve coverage of simulated OOM testing for ARM compilation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

As mentioned by jolesen, we have poor coverage when testing ARM compilation since we will only simulate OOM when we allocate a new buffer slice not at all possible failure points.

(I didn't realise at first that we use a separate AssemblerBuffer for ARM (and in fact for MIPS too).  On X86 platforms AssemblerBuffer is based on a Vector, and we already simulate OOM any time we append to a Vector.)

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

This would be pretty easy to fix in AssemblerBuffer::ensureSpace().

Feel free to assign to me.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

I just attached patches to bug 1207827 which use a Vector for the constant pool data structures in AssemblerBufferWithConstantPools.

The main instruction buffer still needs better OOM simulation, though.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1215555-oom-in-assembler-buffer

Here's a patch to add this and fix the problems.  Mostly it's a case of not using offsets into the buffer if we've had OOM.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #3)
Probably some of this is unnecessary with your patches in bug 1207827.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8675000 [details] [diff] [review]
bug1215555-oom-in-assembler-buffer

Review of attachment 8675000 [details] [diff] [review]:
-----------------------------------------------------------------

This is general goodness, and there is some redundancy with the patches I attached to bug 1207827.

Looks good!

::: js/src/jit/arm/Assembler-arm.cpp
@@ +2388,5 @@
>      if (l->bound()) {
>          // Note only one instruction is emitted here, the NOP is overwritten.
>          BufferOffset ret = allocBranchInst();
> +        if (m_buffer.oom())
> +            return BufferOffset();

I like this better than the old multi-line version.

@@ +2398,5 @@
>          return ret;
>      }
>  
> +    if (m_buffer.oom())
> +        return BufferOffset();

Here and above: Any reason to call m_buffer.oom() instead of simply oom()? Assembler::oom() is more general.

@@ +2419,5 @@
>      }
> +    if (!oom()) {
> +        DebugOnly<int32_t> check = l->use(ret.getOffset());
> +        MOZ_ASSERT(check == old);
> +    }

Here |ret| is unassigned in case of OOM, so returning it is correct. Is that too subtle?

How about the more direct:

if (oom())
  return BufferOffset()

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +154,5 @@
>          // Space can exist in the most recent Slice.
> +        if (tail && tail->length() + size <= tail->Capacity()) {
> +            // Simulate allocation failure even when we don't need a new slice.
> +            if (js::oom::ShouldFailWithOOM())
> +                return fail_oom();

Yes! Just like Vector.

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +704,5 @@
>          // Mark and emit the guard branch.
>          markNextAsBranch();
>          this->putBytes(guardSize_ * InstSize, nullptr);
> +        if (this->oom())
> +            return;

This is going to collide with bug 1207827 which rewrites this bit of code completely.

@@ +865,5 @@
>              finishPool();
>          }
>  
>          inhibitNops_ = true;
> +        while ((sizeExcludingCurrentPool() & (alignment - 1)) && !this->oom())

Oooh, infinite loop?

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #5)

Thanks for the comments.

> Here and above: Any reason to call m_buffer.oom() instead of simply oom()?
> Assembler::oom() is more general.

Good idea, that's better.

> How about the more direct:
> 
> if (oom())
>   return BufferOffset()

Yes, good plan.

> Oooh, infinite loop?

Indeed :)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0fadc97308

Comment 8

[Jon Coppeard (:jonco)]

Backed out for bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6a1fffd326

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1deba998864f

Comment 10

[Jon Coppeard (:jonco)]

Backed out again for Android mochitest failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aec32dbd6be

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

Adding test coverage is a thankless job ;-)

Thanks, Jon!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d5bfcd2571

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5d5bfcd2571