Fix crash with invalid member function param list

[AaronBallman]

We cannot consume annotation tokens with ConsumeToken(), so any pragmas present in an invalid initializer would previously crash. Now we handle annotation tokens more generally and avoid the crash.

Fixes #113722

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

We cannot consume annotation tokens with ConsumeToken(), so any pragmas present in an invalid initializer would previously crash. Now we handle annotation tokens more generally and avoid the crash.

Fixes #113722

---

Full diff: https://github.com/llvm/llvm-project/pull/139595.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Parse/ParseCXXInlineMethods.cpp (+5-4)
• (modified) clang/test/Parser/cxx-invalid-function-decl.cpp (+10)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 257c4696de403..37cc037be0026 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -579,6 +579,8 @@ Bug Fixes in This Version
   ``#include`` directive. (#GH138094)
 - Fixed a crash during constant evaluation involving invalid lambda captures
   (#GH138832)
+- Fixed a crash with an invalid member function parameter list with a default
+  argument which contains a pragma. (#GH113722)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseCXXInlineMethods.cpp b/clang/lib/Parse/ParseCXXInlineMethods.cpp
index e76435d0e9de7..342d46770c656 100644
--- a/clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ b/clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -1272,10 +1272,6 @@ bool Parser::ConsumeAndStoreInitializer(CachedTokens &Toks,
       goto consume_token;
 
     case tok::eof:
-    case tok::annot_module_begin:
-    case tok::annot_module_end:
-    case tok::annot_module_include:
-    case tok::annot_repl_input_end:
       // Ran out of tokens.
       return false;
 
@@ -1411,6 +1407,11 @@ bool Parser::ConsumeAndStoreInitializer(CachedTokens &Toks,
       [[fallthrough]];
     default:
     consume_token:
+      // If it's an annotation token, then we've run out of tokens and should
+      // bail out. Otherwise, cache the token and consume it.
+      if (Tok.isAnnotation())
+        return false;
+
       Toks.push_back(Tok);
       ConsumeToken();
       break;
diff --git a/clang/test/Parser/cxx-invalid-function-decl.cpp b/clang/test/Parser/cxx-invalid-function-decl.cpp
index 2db27516eefab..39d3221d3d279 100644
--- a/clang/test/Parser/cxx-invalid-function-decl.cpp
+++ b/clang/test/Parser/cxx-invalid-function-decl.cpp
@@ -40,3 +40,13 @@ struct S : public Base1<int>, public Base2<float> {
   // All initializers are correct, nothing to skip, diagnose 2 missing commas.
   S(const S &) : Base1<int>(0) ::Base2<float>(1.0) x(2) {} // expected-error2{{missing ',' between base or member initializers}}
 };
+
+namespace GH113722 {
+struct S {
+  void m(int x = 0;   // expected-error {{unexpected end of default argument expression}} \
+                         expected-note {{to match this '('}}
+    #pragma unused(x) // expected-error {{expected ')'}} \
+                         expected-error {{expected ';' at end of declaration list}}
+  }                   // expected-error {{expected ';' after struct}}
+};
+}                     // expected-error {{extraneous closing brace ('}')}}

[erichkeane]

Does this mean we can't have pragmas in the middle of a statement? It seems that something like the above is actually useful/a shame to lose. It would be wonderful if instead the ConsumeAndStoreInitializer just 'did the pragma parsing and came back', but I presume that is a lot more work?

[AaronBallman]

Does this mean we can't have pragmas in the middle of a statement?

Nope, this still works fine:

struct S {
  void m(int x =
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wconversion"
  1.2f
  #pragma clang diagnostic pop
  );
  void n(int x =
  1.2f
  );
};

I can add a test case for that if you want. This is just an issue with an ill-formed initializer, I believe.

[erichkeane]

OH! missed that this was a function parameter/default argument and part of one that is already ill-formed. I thinks this recovery makes sense, I don't see any need for other tests