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
