Page MenuHomePhabricator

[Parser] Fix assertion-on-invalid for unexpected typename.
ClosedPublic

Authored by vsapsai on Mar 13 2018, 3:29 PM.

Details

Summary

In ParseDeclarationSpecifiers for the code

class A typename A;

we were able to annotate token kw_typename because it refers to
existing type. But later during processing token annot_typename we
failed to SetTypeSpecType and exited switch statement leaving
annotation token unconsumed. The code after the switch statement failed
because it didn't expect a special token.

The fix is not to assume that switch statement consumes all special
tokens and consume any token, not just non-special.

rdar://problem/37099386

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Mar 13 2018, 3:29 PM

I had performance considerations regarding this change because ConsumeAnyToken is heavier than ConsumeToken. But I didn't notice any problems. If you know this is a hot path that deserves more attention, please let me know.

clang/lib/Parse/ParseDecl.cpp
3076 ↗(On Diff #138269)

Here we potentially can leave annotation token unconsumed. But I wasn't able to create a test case that would trigger a problem at this point.

3148 ↗(On Diff #138269)

We didn't consume the annotation token because of break on isInvalid a few lines above.

rsmith accepted this revision.Apr 4 2018, 1:42 PM

Thanks, seems reasonable to me.

clang/lib/Parse/ParseDecl.cpp
3076 ↗(On Diff #138269)

I think you're talking about the break a few lines back?

I think we actually get away with this, but only because SetTypeSpecType can only fail if there was already a type specifier, which we checked for on line 3019, so isInvalid is never true.

3148 ↗(On Diff #138269)

I wonder if our error recovery would be improved by changing line 3134 to just

if (DS.hasTypeSpecifier())

(which would likewise render the break here unreachable given the current rules enforced by SetTypeSpecType).

3802 ↗(On Diff #138269)

Maybe add a comment here saying we can get here with an annotation token after an error?

This revision is now accepted and ready to land.Apr 4 2018, 1:42 PM
vsapsai added inline comments.Apr 6 2018, 3:36 PM
clang/lib/Parse/ParseDecl.cpp
3076 ↗(On Diff #138269)

You are right, I was talking about break on the line 3071.

3148 ↗(On Diff #138269)

Change in if condition fixes the assertion too and the diagnostics are

repro.cpp:1:18: error: expected a qualified name after 'typename'
class A typename A;
                 ^
repro.cpp:1:18: error: expected unqualified-id
2 errors generated.

With the currently reviewed change diagnostics are

repro.cpp:1:18: error: expected a qualified name after 'typename'
class A typename A;
                 ^
repro.cpp:1:18: error: cannot combine with previous 'class' declaration specifier
2 errors generated.

This is for the file

$ cat repro.cpp 
class A typename A;

This small test isn't representable but I find showing next to each other

expected a qualified name
expected unqualified-id

confusing and contradictory at the first glance.

If there are no further comments, I'll commit the change as is (after adding a comment for ConsumeAnyToken).

3802 ↗(On Diff #138269)

Will do.

vsapsai updated this revision to Diff 141448.Apr 6 2018, 3:55 PM
  • Add a comment.
This revision was automatically updated to reflect the committed changes.