[Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

[shafik]

Summary

When attempting to decide if in C++17 a type template for class template argument deduction and the code is ill-formed the condition to break is checking the current token is an identifier when it should be checking if the next token is an identifier.

This fixes: https://github.com/llvm/llvm-project/issues/57495
https://github.com/llvm/llvm-project/issues/63052

[erichkeane]

I have no real idea what is going on here, the parser isn't an area where I spend much time. Can you ELI5?

[shafik]

In D134334#3805590, @erichkeane wrote:

I have no real idea what is going on here, the parser isn't an area where I spend much time. Can you ELI5?

I am going to try but perhaps fail to explain this in more detail and more clearly.

IIUC we are trying to error recover, we get to

	  // Try to resolve the name. If it doesn't exist, assume it was
          // intended to name a type and keep disambiguating.
          switch (TryAnnotateName()) {

At this point we know the current token is :: and the next token is an identifier. We are trying to annotate the name and it could be a C++17 class template argument deduction case: https://en.cppreference.com/w/cpp/language/class_template_argument_deduction

If we annotate it we should not be left with current token as :: and the next token as an identifier, this is what the assert verifies here:

// Annotated it, check again.
assert(Tok.isNot(tok::annot_cxxscope) ||
       NextToken().isNot(tok::identifier));

So the line I modified says we should only break if we actually annotated and therefore the next token IS NOT an identifier b/c we would have advanced.

[erichkeane]

In D134334#3827895, @shafik wrote:

In D134334#3805590, @erichkeane wrote:

I have no real idea what is going on here, the parser isn't an area where I spend much time. Can you ELI5?

I am going to try but perhaps fail to explain this in more detail and more clearly.

IIUC we are trying to error recover, we get to

	  // Try to resolve the name. If it doesn't exist, assume it was
          // intended to name a type and keep disambiguating.
          switch (TryAnnotateName()) {

At this point we know the current token is :: and the next token is an identifier. We are trying to annotate the name and it could be a C++17 class template argument deduction case: https://en.cppreference.com/w/cpp/language/class_template_argument_deduction

If we annotate it we should not be left with current token as :: and the next token as an identifier, this is what the assert verifies here:

// Annotated it, check again.
assert(Tok.isNot(tok::annot_cxxscope) ||
       NextToken().isNot(tok::identifier));

So the line I modified says we should only break if we actually annotated and therefore the next token IS NOT an identifier b/c we would have advanced.

In that case: 1- could you document that? 2- is there some level of more-clear check you could do? Someting like "if (NextToken().is(tok::annotateTok)"? 3- Should TryAnnotateTypeOrScopeToken give SOME sort of 'I successfully did the thing?' instead?

[shafik]

Adding more reviewers for more visibility.

[cor3ntin]

I does think this patch probably makes sense but I agree with Erich that a comment is in order.
I'm also wondering whether we can end up in a situation where Tok and NextToken() get replaced by a single annotation in which case this would not work. I don't think so.

[rsmith]

clang/lib/Parse/ParseTentative.cpp

1659–1663

This doesn't seem correct to me. If we had scope::foo bar, and we annotate scope::foo as a type, then this will get confused by the next token now being an (unrelated) identifier. This code is trying to detect if an annotation was performed, so I think it intended to check if the current token's kind has changed, like is done on line 1295.

[cor3ntin]

clang/lib/Parse/ParseTentative.cpp

1659–1663

The confusing bit is that Tok is always an annotated scope already here (L1598), so TryAnnotateName should not modify that first token (unless TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current annot_cxxscope by another one, which i don't think can happen?)

[shafik]

• Update check to use tok::annot_cxxscope

[shafik]

clang/lib/Parse/ParseTentative.cpp

1659–1663

Ok using tok::annot_cxxscope also works and I agree it makes sense as well, check-clang also passes.

So then is the assert below wrong?

// Annotated it, check again.
assert(Tok.isNot(tok::annot_cxxscope) ||
       NextToken().isNot(tok::identifier));

It looks like it will work by accident for most cases b/c it checks tok::annot_cxxscope first.

[shafik]

clang/test/ClangScanDeps/modules-full.cpp passes locally for me and I don't see why my change would effect this test.

[rsmith]

clang/lib/Parse/ParseTentative.cpp

1659–1663

The confusing bit is that Tok is always an annotated scope already here (L1598), so TryAnnotateName should not modify that first token (unless TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current annot_cxxscope by another one, which i don't think can happen?)

Yeah, I think TryAnnotateTypeOrScopeToken shouldn't ever replace an annot_cxxscope token with a different annot_cxxscope token representing a longer scope specifier -- an annot_cxxscope token should always be as long as it can be. But it might replace the annot_cxxscope token with an annot_typename, in which case we want to jump out to line 1671 and try again.

So then is the assert below wrong?

I think it's right -- we either reach the assert if we replace the annot_cxxscope with something else (an annot_typename), in the ANK_TemplateName case, or if we've successfully annotated the name (as one of various non-identifier things), in the ANK_Success case. In either case, we only reach the assert if we successfully replaced the identifier with an annotation token, so the assert should succeed.

And the point of the assert, I think, is to ensure that the recursive call to isCXXDeclarationSpecifier cannot reach this same codepath again and recurse forever, so checking the same condition that we checked on entry seems appropriate.

[cor3ntin]

This makes sense to me, thanks!

[shafik]

clang/lib/Parse/ParseTentative.cpp

1659–1663

The confusing bit is that Tok is always an annotated scope already here (L1598), so TryAnnotateName should not modify that first token (unless TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current annot_cxxscope by another one, which i don't think can happen?)

Yeah, I think TryAnnotateTypeOrScopeToken shouldn't ever replace an annot_cxxscope token with a different annot_cxxscope token representing a longer scope specifier -- an annot_cxxscope token should always be as long as it can be. But it might replace the annot_cxxscope token with an annot_typename, in which case we want to jump out to line 1671 and try again.

I see the code that can generate annot_typename but I am so far not able to come up w/ a scenario that hits that case. So I am a little hesitant to handle that w/o adding a test that covers it. Although using

if (Tok.isNot(tok::annot_cxxscope) || Tok.is(tok::annot_typename))
                 break;

does pass check-clang

[shafik]

ping @rsmith

[shafik]

I think I am going to land this as is and if we can come up w/ an example that covers the annot_typename I can do a follow-up. As this is now it fixes a crash bug.

clang/docs/ReleaseNotes.rst

- Fixed a failing assertion when implicitly defining a function within a GNU
   statement expression that appears outside of a function block scope. The
   assertion was benign outside of asserts builds and would only fire in C.
   (`#48579 <https://github.com/llvm/llvm-project/issues/48579>`_).
 - Fixed a failing assertion when applying an attribute to an anonymous union.
   The assertion was benign outside of asserts builds and would only fire in C++.
   (`#48512 <https://github.com/llvm/llvm-project/issues/48512>`_).
 - Fixed a failing assertion when parsing incomplete destructor.
   (`#63503 <https://github.com/llvm/llvm-project/issues/63503>`_)
+- Fix C++17 mode assert when parsing malformed code and the compiler is
+  attempting to see if it could be type template for class template argument
+  deduction. This fixes
+  (`Issue 57495 <https://github.com/llvm/llvm-project/issues/57495>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Parse/ParseTentative.cpp

if (!Tok.is(tok::annot_typename)) {
           case ANK_TentativeDecl:
             return TPResult::False;
           case ANK_TemplateName:
             // In C++17, this could be a type template for class template
             // argument deduction.
             if (getLangOpts().CPlusPlus17) {
               if (TryAnnotateTypeOrScopeToken())
                 return TPResult::Error;
-              if (Tok.isNot(tok::identifier))
+              // If we annotated then the current token should not still be ::
+              // FIXME we may want to also check for tok::annot_typename but
+              // currently don't have a test case.
+              if (Tok.isNot(tok::annot_cxxscope))
                 break;
             }
 
             // A bare type template-name which can't be a template template
             // argument is an error, and was probably intended to be a type.
             // In C++17, this could be class template argument deduction.
             return (getLangOpts().CPlusPlus17 || GreaterThanIsOperator)
                        ? TPResult::True
                        : TPResult::False;

clang/test/Parser/cxx1z-class-template-argument-deduction.cpp

 
 struct A2 {
   template <typename Ty>
   B() noexcept(false); // expected-error {{deduction guide must be declared in the same scope as template 'PR49735::B'}} \
                        // expected-error {{deduction guide declaration without trailing return type}}
 };
 
 }
+
+namespace GH57495 {
+template <typename T> struct vector{};
+
+void f() {
+  GH57495::vector.d; // expected-error {{cannot use dot operator on a type}}
+}
+}