Bug 226660 – [Metal ANGLE] Shaders with reserved metal keywords do not translate, nor do shaders with struct and variable names that are the same except prefixed by an underscore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[John Cunningham]

[Metal ANGLE] Shaders with reserved metal keywords do not translate, nor do shaders with struct and variable names that are the same except prefixed by an underscore

[John Cunningham]

Created attachment 430604 [details]
Patch

[John Cunningham]

Created attachment 430605 [details]
Testcase

[Kenneth Russell]

Note that the WebGL variant of GLSL ES reserves some keyword prefixes:
https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.3

so you could safely rename any of these variables which collide with Metal's reserved ones by prepending for example "_webgl_" to it after the initial parse and validation.

[John Cunningham]

(In reply to Kenneth Russell from comment #3)
> Note that the WebGL variant of GLSL ES reserves some keyword prefixes:
> https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.3
> 
> so you could safely rename any of these variables which collide with Metal's
> reserved ones by prepending for example "_webgl_" to it after the initial
> parse and validation.

Correct, we actually do a pass already of this in RewriteKeywords.cpp, with the keywords defined in GetMSLKeywords, however, in this case something is not being plumbed through correctly and the renaming is not consistent throughout the generated shader.

[Alexey Proskuryakov]

Comment on attachment 430605 [details]
Testcase

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

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword-expected.txt:7
> +TEST COMPLETE

The test needs to be updated for this to be the last line.

[John Cunningham]

Here's the fragment shader converted to MSL:

#define ANGLE_ALWAYS_INLINE __attribute__((always_inline))

ANGLE_ALWAYS_INLINE int ANGLE_int_clamp(int value, int minValue, int maxValue)
{
    return ((value < minValue) ?  minValue : ((value > maxValue) ? maxValue : value));
};

#define ANGLE_SAMPLE_COMPARE_GRADIENT_INDEX 0
#define ANGLE_SAMPLE_COMPARE_LOD_INDEX      1
#define ANGLE_RASTERIZATION_DISCARD_INDEX   2
#define ANGLE_COVERAGE_MASK_ENABLED_INDEX   3

constant bool ANGLE_UseSampleCompareGradient [[function_constant(ANGLE_SAMPLE_COMPARE_GRADIENT_INDEX)]];
constant bool ANGLE_UseSampleCompareLod      [[function_constant(ANGLE_SAMPLE_COMPARE_LOD_INDEX)]];
constant bool ANGLE_RasterizationDiscard     [[function_constant(ANGLE_RASTERIZATION_DISCARD_INDEX)]];
constant bool ANGLE_CoverageMaskEnabled      [[function_constant(ANGLE_COVERAGE_MASK_ENABLED_INDEX)]];

ANGLE_ALWAYS_INLINE void ANGLE_writeSampleMask(const uint mask,
                                               thread uint& gl_SampleMask)
{
    if (ANGLE_CoverageMaskEnabled)
    {
        gl_SampleMask = as_type<int>(mask);
    }
}

#define ANGLE_tensor metal::array
#pragma clang diagnostic ignored "-Wunused-value"
struct ANGLE_s
{
  float ANGLE_1_metal;
  char ANGLE_5_pad[12];
};

struct ANGLE_DepthRangeParams
{
  float ANGLE_near;
  float ANGLE_far;
  float ANGLE_diff;
  float ANGLE_reserved;
};

struct ANGLE_AngleUniforms
{
  metal::float4 ANGLE_viewport;
  metal::float2 ANGLE_halfRenderArea;
  metal::float2 ANGLE_flipXY;
  metal::float2 ANGLE_negFlipXY;
  uint ANGLE_clipDistancesEnabled;
  uint ANGLE_xfbActiveUnpaused;
  uint ANGLE_xfbVerticesPerDraw;
  metal::int4 ANGLE_xfbBufferOffsets;
  metal::uint4 ANGLE_acbBufferOffsets;
  ANGLE_DepthRangeParams ANGLE_depthRange;
  metal::float2x4 ANGLE_preRotation;
  metal::float2x4 ANGLE_fragRotation;
  uint coverageMask;
};

struct ANGLE_FragmentOut
{
  metal::float4 color [[color(0)]];
  uint gl_SampleMask [[sample_mask, function_constant(ANGLE_CoverageMaskEnabled)]];
};

ANGLE_s ANGLE_sb6c;

void ANGLE_helper(ANGLE_s ANGLE_s)
{
}

void ANGLE_0_main(thread ANGLE_FragmentOut & ANGLE_fragmentOut)
{
  ANGLE_fragmentOut.color = metal::float4(0.0f, 0.0f, 0.0f, 0.0f);
  ANGLE_s ANGLE_s = ANGLE_s{0.0f};
  ANGLE_helper(ANGLE_s);
  ANGLE_fragmentOut.color = metal::float4(0.0f, 0.80000001f, 0.0f, 1.0f);
}

fragment ANGLE_FragmentOut main0(constant ANGLE_AngleUniforms & ANGLE_angleUniforms [[buffer(17)]])
{
  ANGLE_FragmentOut ANGLE_fragmentOut;
  {
    ANGLE_0_main(ANGLE_fragmentOut);
    if (ANGLE_CoverageMaskEnabled)
    {
      ANGLE_writeSampleMask(ANGLE_angleUniforms.coverageMask, ANGLE_fragmentOut.gl_SampleMask);
    } else {}
  }
  return ANGLE_fragmentOut;;
}

If I compile this I get the following errors:

ANGLE_s ANGLE_sb6c;
        ^
shader.metal:77:28: error: expected ';' at end of declaration
  ANGLE_s ANGLE_s = ANGLE_s{0.0f};
                           ^
                           ;
shader.metal:77:21: warning: variable 'ANGLE_s' is uninitialized when used within its own initialization [-Wuninitialized]
  ANGLE_s ANGLE_s = ANGLE_s{0.0f};
          ~~~~~~~   ^~~~~~~
1 warning and 2 errors generated.

[Kenneth Russell]

It looks to me like the name mangling scheme in the direct-to-Metal backend has to be changed to make sure that structs' names and variables' names don't collide.

[Radar WebKit Bug Importer]

<rdar://problem/79213149>

[Kyle Piddington]

Created attachment 431506 [details]
Patch

[EWS Watchlist]

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

[John Cunningham]

Comment on attachment 431506 [details]
Patch

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

LGTM. r+ with the expectation that you will fix the nits

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/RewriteKeywords.cpp:137
> +            renamed->setQualifier(type.getQualifier());

is there a constructor that takes a qualifier? otherwise it's okay

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/RewriteKeywords.cpp:158
> +        

not sure what happened here, maybe just formatting

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword-expected.txt:15
> +TEST COMPLETE

Alexey's previous comment suggests this should be at the end.

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword.html:33
> +    	description("Tests that program compiling/linking with a reserved keyword.");

indentation

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword.html:38
> +            var fShaderSouce ='#version 300 es\n precision mediump float;\n out vec4 color;\n struct s { float metal; };\n float helper(s _s) { return _s.metal; }\n void main() { s _s; float a = helper(_s); color = vec4(a,0.8,0,1); }\n'

space after "=", same for each fShaderSouce. Also typo for Souce ;)

[Dean Jackson]

Comment on attachment 431506 [details]
Patch

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

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/RewriteKeywords.cpp:159
>          renamed->setLayoutQualifier(type.getLayoutQualifier());
> +        
>  

Oops.

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword-expected.txt:22
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword in struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for struct name
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for struct name with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword in type with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for struct name  with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for variable name with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for variable name and struct name with global struct

I'm not sure why the test output is coming after the test complete message. I think it's because you're using the webgl test utils but also mixing it with the WebKit utils.

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword.html:22
> +        console.log(gl.getShaderInfoLog(fragmentShader));

I think you left this in by accident.

[Kyle Piddington]

Created attachment 431591 [details]
Patch

[Kyle Piddington]

Created attachment 431597 [details]
Patch

[John Cunningham]

Comment on attachment 431597 [details]
Patch

LGTM

[John Cunningham]

Comment on attachment 431597 [details]
Patch

LGTM

[EWS]

Committed r279016 (238941@main): <https://commits.webkit.org/238941@main>

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

[Kyle Piddington]

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

[Kimmo Kinnunen]

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