Page MenuHomePhabricator

[Lex] Fix crash on code completion in comment in included file.
ClosedPublic

Authored by vsapsai on Jan 2 2018, 5:22 PM.

Details

Summary

This fixes PR32732 by updating CurLexerKind to reflect available lexers.
We were hitting null pointer in Preprocessor::Lex because CurLexerKind
was CLK_Lexer but CurLexer was null. And we set it to null in
Preprocessor::HandleEndOfFile when exiting a file with code completion
point.

To reproduce the crash it is important for a comment to be inside a
class specifier. In this case in Parser::ParseClassSpecifier we improve
error recovery by pushing a semicolon token back into the preprocessor
and later on try to lex a token because we haven't reached the end of
file.

Also clang crashes only on code completion in included file, i.e. when
IncludeMacroStack is not empty. Though we reset CurLexer even if include
stack is empty. The difference is that during pushing back a semicolon
token, preprocessor calls EnterCachingLexMode which decides it is
already in caching mode because various lexers are null and
IncludeMacroStack is not empty. As the result, CurLexerKind remains
CLK_Lexer instead of updating to CLK_CachingLexer.

rdar://problem/34787685

Diff Detail

Repository
rC Clang

Event Timeline

vsapsai created this revision.Jan 2 2018, 5:22 PM

A few approaches that I've considered but decided not to pursue:

  • Change code completion in comment so we don't exit file early but more gracefully. Decided not to do it because I believe early exit is important for code completion performance. And CurLexer/CurLexerKind discrepancy would remain anyway, which is not good.
  • Use RemoveTopOfLexerStack instead of recomputeCurLexerKind. It works but I'm not confident it doesn't have undesired consequences. And I don't like that after removing the top of lexer stack, pushing back tokens would happen on top of includer, not on top of includee. But it might be totally OK.
  • Add assertion in InCachingLexMode that returned value is in sync with CurLexerKind. It works but I suspect there might be legitimate reasons for them to be out of sync for some time. Though it can be a mistake and assertion can help to catch it.
vsapsai added a subscriber: kfunk.Jan 2 2018, 5:54 PM
arphaman accepted this revision.Jan 19 2018, 3:14 PM
arphaman added a subscriber: arphaman.

I think that's a sensible fix, thanks!

This revision is now accepted and ready to land.Jan 19 2018, 3:14 PM
This revision was automatically updated to reflect the committed changes.