CSE should be more careful with values that have WritesPinned, ExitsSideways, or are of different sizes

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

processWasmAddressValue is not looking at the full set of basic blocks between use and def before concluding it can CSE a WasmAddressValue

[Justin Michaud]

Created attachment 455592 [details]
Crashing unity project

[Justin Michaud]

Created attachment 455593 [details]
Working unity project

[Justin Michaud]

Ruffle.rs test case repro:
1. Load https://ruffle.rs/demo/
2. Select Sample SWF "Bites of Brackenwood"
3. Click play button
4. See if we throw an out of bounds exception or not using web inspector (web inspector may need to be open for duration of the above steps).

Test case summary:

iOS 15.4 safari-613.1.17.0-branch:
- Crashing unity project crashes with OOM
- Working unity project works
- Ruffle.rs crashes

With this pass disabled:
- All 3 work

[Justin Michaud]

Created attachment 456073 [details]
Patch

[Saam Barati]

Comment on attachment 456073 [details]
Patch

r=me

[Radar WebKit Bug Importer]

<rdar://problem/91078546>

[Justin Michaud]

Created attachment 456379 [details]
Patch

[Justin Michaud]

Created attachment 456381 [details]
Patch

[Saam Barati]

Comment on attachment 456381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456381&action=review

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2631
> +    result->effects.reads = B3::HeapRange::top();

by default this watchpoint is Effects::forCall(). Why do we need this?

[Keith Miller]

I think your missing a file.

[Keith Miller]

(In reply to Keith Miller from comment #10)
> I think your missing a file.

ugh, you are*

[Justin Michaud]

Created attachment 456392 [details]
Patch

[Justin Michaud]

Created attachment 456394 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 456394 [details]
Patch

r=me

[Justin Michaud]

Created attachment 456414 [details]
Patch

[Saam Barati]

Comment on attachment 456414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456414&action=review

I think my comment about filter not doing the right thing is legit. But otherwise, LGTM

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:542
> +                if (memoryValue->lastChild() == ptr && filter(memoryValue) 
> +                    && memoryValue->accessWidth() >= m_value->as<MemoryValue>()->accessWidth())

Why is the filter function not handling this?

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:561
> +            if (match && match != m_value && match->accessWidth() >= m_value->as<MemoryValue>()->accessWidth())

ditto

> Source/JavaScriptCore/b3/B3Validate.cpp:573
> +            // Conceptually, if you exit sideways, you should read Top or (AbstractHeapRepository) root. 
> +            // Since we can't easily find out what root is, we settle for this, to make sure things aren't
> +            // horribly wrong at least.

Might be worth making note that this is a pretty strict validation rule, and philosophically isn't actually necessary. But it's serving a practical use case. Like, theoretically, you can sideExit and never read anything. But practically speaking, we never use sideExit like this. So it helps us find bugs.

Hi,
We are affected as well.
Thanks for the patch.

[Saam Barati]

(In reply to ajn35 from comment #17)
> Hi,
> We are affected as well.
> Thanks for the patch.

Do you have a test case you can share?

[Justin Michaud]

Created attachment 456612 [details]
Patch

[Saam Barati]

Comment on attachment 456612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456612&action=review

r=me

> Source/JavaScriptCore/ChangeLog:17
> +

you're missing some blurb here about WasmBoundCheck needed to read top

[Justin Michaud]

Created attachment 456759 [details]
Patch

[EWS]

Committed r292475 (249326@main): <https://commits.webkit.org/249326@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456759 [details].

Created attachment 456925 [details]
TestCaseRepro

Sorry for the delay, please find enclosed a test case that reproduces the bug on Safari iOS 15.4.
Instructions for repro are in README file.

(In reply to Saam Barati from comment #18)
> (In reply to ajn35 from comment #17)
> > Hi,
> > We are affected as well.
> > Thanks for the patch.
> 
> Do you have a test case you can share?

Yes, please see attachment 456925 [details]

[Justin Michaud]

I have confirmed that your test case is fixed! Thanks for including it.

[Saam Barati]

The fix for this is in iOS 15.5 Beta 4. Can folks confirm it resolves issues, and if not, report back to us?

Thanks

[Justin Michaud]

The parent https://bugs.webkit.org/show_bug.cgi?id=237180 has some reports that the issue is resolved by beta 4.

(In reply to Saam Barati from comment #26)
> The fix for this is in iOS 15.5 Beta 4. Can folks confirm it resolves
> issues, and if not, report back to us?
> 
> Thanks

Hi, I confirm it solves our issue.

Thanks

[Brent Fulgham]

This fix shipped with Safari 15.5 (all platforms).