[WHLSL] Don't generate empty comma expressions for bare ';'
authorrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Aug 2019 01:05:27 +0000 (01:05 +0000)
committerrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Aug 2019 01:05:27 +0000 (01:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200681

Reviewed by Myles C. Maxfield.

Currently we emit a comma expression with no sub-expression for bare ';', as well as for the initialization of for loops with no initializers.
This crashes the Checker, as it tries to access the last sub-expression of comma expressions.
Instead we should generate an empty statement block for that case.

This problem was found (and originally fixed before the commit was reverted) in https://bugs.webkit.org/show_bug.cgi?id=199726.
I am just isolating the fix here for easier review and debugging.

New test: LayoutTests/webgpu/whlsl/for-loop.html

* Modules/webgpu/WHLSL/AST/WHLSLForLoop.h:
* Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:
(WebCore::WHLSL::Metal::FunctionDefinitionWriter::visit):
* Modules/webgpu/WHLSL/WHLSLASTDumper.cpp:
(WebCore::WHLSL::ASTDumper::visit):
* Modules/webgpu/WHLSL/WHLSLChecker.cpp:
(WebCore::WHLSL::Checker::visit):
* Modules/webgpu/WHLSL/WHLSLParser.cpp:
(WebCore::WHLSL::Parser::parseForLoop):
(WebCore::WHLSL::Parser::parseStatement):
(WebCore::WHLSL::Parser::parseEffectfulExpression):
* Modules/webgpu/WHLSL/WHLSLParser.h:
* Modules/webgpu/WHLSL/WHLSLVisitor.cpp:
(WebCore::WHLSL::Visitor::visit):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248650 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLForLoop.h
Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp
Source/WebCore/Modules/webgpu/WHLSL/WHLSLASTDumper.cpp
Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp
Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp
Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.h
Source/WebCore/Modules/webgpu/WHLSL/WHLSLVisitor.cpp

index 71d5cc2..a5f5896 100644 (file)
@@ -1,3 +1,34 @@
+2019-08-13  Robin Morisset  <rmorisset@apple.com>
+
+        [WHLSL] Don't generate empty comma expressions for bare ';'
+        https://bugs.webkit.org/show_bug.cgi?id=200681
+
+        Reviewed by Myles C. Maxfield.
+
+        Currently we emit a comma expression with no sub-expression for bare ';', as well as for the initialization of for loops with no initializers.
+        This crashes the Checker, as it tries to access the last sub-expression of comma expressions.
+        Instead we should generate an empty statement block for that case.
+
+        This problem was found (and originally fixed before the commit was reverted) in https://bugs.webkit.org/show_bug.cgi?id=199726.
+        I am just isolating the fix here for easier review and debugging.
+
+        New test: LayoutTests/webgpu/whlsl/for-loop.html
+
+        * Modules/webgpu/WHLSL/AST/WHLSLForLoop.h:
+        * Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:
+        (WebCore::WHLSL::Metal::FunctionDefinitionWriter::visit):
+        * Modules/webgpu/WHLSL/WHLSLASTDumper.cpp:
+        (WebCore::WHLSL::ASTDumper::visit):
+        * Modules/webgpu/WHLSL/WHLSLChecker.cpp:
+        (WebCore::WHLSL::Checker::visit):
+        * Modules/webgpu/WHLSL/WHLSLParser.cpp:
+        (WebCore::WHLSL::Parser::parseForLoop):
+        (WebCore::WHLSL::Parser::parseStatement):
+        (WebCore::WHLSL::Parser::parseEffectfulExpression):
+        * Modules/webgpu/WHLSL/WHLSLParser.h:
+        * Modules/webgpu/WHLSL/WHLSLVisitor.cpp:
+        (WebCore::WHLSL::Visitor::visit):
+
 2019-08-13  Daniel Bates  <dabates@apple.com>
 
         Focus rings are black
index 0a7e6f8..3e7057d 100644 (file)
@@ -45,7 +45,7 @@ namespace AST {
 class ForLoop final : public Statement {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    ForLoop(CodeLocation location, Variant<UniqueRef<Statement>, UniqueRef<Expression>>&& initialization, std::unique_ptr<Expression>&& condition, std::unique_ptr<Expression>&& increment, UniqueRef<Statement>&& body)
+    ForLoop(CodeLocation location, UniqueRef<Statement>&& initialization, std::unique_ptr<Expression>&& condition, std::unique_ptr<Expression>&& increment, UniqueRef<Statement>&& body)
         : Statement(location, Kind::ForLoop)
         , m_initialization(WTFMove(initialization))
         , m_condition(WTFMove(condition))
@@ -59,13 +59,13 @@ public:
     ForLoop(const ForLoop&) = delete;
     ForLoop(ForLoop&&) = default;
 
-    Variant<UniqueRef<Statement>, UniqueRef<Expression>>& initialization() { return m_initialization; }
+    UniqueRef<Statement>& initialization() { return m_initialization; }
     Expression* condition() { return m_condition.get(); }
     Expression* increment() { return m_increment.get(); }
     Statement& body() { return m_body; }
 
 private:
-    Variant<UniqueRef<Statement>, UniqueRef<Expression>> m_initialization;
+    UniqueRef<Statement> m_initialization;
     std::unique_ptr<Expression> m_condition;
     std::unique_ptr<Expression> m_increment;
     UniqueRef<Statement> m_body;
index ca346e3..82548b8 100644 (file)
@@ -356,14 +356,7 @@ void FunctionDefinitionWriter::visit(AST::WhileLoop& whileLoop)
 void FunctionDefinitionWriter::visit(AST::ForLoop& forLoop)
 {
     m_stringBuilder.append("{\n");
-
-    WTF::visit(WTF::makeVisitor([&](AST::Statement& statement) {
-        checkErrorAndVisit(statement);
-    }, [&](UniqueRef<AST::Expression>& expression) {
-        checkErrorAndVisit(expression);
-        takeLastValue(); // We don't need to do anything with the result.
-    }), forLoop.initialization());
-
+    checkErrorAndVisit(forLoop.initialization());
     emitLoop(LoopConditionLocation::BeforeBody, forLoop.condition(), forLoop.increment(), forLoop.body());
     m_stringBuilder.append("}\n");
 }
index d20128a..396f84f 100644 (file)
@@ -409,11 +409,7 @@ void ASTDumper::visit(AST::DoWhileLoop& doWhileLoop)
 void ASTDumper::visit(AST::ForLoop& forLoop)
 {
     m_out.print("for (");
-    WTF::visit(WTF::makeVisitor([&](UniqueRef<AST::Statement>& statement) {
-        visit(statement);
-    }, [&](UniqueRef<AST::Expression>& expression) {
-        visit(expression);
-    }), forLoop.initialization());
+    visit(forLoop.initialization());
     m_out.print("; ");
     if (forLoop.condition())
         visit(*forLoop.condition());
index 333bdf6..77da48e 100644 (file)
@@ -1443,11 +1443,7 @@ void Checker::visit(AST::DoWhileLoop& doWhileLoop)
 
 void Checker::visit(AST::ForLoop& forLoop)
 {
-    WTF::visit(WTF::makeVisitor([&](UniqueRef<AST::Statement>& statement) {
-        checkErrorAndVisit(statement);
-    }, [&](UniqueRef<AST::Expression>& expression) {
-        checkErrorAndVisit(expression);
-    }), forLoop.initialization());
+    checkErrorAndVisit(forLoop.initialization());
     if (hasError())
         return;
     if (forLoop.condition()) {
index 7cab6c9..016114a 100644 (file)
@@ -1163,7 +1163,7 @@ auto Parser::parseForLoop() -> Expected<AST::ForLoop, Error>
     CONSUME_TYPE(origin, For);
     CONSUME_TYPE(leftParenthesis, LeftParenthesis);
 
-    auto parseRemainder = [&](Variant<UniqueRef<AST::Statement>, UniqueRef<AST::Expression>>&& initialization) -> Expected<AST::ForLoop, Error> {
+    auto parseRemainder = [&](UniqueRef<AST::Statement>&& initialization) -> Expected<AST::ForLoop, Error> {
         CONSUME_TYPE(semicolon, Semicolon);
 
         std::unique_ptr<AST::Expression> condition(nullptr);
@@ -1356,13 +1356,13 @@ auto Parser::parseStatement() -> Expected<UniqueRef<AST::Statement>, Error>
     }
 
     {
-        auto effectfulExpression = backtrackingScope<Expected<UniqueRef<AST::Expression>, Error>>([&]() -> Expected<UniqueRef<AST::Expression>, Error> {
+        auto effectfulExpression = backtrackingScope<Expected<UniqueRef<AST::Statement>, Error>>([&]() -> Expected<UniqueRef<AST::Statement>, Error> {
             PARSE(result, EffectfulExpression);
             CONSUME_TYPE(semicolon, Semicolon);
             return result;
         });
         if (effectfulExpression)
-            return { makeUniqueRef<AST::EffectfulExpressionStatement>(WTFMove(*effectfulExpression)) };
+            return WTFMove(*effectfulExpression);
     }
 
     PARSE(variableDeclarations, VariableDeclarations);
@@ -1370,13 +1370,13 @@ auto Parser::parseStatement() -> Expected<UniqueRef<AST::Statement>, Error>
     return { makeUniqueRef<AST::VariableDeclarationsStatement>(WTFMove(*variableDeclarations)) };
 }
 
-auto Parser::parseEffectfulExpression() -> Expected<UniqueRef<AST::Expression>, Error>
+auto Parser::parseEffectfulExpression() -> Expected<UniqueRef<AST::Statement>, Error>
 {
     PEEK(origin);
-    Vector<UniqueRef<AST::Expression>> expressions;
     if (origin->type == Token::Type::Semicolon)
-        return { makeUniqueRef<AST::CommaExpression>(*origin, WTFMove(expressions)) };
+        return { makeUniqueRef<AST::Block>(*origin, Vector<UniqueRef<AST::Statement>>()) };
 
+    Vector<UniqueRef<AST::Expression>> expressions;
     PARSE(effectfulExpression, EffectfulAssignment);
     expressions.append(WTFMove(*effectfulExpression));
 
@@ -1386,10 +1386,11 @@ auto Parser::parseEffectfulExpression() -> Expected<UniqueRef<AST::Expression>,
     }
 
     if (expressions.size() == 1)
-        return WTFMove(expressions[0]);
+        return { makeUniqueRef<AST::EffectfulExpressionStatement>(WTFMove(expressions[0])) };
     unsigned endOffset = m_lexer.peek().startOffset();
     CodeLocation location(origin->startOffset(), endOffset);
-    return { makeUniqueRef<AST::CommaExpression>(location, WTFMove(expressions)) };
+    auto commaExpression = makeUniqueRef<AST::CommaExpression>(location, WTFMove(expressions));
+    return { makeUniqueRef<AST::EffectfulExpressionStatement>(WTFMove(commaExpression)) };
 }
 
 auto Parser::parseEffectfulAssignment() -> Expected<UniqueRef<AST::Expression>, Error>
index 4ab832a..be0532d 100644 (file)
@@ -131,7 +131,7 @@ private:
     Expected<AST::VariableDeclarationsStatement, Error> parseVariableDeclarations();
     Expected<UniqueRef<AST::Statement>, Error> parseStatement();
 
-    Expected<UniqueRef<AST::Expression>, Error> parseEffectfulExpression();
+    Expected<UniqueRef<AST::Statement>, Error> parseEffectfulExpression();
     Expected<UniqueRef<AST::Expression>, Error> parseEffectfulAssignment();
     struct SuffixExpression {
         SuffixExpression(UniqueRef<AST::Expression>&& result, bool success)
index c8cd025..8123d16 100644 (file)
@@ -461,11 +461,7 @@ void Visitor::visit(AST::Fallthrough&)
 
 void Visitor::visit(AST::ForLoop& forLoop)
 {
-    WTF::visit(WTF::makeVisitor([&](UniqueRef<AST::Statement>& statement) {
-        checkErrorAndVisit(statement);
-    }, [&](UniqueRef<AST::Expression>& expression) {
-        checkErrorAndVisit(expression);
-    }), forLoop.initialization());
+    checkErrorAndVisit(forLoop.initialization());
     if (forLoop.condition())
         checkErrorAndVisit(*forLoop.condition());
     if (forLoop.increment())