[webkit-reviews] review granted: [Bug 198414] [WHLSL] Implement arrays and MakeArrayReference : [Attachment 372859] patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 198414: [WHLSL] Implement arrays and MakeArrayReference
https://bugs.webkit.org/show_bug.cgi?id=198414

Attachment 372859: patch

https://bugs.webkit.org/attachment.cgi?id=372859&action=review




--- Comment #4 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 372859
  --> https://bugs.webkit.org/attachment.cgi?id=372859
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:635
> +	   m_stringBuilder.append(makeString(mangledTypeName, ' ',
variableName, " = { ", takeLastValue(), ", 1 };\n"));

I know you didn’t change this, but shouldn’t this be 0 if the original pointer
is null?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:-178
> -	       stringBuilder.append(makeString("    return ",
arrayParameterType.numElements(), "u;\n"));

The “u” suffix was to make it an unsigned, in case it’s > 2B

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:343
> +    return &*iterator;

Isn’t returning a pointer pointing into the array a use after free if something
else gets added to the array before you dereference the pointer? I think that’s
why I made it an index instead of a pointer.

> LayoutTests/webgpu/whlsl-huge-array.html:97
> +    shouldBe("resultsInt32Array[0]", "1");

Can we update these tests to use the pattern that caides tests to pass if the
machine doesn’t support Meta? I’ve migrated all the existing tests.