Bug 226468 - ANGLE Metal translator should clamp array access in MSL
Summary: ANGLE Metal translator should clamp array access in MSL
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-31 06:07 PDT by Kimmo Kinnunen
Modified: 2024-01-22 23:52 PST (History)
10 users (show)

See Also:

Attachments
Patch (2.01 KB, patch)
2021-05-31 06:10 PDT, Kimmo Kinnunen 		dino: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-05-31 06:07:12 PDT 
ANGLE Metal translator should clamp array access in MSL
Comment 1 Kirtikumar Anandrao Ramchandani 2021-05-31 06:09:56 PDT 
Looks like this issue was created after that, yes?
Comment 2 Kimmo Kinnunen 2021-05-31 06:10:17 PDT 
Created attachment 430200 [details]
Patch
Comment 3 EWS Watchlist 2021-05-31 06:11:25 PDT 
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Kimmo Kinnunen 2021-05-31 06:24:59 PDT 
(In reply to Kirtikumar Anandrao Ramchandani from comment #1)
> Looks like this issue was created after that, yes?

This bug report was created based on bug 226397 and after that, yes.
Comment 5 Dean Jackson 2021-05-31 11:02:10 PDT 
Can we write a test for this?
Comment 6 Kirtikumar Anandrao Ramchandani 2021-05-31 11:03:22 PDT 
We can write a test case to reach that particular piece of code, yes.
Comment 7 Kimmo Kinnunen 2021-05-31 23:49:48 PDT 
(In reply to Dean Jackson from comment #5)
> Can we write a test for this?

At the moment it's a bit cumbersome.
For manual inspection of shader text output we'd either need to:
a) add a text-dumping translator tester to ANGLE 
b) implement (the long overdue) per-gpu filtering possibility to RWT and use the layout test to dump the GL/MTL shader output to expected files

For draw inspection-based test it's hard to do since we don't know what to expect in the failure case. In practice we can't know that a success would indicate lack of bug.

This is tested by conformance/uniforms/out-of-bounds-uniform-array-access.html but it's unclear to what extent we can observe the bug. Obviously we cannot, as the test did not trigger.

I'll check if we can later turn on validating layer for Metal on Debug builds, but I doubt it will detect this either.

I'll put testing this in the "back burner" and take the liberty to merge without, as we have bigger issues in testing than this.
Comment 8 Kimmo Kinnunen 2021-05-31 23:56:12 PDT 
> b) implement (the long overdue) per-gpu filtering possibility to RWT and use the layout test to dump the GL/MTL shader output to expected files


Except, maybe it's doable since this the "MSL or GLSL or HLSL" decision is per platform in WebKit at the moment. I'll try to write the test.
Comment 9 Radar WebKit Bug Importer 2021-06-07 06:08:18 PDT 
<rdar://problem/78944685>
Comment 10 Ahmad Saleem 2022-09-25 12:42:37 PDT 
It seems this r+ patch didn't landed. Is it something still needed or being worked on? Thanks!
Comment 11 Kimmo Kinnunen 2022-10-04 03:47:55 PDT 
Thanks for the heads. I think I need to upstream this.
Comment 12 Kimmo Kinnunen 2024-01-22 23:51:52 PST 
Already fixed in  GenMetalTraverser::visitBinary(Visit, TIntermBinary *binaryNode)