1322320 - CacheIR: ensure we don't attach duplicate Baseline stubs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

Baseline ICs currently have some ad-hoc checks to avoid attaching duplicate stubs (this can happen for example when we fail a bounds check and then attach a similar stub). We don't check this everywhere though.

With CacheIR we can implement this in one place by checking (right before we attach a new stub) if there's already a stub with the same CacheIRStubInfo* (which means same IR) and same stub fields.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

I added some logging and ran the sunspider/v8 jit-tests. Apparently we had a case where this happens with the dense element stub: when the index is in-bounds but the element is the hole value, we attached duplicate stubs.

With the patch we no longer do this. I also changed the initialized-length check to use containsDenseElement (it does both bounds checking and checking for holes), as it's a little faster than relying on the duplicate-stub-detection code.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8817710 [details] [diff] [review]
Patch

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

Just a general question. Should we make it a debug failure when adding a stub that has been added already?
That way we cannot add multiple similar stubs anymore and we don't have to iterate the list on release builds?

::: js/src/jit/BaselineCacheIR.cpp
@@ +1916,5 @@
>  CacheIRWriter::copyStubData(uint8_t* dest) const
>  {
>      uintptr_t* destWords = reinterpret_cast<uintptr_t*>(dest);
>  
> +    for (const StubField& field : stubFields_) {

We can do that now. Cool!

@@ +2099,5 @@
> +
> +        ICCacheIR_Monitored* otherStub = iter->toCacheIR_Monitored();
> +        if (otherStub->stubInfo() != stubInfo)
> +            continue;
> +        if (!writer.stubDataEquals((uint8_t*)otherStub + stubInfo->stubDataOffset()))

can we create a method for:
(uint8_t*)otherStub + stubInfo->stubDataOffset()

stubDataStart() or something?

::: js/src/jit/CacheIR.cpp
@@ +901,1 @@
>          return false;

Nice find!

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> Just a general question. Should we make it a debug failure when adding a
> stub that has been added already?
> That way we cannot add multiple similar stubs anymore and we don't have to
> iterate the list on release builds?

Good question. I know there are some edge cases the ICs check but that we currently don't check when emitting IR. I can file a follow-up bug to fix all these cases and then we can add a MOZ_ASSERT.

I first want to convert more IC code before spending time on this (and potential fuzz bugs from it), but I agree it might make sense to start enforcing this at some point. We will have to see how hard it is, but it's probably not that bad.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f456cfc23c5d
Ensure we don't attach duplicate Baseline stubs. r=h4writer

Comment 5

[Carsten Book [:Tomcat]]

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