Skip to content

Commit f8bd44c

Browse files
kpiddingtonwebkit-commit-queue
kpiddington
authored and
webkit-commit-queue
committed
Crash in webgl/2.0.y/conformance/glsl/misc/uninitialized-local-global-variables.html ANGLE+METAL
https://bugs.webkit.org/show_bug.cgi?id=223923 Anonymous structs require a name in MSL, add a default name ANGLE__unnamed$id to any structs. Also add a unit test to ensure this works. Patch by Kyle Piddington <kpiddington@apple.com> on 2021-04-12 Reviewed by Kenneth Russell. * src/compiler/translator/TranslatorMetalDirect.cpp: (sh::TranslatorMetalDirect::translateImpl): * src/compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.cpp: (sh::Separator::Separator): (sh::SeparateCompoundStructDeclarations): * src/compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.h: * src/tests/BUILD.gn: * src/tests/angle_unittests.gni: * src/tests/compiler_tests/MSLOutput_Test.cpp: Canonical link: https://commits.webkit.org/236402@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@275832 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 5701596 commit f8bd44c

7 files changed

Lines changed: 115 additions & 27 deletions

File tree

‎Source/ThirdParty/ANGLE/ChangeLog‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,22 @@
1+
2021-04-12 Kyle Piddington <kpiddington@apple.com>
2+
3+
Crash in webgl/2.0.y/conformance/glsl/misc/uninitialized-local-global-variables.html ANGLE+METAL
4+
https://bugs.webkit.org/show_bug.cgi?id=223923
5+
6+
Anonymous structs require a name in MSL, add a default name ANGLE__unnamed$id to any structs.
7+
Also add a unit test to ensure this works.
8+
Reviewed by Kenneth Russell.
9+
10+
* src/compiler/translator/TranslatorMetalDirect.cpp:
11+
(sh::TranslatorMetalDirect::translateImpl):
12+
* src/compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.cpp:
13+
(sh::Separator::Separator):
14+
(sh::SeparateCompoundStructDeclarations):
15+
* src/compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.h:
16+
* src/tests/BUILD.gn:
17+
* src/tests/angle_unittests.gni:
18+
* src/tests/compiler_tests/MSLOutput_Test.cpp:
19+
120
2021-04-08 Kyle Piddington <kpiddington@apple.com>
221

322
[Metal-ANGLE] Support GPU power preferences, select low-power GPU by default.

‎Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1435,7 +1435,7 @@ bool TranslatorMetalDirect::translateImpl(TIntermBlock &root, ShCompileOptions c
14351435
return false;
14361436
}
14371437

1438-
if (!SeparateCompoundStructDeclarations(*this, root))
1438+
if (!SeparateCompoundStructDeclarations(*this, idGen, root))
14391439
{
14401440
return false;
14411441
}

‎Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.cpp‎

Lines changed: 43 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.h"
1010
#include "compiler/translator/tree_ops/SeparateDeclarations.h"
1111
#include "compiler/translator/tree_util/IntermTraverse.h"
12+
#include "compiler/translator/TranslatorMetalDirect/AstHelpers.h"
1213

1314
using namespace sh;
1415

@@ -21,37 +22,55 @@ class Separator : public TIntermTraverser
2122
{
2223
public:
2324
std::unordered_map<int, TIntermSymbol *> replacementMap;
24-
Separator(TSymbolTable &symbolTable) : TIntermTraverser(false, false, true, &symbolTable) {}
25-
25+
Separator(TSymbolTable &symbolTable, IdGen &idGen) : TIntermTraverser(false, false, true, &symbolTable),
26+
mIdGen(idGen)
27+
{}
28+
IdGen &mIdGen;
2629
bool visitDeclaration(Visit, TIntermDeclaration *declNode) override
2730
{
2831
ASSERT(declNode->getChildCount() == 1);
29-
TIntermNode &node = *declNode->getChildNode(0);
32+
Declaration declaration = ViewDeclaration(*declNode);
3033

31-
if (TIntermSymbol *symbolNode = node.getAsSymbolNode())
34+
const TVariable &var = declaration.symbol.variable();
35+
const TType &type = var.getType();
36+
const SymbolType symbolType = var.symbolType();
37+
if (type.isStructSpecifier() && symbolType != SymbolType::Empty)
3238
{
33-
const TVariable &var = symbolNode->variable();
34-
const TType &type = var.getType();
35-
const SymbolType symbolType = var.symbolType();
36-
if (type.isStructSpecifier() && symbolType != SymbolType::Empty)
39+
const TStructure *structure = type.getStruct();
40+
TVariable *structVar = nullptr;
41+
TType * instanceType = nullptr;
42+
//Name unnamed inline structs
43+
if(structure->symbolType() == SymbolType::Empty)
44+
{
45+
const TStructure * structDefn = new TStructure(mSymbolTable, mIdGen.createNewName("__unnamed").rawName(), &(structure->fields()) , SymbolType::AngleInternal);
46+
structVar = new TVariable(mSymbolTable, ImmutableString(""),
47+
new TType(structDefn, true), SymbolType::Empty);
48+
instanceType = new TType(structDefn, false);
49+
}
50+
else
3751
{
38-
const TStructure *structure = type.getStruct();
39-
auto *structVar = new TVariable(mSymbolTable, ImmutableString(""),
52+
structVar = new TVariable(mSymbolTable, ImmutableString(""),
4053
new TType(structure, true), SymbolType::Empty);
54+
instanceType = new TType(structure, false);
55+
}
56+
instanceType->setQualifier(type.getQualifier());
57+
auto *instanceVar = new TVariable(mSymbolTable, var.name(), instanceType,
58+
symbolType, var.extension());
4159

42-
auto *instanceType = new TType(structure, false);
43-
instanceType->setQualifier(type.getQualifier());
44-
auto *instanceVar = new TVariable(mSymbolTable, var.name(), instanceType,
45-
symbolType, var.extension());
46-
47-
TIntermSequence replacements;
48-
TIntermSymbol * instanceSymbol = new TIntermSymbol(instanceVar);
49-
replacements.push_back(new TIntermSymbol(structVar));
50-
replacements.push_back(instanceSymbol);
51-
replacementMap[symbolNode->uniqueId().get()] = instanceSymbol;
52-
mMultiReplacements.push_back(
53-
NodeReplaceWithMultipleEntry(declNode, symbolNode, std::move(replacements)));
60+
TIntermSequence replacements;
61+
replacements.push_back(new TIntermSymbol(structVar));
62+
63+
TIntermSymbol * instanceSymbol = new TIntermSymbol(instanceVar);
64+
TIntermNode * instanceReplacement = instanceSymbol;
65+
if(declaration.initExpr)
66+
{
67+
instanceReplacement = new TIntermBinary(EOpInitialize, instanceSymbol, declaration.initExpr);
5468
}
69+
replacements.push_back(instanceReplacement);
70+
71+
replacementMap[declaration.symbol.uniqueId().get()] = instanceSymbol;
72+
mMultiReplacements.push_back(
73+
NodeReplaceWithMultipleEntry(declNode, declNode->getChildNode(0), std::move(replacements)));
5574
}
5675

5776
return false;
@@ -71,9 +90,9 @@ class Separator : public TIntermTraverser
7190

7291
////////////////////////////////////////////////////////////////////////////////
7392

74-
bool sh::SeparateCompoundStructDeclarations(TCompiler &compiler, TIntermBlock &root)
93+
bool sh::SeparateCompoundStructDeclarations(TCompiler &compiler, IdGen &idGen, TIntermBlock &root)
7594
{
76-
Separator separator(compiler.getSymbolTable());
95+
Separator separator(compiler.getSymbolTable(), idGen);
7796
root.traverse(&separator);
7897
if (!separator.updateTree(&compiler, &root))
7998
{

‎Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/SeparateCompoundStructDeclarations.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@
99

1010
#include "common/angleutils.h"
1111
#include "compiler/translator/Compiler.h"
12-
12+
#include "compiler/translator/TranslatorMetalDirect/IdGen.h"
1313
namespace sh
1414
{
1515

1616
// Example:
1717
// struct Foo { int x; } foo;
1818
// Becomes:
1919
// struct Foo {int x; }; Foo foo;
20-
ANGLE_NO_DISCARD bool SeparateCompoundStructDeclarations(TCompiler &compiler, TIntermBlock &root);
20+
ANGLE_NO_DISCARD bool SeparateCompoundStructDeclarations(TCompiler &compiler, IdGen &idGen, TIntermBlock &root);
2121

2222
} // namespace sh
2323

‎Source/ThirdParty/ANGLE/src/tests/BUILD.gn‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ angle_test("angle_unittests") {
117117
sources += angle_unittests_hlsl_sources
118118
defines = [ "ANGLE_ENABLE_HLSL" ]
119119
}
120+
121+
if (angle_enable_metal) {
122+
sources += angle_unittests_msl_sources
123+
defines += [ "ANGLE_ENABLE_METAL", "ANGLE_ENABLE_METAL_SPIRV" ]
124+
}
120125

121126
deps = [
122127
":angle_test_expectations",

‎Source/ThirdParty/ANGLE/src/tests/angle_unittests.gni‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,8 @@ if (is_android) {
154154
[ "../tests/compiler_tests/ImmutableString_test_autogen.cpp" ]
155155
}
156156

157+
angle_unittests_msl_sources = [ "../tests/compiler_tests/MSLOutput_test.cpp" ]
158+
157159
if (!is_android && !is_fuchsia) {
158160
angle_unittests_sources +=
159161
[ "../tests/test_utils/runner/TestSuite_unittest.cpp" ]

‎Source/ThirdParty/ANGLE/src/tests/compiler_tests/MSLOutput_test.cpp‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
//
2+
// Copyright 2017 The ANGLE Project Authors. All rights reserved.
3+
// Use of this source code is governed by a BSD-style license that can be
4+
// found in the LICENSE file.
5+
//
6+
// MSLOutput_test.cpp:
7+
// Tests for MSL output.
8+
//
9+
10+
#include <regex>
11+
#include "GLSLANG/ShaderLang.h"
12+
#include "angle_gl.h"
13+
#include "gtest/gtest.h"
14+
#include "tests/test_utils/compiler_test.h"
15+
16+
using namespace sh;
17+
18+
class MSLVertexOutputTest : public MatchOutputCodeTest
19+
{
20+
public:
21+
MSLVertexOutputTest() : MatchOutputCodeTest(GL_VERTEX_SHADER, 0, SH_MSL_METAL_OUTPUT) {}
22+
};
23+
24+
class MSLOutputTest : public MatchOutputCodeTest
25+
{
26+
public:
27+
MSLOutputTest() : MatchOutputCodeTest(GL_FRAGMENT_SHADER, 0, SH_MSL_METAL_OUTPUT) {}
28+
};
29+
30+
TEST_F(MSLOutputTest, AnonymousStruct)
31+
{
32+
const std::string &shaderString =
33+
R"(
34+
precision mediump float;
35+
struct { vec4 v; } anonStruct;
36+
void main() {
37+
anonStruct.v = vec4(0.0,1.0,0.0,1.0);
38+
gl_FragColor = anonStruct.v;
39+
})";
40+
compile(shaderString, SH_VARIABLES);
41+
ASSERT_TRUE(foundInCode(SH_MSL_METAL_OUTPUT, "__unnamed"));
42+
}
43+

0 commit comments

Comments
 (0)