Bug 229378

Thanks for the report. Will investigate.

Seems like the case where GLSL reserved words are different to MSL reserved words. probable repro: float or = 1.; IMO the design should be so that MSL output shouldn't have any user-definable names in production builds.

<rdar://problem/82299053>

Created attachment 436355 [details] Patch

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

Comment on attachment 436355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436355&action=review This looks fine assuming it passes tests. Can you please add a test for this under LayoutTests/webgl/pending/conformance/ or conformance2/ , as Kimmo's done recently? That way we'll make sure to upstream it later. Thanks. > Source/ThirdParty/ANGLE/ChangeLog:6 > + Change TranslatorMetalDirect to append '_u_' to all user defined variables. Clean up builtins like samplers and ANGLE structs that were being Could you rewrite this to say something like "prefix all user defined variables with '_u_'"? That'll make the comment match the code.

Comment on attachment 436355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436355&action=review Other code uses kUserDefinedName, so if we're using "_u_" then we could use "_u" instead. However, I think "u_" or "u" would be more appropriate. I'm not 100% sure what the distinction with BuiltIn and AngleInternal is. However, the AngleUniform (driver uniform) struct looks strange since all the names are AngleInternal except stuff modified here. If BuiltIn is "GLSL built-in variable or function", then marking these BuiltIn is perhaps not appropriate and AngleInternal should be used? If BuiltIn is "symbols present but not declarable as GLSL", then BuiltIn is perhaps appropriate? The variable names introduced here start to get a bit wild: a -> _u_a thread -> _u_uthread # (somehow u appears) ANGLE_thread -> _u__1_thread # As per Name replacing ANGLE_ and all that.. _1_thread -> _u__1_thread # Collides with ANGLE_thread if that was the second symbol that was renamed. Would there be some possibility to simplify? The test could introduce similar file to float-constant-expressions.html there you could generate the test cases like so: var cases = ['and', 'thread', 'INT_MAX', 'Pragma', '_Pragma', ..] Then use `cases`to generate the shaders for GLSLConformanceTester.runRenderTests . The fShaderSource can be used to define the source string. > Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/Name.cpp:78 > + out << "_u_" << mRawName; This introduces beginning underscore as well as double underscores, which neither are strictly speaking valid c++. """ Also, all identifiers that contain a double underscore __ in any position and each identifier that begins with an underscore followed by an uppercase letter is always reserved and all identifiers that begin with an underscore are reserved for use as names in the global namespace. See identifiers for more details. """ https://en.cppreference.com/w/cpp/keyword

Created attachment 436445 [details] Patch

Comment on attachment 436445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436445&action=review Kimmo had some detailed comments so let's let him reply again. A couple more small comments. > Source/ThirdParty/ANGLE/ChangeLog:6 > + Change TranslatorMetalDirect to append '_u' to all user defined variables. Clean up builtins like samplers and ANGLE structs that were being "Change TranslatorMetalDirect to prefix all user defined variables with '_u'"? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:20 > + constexpr char kUserDefinedNamePrefix[] = "_u"; Does this have to be equivalent to some other constant somewhere else in the code? If so could you please add a comment pointing to it, or ideally find a place where it can be shared? > Tools/MiniBrowser/MiniBrowser.xcodeproj/xcshareddata/xcschemes/MiniBrowser.xcscheme:37 > + launchStyle = "1" Was this change intentional?

shader-with-non-reserved-words.html could be the test to copy to webgl/pending add the test keywords..

Created attachment 436691 [details] Patch

Created attachment 436696 [details] Patch

Currently shader-with-reserved-words.html tests following possible implementations: - d3d - various versions of GLSL So it would be logical to add tests to that. It would then test also: - MSL To edit the test case, currently my idea of it is: 1) fork the test 2) modify the test 3) upstream the test 4) rebase the test suite 5) delete the forked test So to do steps 1+2, you would do something like: ditto LayoutTests/webgl/2.0.y/conformance/glsl/misc/shader-with-reserved-words.html LayoutTests/webgl/pending/conformance/glsl/misc/ vim LayoutTests/webgl/pending/conformance/glsl/misc/shader-with-reserved-words.html # For each line containing path ../../../, remove one "../" ditto LayoutTests/webgl/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words.html LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/ vim LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words.html # Add your test cases as new cases. For my entertainment, maybe you could add few C++ symbols that are obviously not part of MSL filter lists, but which would be problematic in naive implementation. Examples of the latter are for example "INT_MAX" and "_Pragma", "__attribute__", "__file__", "ANGLE_1", . (E.g. We know we have a filter list in c++, and if a word exists in that list, it gets rewritten and we know this. It's good to test that filter list but we know it's a no-op and it was already working. The primary test objective would be the items obviously not in the filter list.)

Comment on attachment 436696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436696&action=review > Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp:71 > +constexpr Name kFlippedPointCoordName("flippedPointCoord", SymbolType::BuiltIn); It's still unclear why something goes UserDefined -> BuiltIn and something goes UserDefined -> AngleInternal Most of these, I would imagine, are still "AngleInternal", e.g. they don't exist in GLSL (if the interpretation of BuiltIn is that BuiltIns are in GLSL). > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:261 > + << "ANGLE_vertexOut." << kUserDefinedNamePrefix << varying.name; So this one seems to indicate that we don't put the prefix in the correct place, in the `varying.mappedName` ? Not sure if it should be blocking the review, but if you know where the mappedName is correctly populated, might as well use the intended logic? varying.name == the name in the user program varying.mappedName == the name in the output program Vulkan is using the mappedName > LayoutTests/ChangeLog:14 > + (error): Long list of copied test functions isn't useful, so typically these are removed. > LayoutTests/webgl/pending/conformance/glsl/misc/shader-with-metal-reserved-words-expected.txt:1 > +This test runs the WebGL Test listed below in an iframe and reports PASS or FAIL. Your commit is missing the test driver that generates this file. > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-metal-reserved-words.html:3 > +/* So this could be just the shader-with-reserved-words.html. > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/resources/webgl-test-utils.js:4 > +** Permission is hereby granted, free of charge, to any person obtaining a This file is already in the pending/ directory

Created attachment 438030 [details] Patch

I updated the patch to contain the test and the test runner. The test fails when run with WebGL2, the code needs a bit of polish still.

Comment on attachment 438030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438030&action=review Once this is passing tests, looks good to me. A couple of small questions/comments. r+ > Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp:71 > +constexpr Name kFlippedFragCoordName("flippedFragCoord", SymbolType::BuiltIn); Curious why these two synthetic names use BuiltIn rather than AngleInternal. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:26 > + // The length of a string defined as a char array is the size of the array minus 1 (the The indentation looks strange here. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:264 > + << "ANGLE_vertexOut." << kUserDefinedNamePrefix << varying.name; Would it be possible to match the reflow of the earlier code to make the single kUserDefinedNamePrefix addition more clear? > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:4 > +** Copyright (c) 2012 The Khronos Group Inc. 2021 The copyright block was shortened recently. Could you take the one from a newer test?

I am seeing what I believe is the same issue for this library: https://github.com/squarefeet/ShaderParticleEngine Demo here: http://squarefeet.github.io/ShaderParticleEngine/examples/basic.html I can confirm both the Shader Particle Engine demo and the shadertoy link work on the latest Safari Technology Preview (Release 132, Safari 15.4, WebKit 16613.1.1.5) on macOS 11.5.2 but fail on iOS 15.0 and 15.1 dev beta. Is this patch currently included in 15.1 dev beta?

(In reply to rigel from comment #18) > I am seeing what I believe is the same issue for this library: > https://github.com/squarefeet/ShaderParticleEngine > > Demo here: > http://squarefeet.github.io/ShaderParticleEngine/examples/basic.html > > I can confirm both the Shader Particle Engine demo and the shadertoy link > work on the latest Safari Technology Preview (Release 132, Safari 15.4, > WebKit 16613.1.1.5) on macOS 11.5.2 but fail on iOS 15.0 and 15.1 dev beta. > > Is this patch currently included in 15.1 dev beta? This patch isn't currently included. We'll have it in shortly!

Created attachment 439225 [details] Patch

Created attachment 440129 [details] Patch

Comment on attachment 440129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440129&action=review > Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp:-23 > -#include "compiler/translator/TranslatorMetalDirect/RewriteKeywords.h" Is this file still used now? This seems to be the only file that uses the now-removed function. > Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/Name.cpp:72 > + out << mRawName; include ASSERT() here also? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:28 > + return N - 1; nit: inconsistent tab spacing.

Comment on attachment 440129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440129&action=review > Source/ThirdParty/ANGLE/ChangeLog:9 > + Change TranslatorMetalDirect to prefix '_u' to all user defined variables. I think you could have used "_webgl_" as a prefix since that is explicitly forbidden in WebGL content. https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.17 > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:175 > +var badWords = [ > + { words: MetalWords } > +]; Since there is only one entry in this array, maybe just declare const badWords = [ ... all the metal names... ]; and then... > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:179 > +var checkedWords = {}; Do you need this variable? I think if you iterate through all the elements in the array you'll never hit the same word twice. > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:181 > +var src = []; I would call this sources. > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:185 > + src.push({vsrc: vsrc, fsrc: fsrc}); You can do src.push({vsrc, fsrc}) (if the variable holding the value has the same name as the key, you only have to give the name - it will become name: name automatically) > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:188 > +var badWordNdx = 0; I don't think you change this, so just have badWords as the array. > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:204 > + var list = badWords[badWordNdx].words; > + if (listNdx >= list.length) { > + ++badWordNdx; > + if (badWordNdx >= badWords.length) { > + finishTest(); > + return; > + } > + listNdx = 0; > + list = badWords[badWordNdx].words; > + } > + testWord(list[listNdx]); > + ++listNdx; > + setTimeout(testNextWord, 0); This would become: function testNextWord() { if (listNdx >= badWords.length) { finishTest(); return; } testWord(badWords[listNdx++]) setTimeout(testNextWord, 0); } > LayoutTests/webgl/pending/resources/webgl_test_files/conformance/glsl/misc/shader-with-reserved-words-2.html:218 > + for (var ii = 0; ii < src.length; ++ii) { > + var vs = src[ii].vsrc.replace(/\$replaceMe/g, word); > + var fs = src[ii].fsrc.replace(/\$replaceMe/g, word); Just FYI, you can avoid the ii variable with: sources.forEach(src => { const vs = src.vsrc..... ... });

> I think you could have used "_webgl_" as a prefix since that is explicitly > forbidden in WebGL content. > https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.17 The problem is the second underscore. Double underscores are reserved for macros, so any user variables like _foo will turn into _webgl__foo This is one of the main reasons the prefix is _u, instead of _u_.

Created attachment 440435 [details] Patch for landing

Committed r283667 (242607@main): <https://commits.webkit.org/242607@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440435 [details].

*** Bug 232169 has been marked as a duplicate of this bug. ***

*** Bug 233405 has been marked as a duplicate of this bug. ***