Bug 229378

Summary: Shadertoy "truchet district" fails to compile with error: Internal error compiling shader with Metal backend"
Product: WebKit Reporter: Lionel Lemarie <bugs.webkit>
Component: WebGLAssignee: Kyle Piddington <kpiddington>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, garettbass, gman, jonlee, kbr, kkinnunen, kondapallykalyan, kpiddington, rigel, spreadpando, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac (Apple Silicon)   
OS: macOS 11   
Bug Depends on:    
Bug Blocks: 222812, 231682    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Lionel Lemarie 2021-08-21 18:28:42 PDT
The ShaderToy “truchet district” at https://www.shadertoy.com/view/ss3GRN fails to compile with error:
	Unknown error: Internal error compiling shader with Metal backend.
	Unknown error: Please submit this shader, or website as a bug to https://bugs.webkit.org

Repro: 
- Go to https://www.shadertoy.com/view/ss3GRN
- The shader compiles automatically and displays the error message.
Comment 1 Kimmo Kinnunen 2021-08-23 10:28:54 PDT
Thanks for the report. Will investigate.
Comment 2 Kimmo Kinnunen 2021-08-23 10:45:46 PDT
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.
Comment 3 Radar WebKit Bug Importer 2021-08-24 10:40:24 PDT
<rdar://problem/82299053>
Comment 4 Kyle Piddington 2021-08-24 17:50:41 PDT
Created attachment 436355 [details]
Patch
Comment 5 EWS Watchlist 2021-08-24 17:51:28 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 6 Kenneth Russell 2021-08-24 18:22:11 PDT
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 7 Kimmo Kinnunen 2021-08-25 06:41:40 PDT
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
Comment 8 Kyle Piddington 2021-08-25 17:23:55 PDT
Created attachment 436445 [details]
Patch
Comment 9 Kenneth Russell 2021-08-25 18:23:28 PDT
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?
Comment 10 Kimmo Kinnunen 2021-08-26 10:58:34 PDT
shader-with-non-reserved-words.html could be the test to copy to webgl/pending add the test keywords..
Comment 11 Kyle Piddington 2021-08-27 16:41:19 PDT
Created attachment 436691 [details]
Patch
Comment 12 Kyle Piddington 2021-08-27 17:31:00 PDT
Created attachment 436696 [details]
Patch
Comment 13 Kimmo Kinnunen 2021-08-30 05:10:20 PDT
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 14 Kimmo Kinnunen 2021-08-30 05:33:08 PDT
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
Comment 15 Kimmo Kinnunen 2021-09-13 05:15:20 PDT
Created attachment 438030 [details]
Patch
Comment 16 Kimmo Kinnunen 2021-09-13 05:16:48 PDT
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 17 Kenneth Russell 2021-09-13 15:46:28 PDT
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?
Comment 18 rigel 2021-09-21 17:11:58 PDT
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?
Comment 19 Kyle Piddington 2021-09-23 14:48:07 PDT
(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!
Comment 20 Kyle Piddington 2021-09-24 18:30:09 PDT
Created attachment 439225 [details]
Patch
Comment 21 Kyle Piddington 2021-10-04 17:43:39 PDT
Created attachment 440129 [details]
Patch
Comment 22 Jon Lee 2021-10-04 18:38:48 PDT
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 23 Dean Jackson 2021-10-04 19:08:26 PDT
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.....
  ...
});
Comment 24 Kyle Piddington 2021-10-05 12:51:14 PDT
> 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_.
Comment 25 Kyle Piddington 2021-10-06 13:40:42 PDT
Created attachment 440435 [details]
Patch for landing
Comment 26 EWS 2021-10-06 14:45:25 PDT
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].
Comment 27 Kyle Piddington 2021-10-25 13:13:56 PDT
*** Bug 232169 has been marked as a duplicate of this bug. ***
Comment 28 Kimmo Kinnunen 2021-11-22 23:58:37 PST
*** Bug 233405 has been marked as a duplicate of this bug. ***