Changeset 248650 in webkit

Timestamp:

Aug 13, 2019 6:05:27 PM (5 years ago)

Author:

rmorisset@apple.com

Message:

[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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/webgpu/WHLSL/AST/WHLSLForLoop.h (modified) (3 diffs)
• Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp (modified) (1 diff)
• Modules/webgpu/WHLSL/WHLSLASTDumper.cpp (modified) (1 diff)
• Modules/webgpu/WHLSL/WHLSLChecker.cpp (modified) (1 diff)
• Modules/webgpu/WHLSL/WHLSLParser.cpp (modified) (5 diffs)
• Modules/webgpu/WHLSL/WHLSLParser.h (modified) (1 diff)
• Modules/webgpu/WHLSL/WHLSLVisitor.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLForLoop.h

@@ -46 +46 @@
     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))
@@ -60 +60 @@
     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(); }
@@ -66 +66 @@
 
 private:
-    Variant<UniqueRef<Statement>, UniqueRef<Expression>> m_initialization;
+    UniqueRef<Statement> m_initialization;
     std::unique_ptr<Expression> m_condition;
     std::unique_ptr<Expression> m_increment;

trunk/Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp

@@ -357 +357 @@
 {
     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");

trunk/Source/WebCore/Modules/webgpu/WHLSL/WHLSLASTDumper.cpp

@@ -410 +410 @@
 {
     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())

trunk/Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp

@@ -1444 +1444 @@
 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;

trunk/Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp

@@ -1164 +1164 @@
     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);
 
@@ -1357 +1357 @@
 
     {
-        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);
@@ -1363 +1363 @@
         });
         if (effectfulExpression)
-            return { makeUniqueRef<AST::EffectfulExpressionStatement>(WTFMove(*effectfulExpression)) };
+            return WTFMove(*effectfulExpression);
     }
 
@@ -1371 +1371 @@
 }
 
-auto Parser::parseEffectfulExpression() -> Expected<UniqueRef<AST::Expression>, Error>
+auto Parser::parseEffectfulExpression() -> Expected<UniqueRef<AST::Statement>, Error>
 {
     PEEK(origin);
+    if (origin->type == Token::Type::Semicolon)
+        return { makeUniqueRef<AST::Block>(*origin, Vector<UniqueRef<AST::Statement>>()) };
+
     Vector<UniqueRef<AST::Expression>> expressions;
-    if (origin->type == Token::Type::Semicolon)
-        return { makeUniqueRef<AST::CommaExpression>(*origin, WTFMove(expressions)) };
-
     PARSE(effectfulExpression, EffectfulAssignment);
     expressions.append(WTFMove(*effectfulExpression));
@@ -1387 +1387 @@
 
     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)) };
 }
 

trunk/Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.h

@@ -132 +132 @@
     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 {

trunk/Source/WebCore/Modules/webgpu/WHLSL/WHLSLVisitor.cpp

@@ -462 +462 @@
 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());