Fixed crash during code completion in file included within declaration

[cameron314]

Summary

When triggering code completion within a file that is included in the middle of a declaration in another file, clang would crash while parsing the code.

This occurred with real-world code; there was an enum declaration that included a header in the middle of its declaration to specify the enum members.

[rsmith]

Please add a test to test/CodeCompletion.

lib/Lex/PPLexerChange.cpp

380–383

Can you use CurLexer->cutOffLexing() instead?

[cameron314]

I fixed this last July so the details are a little hazy, but fortunately it turns out I documented what I had found:

It seems when clang parses an enum declaration, it first parses the declaration specifiers, then stops if a semicolon is present.
The trouble is, an #include could cause the file to change part-way through the declaration. If completion is enabled, then when leaving the included file in which the completion was requested, the tokenizer is cleared (clang tries to simulate an EOF at that point to prevent further parsing).
But, while the tokenizer is cleared, the rest of the declaration still needs to be parsed, and the peeked token (a semicolon here) can no longer be advanced over, since it refers in this case to a token in a tokenizer that has been freed. Boom, null reference exception!
It seems changing the lexer kind to the caching lexer when the artificial EOF is inserted does the right thing in this case – a cached EOF token is returned on the advancement of the peeked semicolon.

I tried to add a test; I can reproduce the crash, and with this patch it no longer crashes, but because there's a subsequent parsing error after the completion point, when passing via clang.exe only the error diagnostics are printed without any of the completions (compared to libclang which allows you to retrieve both the completions and the diagnostics). This means I cannot write a RUN command in the lit test without it failing (the exit code is non-zero). I have no way to distinguish between a crash and the normal errors (with hidden completions). Is there any other way to test this?

lib/Lex/PPLexerChange.cpp

380–383

Nope, still crashes when I try (before the reset, obviously).

[rsmith]

In D20131#427717, @cameron314 wrote:

I have no way to distinguish between a crash and the normal errors (with hidden completions). Is there any other way to test this?

If you use // RUN: not %clang_cc1 -code-completion-at=[...], the test should pass if clang fails normally but fail if clang crashes.

[cameron314]

Ah, perfect, thanks.
Behold, a test that fails without the patch and passes with it :-)

[cameron314]

lib/Lex/PPLexerChange.cpp

380–383

Ah, wait, yes this also prevents the crash if I remove the CurLexer.reset() and use CurLexer->cutOffLexing(), but it not only produces an error about a missing '}', but an error about an extra '}' just after, which doesn't really make sense. That means it's inspecting tokens past the EOF that was injected.

lib/Lex/PPLexerChange.cpp

if (!IncludeMacroStack.empty()) {
 
     // If we lexed the code-completion file, act as if we reached EOF.
     if (isCodeCompletionEnabled() && CurPPLexer &&
         SourceMgr.getLocForStartOfFile(CurPPLexer->getFileID()) ==
             CodeCompletionFileLoc) {
       if (CurLexer) {
         Result.startToken();
         CurLexer->FormTokenWithChars(Result, CurLexer->BufferEnd, tok::eof);
         CurLexer.reset();
+        if (CurLexerKind == CLK_Lexer)
+          CurLexerKind = CLK_CachingLexer;
       } else {
         assert(CurPTHLexer && "Got EOF but no current lexer set!");
         CurPTHLexer->getEOF(Result);
         CurPTHLexer.reset();
       }
 
       CurPPLexer = nullptr;
       return true;
     }

test/CodeCompletion/include-within-declaration.c

+// RUN: rm -rf %t && mkdir %t
+// RUN: echo 'MACRO(here)' > %t/y.h
+// Clang will generate an error in this case (though internally the correct completions
+// are present -- this can be seen via libclang) but it should not crash. If it crashes
+// then `not` will fail, otherwise the test succeeds.
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%t/y.h:1:7 -I %t %s
+
+enum {
+#define MACRO(a) FOO
+#include "y.h"
+};