Re: [PATCH] D20131: Fixed crash during code completion in file included within declaration
rsmith added a comment. Please add a test to test/CodeCompletion. Comment at: lib/Lex/PPLexerChange.cpp:380-382 @@ -379,3 +379,5 @@ CurLexer->FormTokenWithChars(Result, CurLexer->BufferEnd, tok::eof); CurLexer.reset(); +if (CurLexerKind == CLK_Lexer) + CurLexerKind = CLK_CachingLexer; } else { Can you use `CurLexer->cutOffLexing()` instead? http://reviews.llvm.org/D20131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20131: Fixed crash during code completion in file included within declaration
cameron314 added a comment. 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? Comment at: lib/Lex/PPLexerChange.cpp:380-382 @@ -379,3 +379,5 @@ CurLexer->FormTokenWithChars(Result, CurLexer->BufferEnd, tok::eof); CurLexer.reset(); +if (CurLexerKind == CLK_Lexer) + CurLexerKind = CLK_CachingLexer; } else { rsmith wrote: > Can you use `CurLexer->cutOffLexing()` instead? Nope, still crashes when I try (before the reset, obviously). http://reviews.llvm.org/D20131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20131: Fixed crash during code completion in file included within declaration
rsmith added a comment. In http://reviews.llvm.org/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. http://reviews.llvm.org/D20131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20131: Fixed crash during code completion in file included within declaration
cameron314 updated this revision to Diff 56961. cameron314 added a comment. Ah, perfect, thanks. Behold, a test that fails without the patch and passes with it :-) http://reviews.llvm.org/D20131 Files: lib/Lex/PPLexerChange.cpp test/CodeCompletion/include-within-declaration.c Index: test/CodeCompletion/include-within-declaration.c === --- /dev/null +++ test/CodeCompletion/include-within-declaration.c @@ -0,0 +1,11 @@ +// 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" +}; Index: lib/Lex/PPLexerChange.cpp === --- lib/Lex/PPLexerChange.cpp +++ lib/Lex/PPLexerChange.cpp @@ -378,6 +378,8 @@ 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); Index: test/CodeCompletion/include-within-declaration.c === --- /dev/null +++ test/CodeCompletion/include-within-declaration.c @@ -0,0 +1,11 @@ +// 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" +}; Index: lib/Lex/PPLexerChange.cpp === --- lib/Lex/PPLexerChange.cpp +++ lib/Lex/PPLexerChange.cpp @@ -378,6 +378,8 @@ 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); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20131: Fixed crash during code completion in file included within declaration
cameron314 added inline comments. Comment at: lib/Lex/PPLexerChange.cpp:380-383 @@ -379,4 +379,6 @@ 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!"); 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. http://reviews.llvm.org/D20131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits