Forbid defined operator generated by macro expansion

src/compiler/preprocessor/DirectiveParser.cpp

@@ -139,6 +139,66 @@ bool isMacroPredefined(const std::string &name,
 namespace pp
 {
 
+class DefinedParser : public Lexer
+{
+  public:
+    DefinedParser(Lexer *lexer, const MacroSet *macroSet, Diagnostics *diagnostics)
+        : mLexer(lexer), mMacroSet(macroSet), mDiagnostics(diagnostics)
+    {
+    }
+
+  protected:
+    void lex(Token *token) override
+    {
+        const char kDefined[] = "defined";
+
+        mLexer->lex(token);
+        if (token->type != Token::IDENTIFIER)
+            return;
+        if (token->text != kDefined)
+            return;
+
+        bool paren = false;
+        mLexer->lex(token);
+        if (token->type == '(')
+        {
+            paren = true;
+            mLexer->lex(token);
+        }
+
+        if (token->type != Token::IDENTIFIER)
+        {
+            mDiagnostics->report(Diagnostics::PP_UNEXPECTED_TOKEN, token->location, token->text);
+            skipUntilEOD(mLexer, token);
+            return;
+        }
+        MacroSet::const_iterator iter = mMacroSet->find(token->text);
+        std::string expression        = iter != mMacroSet->end() ? "1" : "0";
+
+        if (paren)
+        {
+            mLexer->lex(token);
+            if (token->type != ')')
+            {
+                mDiagnostics->report(Diagnostics::PP_UNEXPECTED_TOKEN, token->location,
+                                     token->text);
+                skipUntilEOD(mLexer, token);
+                return;
+            }
+        }
+
+        // We have a valid defined operator.
+        // Convert the current token into a CONST_INT token.
+        token->type = Token::CONST_INT;
+        token->text = expression;
+    }
+
+  private:
+    Lexer *mLexer;
+    const MacroSet *mMacroSet;
+    Diagnostics *mDiagnostics;
+};
+
 DirectiveParser::DirectiveParser(Tokenizer *tokenizer,
                                  MacroSet *macroSet,
                                  Diagnostics *diagnostics,
@@ -776,7 +836,7 @@ void DirectiveParser::parseLine(Token *token)
     bool parsedFileNumber = false;
     int line = 0, file = 0;
 
-    MacroExpander macroExpander(mTokenizer, mMacroSet, mDiagnostics, false);
+    MacroExpander macroExpander(mTokenizer, mMacroSet, mDiagnostics);
 
     // Lex the first token after "#line" so we can check it for EOD.
     macroExpander.lex(token);
@@ -885,7 +945,8 @@ int DirectiveParser::parseExpressionIf(Token *token)
     assert((getDirective(token) == DIRECTIVE_IF) ||
            (getDirective(token) == DIRECTIVE_ELIF));
 
-    MacroExpander macroExpander(mTokenizer, mMacroSet, mDiagnostics, true);
+    DefinedParser definedParser(mTokenizer, mMacroSet, mDiagnostics);
+    MacroExpander macroExpander(&definedParser, mMacroSet, mDiagnostics);
     ExpressionParser expressionParser(&macroExpander, mDiagnostics);
 
     int expression = 0;

src/compiler/preprocessor/MacroExpander.cpp

@@ -46,11 +46,8 @@ class TokenLexer : public Lexer
     TokenVector::const_iterator mIter;
 };
 
-MacroExpander::MacroExpander(Lexer *lexer,
-                             MacroSet *macroSet,
-                             Diagnostics *diagnostics,
-                             bool parseDefined)
-    : mLexer(lexer), mMacroSet(macroSet), mDiagnostics(diagnostics), mParseDefined(parseDefined)
+MacroExpander::MacroExpander(Lexer *lexer, MacroSet *macroSet, Diagnostics *diagnostics)
+    : mLexer(lexer), mMacroSet(macroSet), mDiagnostics(diagnostics)
 {
 }
 
@@ -66,54 +63,11 @@ void MacroExpander::lex(Token *token)
 {
     while (true)
     {
-        const char kDefined[] = "defined";
-
         getToken(token);
 
         if (token->type != Token::IDENTIFIER)
             break;
 
-        // Defined operator is parsed here since it may be generated by macro expansion.
-        // Defined operator produced by macro expansion has undefined behavior according to C++
-        // spec, which the GLSL spec references (see C++14 draft spec section 16.1.4), but this
-        // behavior is needed for passing dEQP tests, which enforce stricter compatibility between
-        // implementations.
-        if (mParseDefined && token->text == kDefined)
-        {
-            bool paren = false;
-            getToken(token);
-            if (token->type == '(')
-            {
-                paren = true;
-                getToken(token);
-            }
-            if (token->type != Token::IDENTIFIER)
-            {
-                mDiagnostics->report(Diagnostics::PP_UNEXPECTED_TOKEN, token->location,
-                                     token->text);
-                break;
-            }
-            auto iter              = mMacroSet->find(token->text);
-            std::string expression = iter != mMacroSet->end() ? "1" : "0";
-
-            if (paren)
-            {
-                getToken(token);
-                if (token->type != ')')
-                {
-                    mDiagnostics->report(Diagnostics::PP_UNEXPECTED_TOKEN, token->location,
-                                         token->text);
-                    break;
-                }
-            }
-
-            // We have a valid defined operator.
-            // Convert the current token into a CONST_INT token.
-            token->type = Token::CONST_INT;
-            token->text = expression;
-            break;
-        }
-
         if (token->expansionDisabled())
             break;
 
@@ -367,7 +321,7 @@ bool MacroExpander::collectMacroArgs(const Macro &macro,
     {
         MacroArg &arg = args->at(i);
         TokenLexer lexer(&arg);
-        MacroExpander expander(&lexer, mMacroSet, mDiagnostics, mParseDefined);
+        MacroExpander expander(&lexer, mMacroSet, mDiagnostics);
 
         arg.clear();
         expander.lex(&token);

src/compiler/preprocessor/MacroExpander.h

@@ -24,7 +24,7 @@ struct SourceLocation;
 class MacroExpander : public Lexer
 {
   public:
-    MacroExpander(Lexer *lexer, MacroSet *macroSet, Diagnostics *diagnostics, bool parseDefined);
+    MacroExpander(Lexer *lexer, MacroSet *macroSet, Diagnostics *diagnostics);
     ~MacroExpander() override;
 
     void lex(Token *token) override;
@@ -81,7 +81,6 @@ class MacroExpander : public Lexer
     Lexer *mLexer;
     MacroSet *mMacroSet;
     Diagnostics *mDiagnostics;
-    bool mParseDefined;
 
     std::unique_ptr<Token> mReserveToken;
     std::vector<MacroContext *> mContextStack;

src/compiler/preprocessor/Preprocessor.cpp

@@ -30,7 +30,7 @@ struct PreprocessorImpl
         : diagnostics(diag),
           tokenizer(diag),
           directiveParser(&tokenizer, &macroSet, diag, directiveHandler),
-          macroExpander(&directiveParser, &macroSet, diag, false)
+          macroExpander(&directiveParser, &macroSet, diag)
     {
     }
 };

src/tests/deqp_support/deqp_gles2_test_expectations.txt

@@ -34,6 +34,9 @@
 998 WIN LINUX MAC : dEQP-GLES2.performance.* = SKIP
 998 WIN LINUX MAC : dEQP-GLES2.stress.* = SKIP
 
+// Tests that we fail because they're not in line with the WebGL spec
+1335 WIN LINUX MAC : dEQP-GLES2.functional.shaders.preprocessor.conditional_inclusion.basic_2* = FAIL
+
 ////////////////////////////////////////////////////////////////////////////////
 //
 //  Temporary entries: they should be removed once the bugs are fixed.

src/tests/deqp_support/deqp_gles3_test_expectations.txt

@@ -37,6 +37,9 @@
 1097 WIN : dEQP-GLES3.functional.fbo.completeness.size.distinct = FAIL
 
 // Tests that we fail because they're not in line with the WebGL spec
+1335 MAC WIN LINUX : dEQP-GLES3.functional.shaders.preprocessor.conditional_inclusion.basic_2* = FAIL
+1335 MAC WIN LINUX : dEQP-GLES3.functional.shaders.preprocessor.conditional_inclusion.defined_macro_defined_test* = FAIL
+1335 MAC WIN LINUX : dEQP-GLES3.functional.shaders.preprocessor.conditional_inclusion.defined_macro_undef* = FAIL
 1335 MAC WIN LINUX : dEQP-GLES3.functional.shaders.preprocessor.conditional_inclusion.define_defined* = FAIL
 1335 MAC WIN LINUX : dEQP-GLES3.functional.shaders.preprocessor.conditional_inclusion.define_defined_outside_if* = FAIL
 

src/tests/preprocessor_tests/if_test.cpp

@@ -902,16 +902,17 @@ TEST_F(IfTest, DefinedOperatorValidAfterMacroExpansion)
         "#if !foo bar\n"
         "pass\n"
         "#endif\n";
-    const char *expected =
-        "\n"
-        "\n"
-        "pass\n"
-        "\n";
+    ASSERT_TRUE(mPreprocessor.init(1, &str, 0));
 
-    preprocess(str, expected);
+    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_CONDITIONAL_UNEXPECTED_TOKEN,
+                                    pp::SourceLocation(0, 2), "defined"));
+    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_CONDITIONAL_UNEXPECTED_TOKEN,
+                                    pp::SourceLocation(0, 2), "bar"));
+
+    pp::Token token;
+    mPreprocessor.lex(&token);
 }
 
-// Unterminated defined operator generated by macro expansion should generate appropriate errors.
 // Defined operator produced by macro expansion has undefined behavior according to C++ spec,
 // which the GLSL spec references (see C++14 draft spec section 16.1.4), but this behavior is
 // needed for passing dEQP tests, which enforce stricter compatibility between implementations.
@@ -923,16 +924,15 @@ TEST_F(IfTest, UnterminatedDefinedInMacro)
         "#endif\n";
     ASSERT_TRUE(mPreprocessor.init(1, &str, 0));
 
-    EXPECT_CALL(mDiagnostics,
-                print(pp::Diagnostics::PP_UNEXPECTED_TOKEN, pp::SourceLocation(0, 2), "\n"));
-    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_INVALID_EXPRESSION,
-                                    pp::SourceLocation(0, 2), "syntax error"));
+    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_CONDITIONAL_UNEXPECTED_TOKEN,
+                                    pp::SourceLocation(0, 2), "defined"));
+    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_CONDITIONAL_UNEXPECTED_TOKEN,
+                                    pp::SourceLocation(0, 2), "("));
 
     pp::Token token;
     mPreprocessor.lex(&token);
 }
 
-// Unterminated defined operator generated by macro expansion should generate appropriate errors.
 // Defined operator produced by macro expansion has undefined behavior according to C++ spec,
 // which the GLSL spec references (see C++14 draft spec section 16.1.4), but this behavior is
 // needed for passing dEQP tests, which enforce stricter compatibility between implementations.
@@ -944,10 +944,10 @@ TEST_F(IfTest, UnterminatedDefinedInMacro2)
         "#endif\n";
     ASSERT_TRUE(mPreprocessor.init(1, &str, 0));
 
-    EXPECT_CALL(mDiagnostics,
-                print(pp::Diagnostics::PP_UNEXPECTED_TOKEN, pp::SourceLocation(0, 2), "\n"));
-    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_INVALID_EXPRESSION,
-                                    pp::SourceLocation(0, 2), "syntax error"));
+    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_CONDITIONAL_UNEXPECTED_TOKEN,
+                                    pp::SourceLocation(0, 2), "defined"));
+    EXPECT_CALL(mDiagnostics, print(pp::Diagnostics::PP_CONDITIONAL_UNEXPECTED_TOKEN,
+                                    pp::SourceLocation(0, 2), "("));
 
     pp::Token token;
     mPreprocessor.lex(&token);