Bug 227449 – ANGLE Metal primitive restart range computation could index with size_t

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Kimmo Kinnunen]

ANGLE Metal primitive restart range computation could index with size_t

It'd be simpler to understand that the index does not overflow. The index limit is size_t.

[Kimmo Kinnunen]

Created attachment 432393 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Kimmo Kinnunen]

Created attachment 432394 [details]
Patch

[Kenneth Russell]

Comment on attachment 432394 [details]
Patch

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

This is certainly easier to understand. r+ with a couple of small questions.

I defer to kpiddington's review if there's any further feedback.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:421
> +    for (size_t i = 0; i < numIndices; i++)

Would it be better to use "++i" here for consistency with the inner loop?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:430
> +        } while (i < numIndices && bufferData[i] == restartMarker);

Out of curiosity do you want to leave the restart indices in these ranges or elide them?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:445
> +                mRestartIndices = calculateRestartRanges<uint8_t>(ctx, getCurrentBuffer());

How hot is this routine? If it's called very often then maybe there is an advantage to continuing to pass mRestartIndices as an argument to reduce copying (or is return value optimization expected to handle this)?

[Kimmo Kinnunen]

Created attachment 432457 [details]
Patch for landing

[Kimmo Kinnunen]

Comment on attachment 432394 [details]
Patch

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

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:430
>> +        } while (i < numIndices && bufferData[i] == restartMarker);
> 
> Out of curiosity do you want to leave the restart indices in these ranges or elide them?

restart indices should be in the ranges.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:445
>> +                mRestartIndices = calculateRestartRanges<uint8_t>(ctx, getCurrentBuffer());
> 
> How hot is this routine? If it's called very often then maybe there is an advantage to continuing to pass mRestartIndices as an argument to reduce copying (or is return value optimization expected to handle this)?

RVO is expected.
It's not hot, it's called during draw, but only once after the buffer is modified.

[EWS]

Committed r279373 (239239@main): <https://commits.webkit.org/239239@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/79913301>