[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
jan-wassenberg added a comment. @nik here's an unusual but real-world example that triggers this. https://github.com/google/highway compiles the same source multiple times (with different macros set) for generating code for multiple SIMD instruction sets. The main source file sets a macro to its filename and includes a "foreach_target.h" which includes the file identified by that macro. I understand this is complicated for analysis, so foreach_target.h does nothing if it detects that an IDE/analyzer are parsing. This relies on predefined macros such as __CDT_PARSER__ __INTELLISENSE__ Q_CREATOR_RUN. Is there such a macro for clangd? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
ilya-biryukov added a comment. I guess using `Edit Related Object -> Edit Commits` should do the trick. I'm not sure what the "Lean Into Action" is either. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik closed this revision. nik added a comment. Huch, I forgot to add "Differential Revision: " to the commit message, so I'll close this manually once I know how to add the svn revision number to this. https://llvm.org/docs/Phabricator.html states: > In the web UI, under “Leap Into Action” put the SVN revision number in the > Comment, set the Action to “Close Revision” and click Submit. Where is the "Lean Into Action" thingy? Can't find it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik updated this revision to Diff 198997.
nik marked an inline comment as done and an inline comment as not done.
nik added a comment.
Renamed to err_pp_including_mainfile_in_preamble.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
Files:
include/clang/Basic/DiagnosticLexKinds.td
lib/Basic/SourceManager.cpp
lib/Lex/PPDirectives.cpp
test/Index/preamble-cyclic-include.cpp
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test
-test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a
preamble
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,18 @@
return {ImportAction::None};
}
+ // Check for circular inclusion of the main file.
+ // We can't generate a consistent preamble with regard to the conditional
+ // stack if the main file is included again as due to the preamble bounds
+ // some directives (e.g. #endif of a header guard) will never be seen.
+ // Since this will lead to confusing errors, avoid the inclusion.
+ if (File && PreambleConditionalStack.isRecording() &&
+ SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_in_preamble);
+return {ImportAction::None};
+ }
+
// Should we enter the source file? Set to Skip if either the source file is
// known to have no effect beyond its effect on module visibility -- that is,
// if it's got an include guard that is already defined, set to Import if it
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1582,7 +1582,7 @@
if (MainSLoc.isFile()) {
const ContentCache *MainContentCache
= MainSLoc.getFile().getContentCache();
- if (!MainContentCache) {
+ if (!MainContentCache || !MainContentCache->OrigEntry) {
// Can't do anything
} else if (MainContentCache->OrigEntry == SourceFile) {
FirstFID = MainFileID;
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -426,6 +426,8 @@
"did not find header '%0' in framework '%1' (loaded from '%2')">;
def err_pp_error_opening_file : Error<
"error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_in_preamble : Error<
+ "main file cannot be included recursively when building a preamble">;
def err_pp_empty_filename : Error<"empty filename">;
def err_pp_include_too_deep : Error<"#include nested too deeply">;
def err_pp_expects_filename : Error<"expected \"FILENAME\" or ">;
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a preamble
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,18 @@
return {ImportAction::None};
}
+ // Check for circular inclusion of the main file.
+ // We can't generate a consistent preamble with regard to the conditional
+ // stack if the main file is included again as due to the preamble bounds
+ // some directives (e.g. #endif of a header guard) will never be seen.
+ // Since this will lead to confusing errors, avoid the inclusion.
+ if (File && PreambleConditionalStack.isRecording() &&
+ SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_in_preamble);
+return {ImportAction::None};
+ }
+
// Should we enter the source file? Set to Skip if either the source file is
// known to have no effect beyond its effect on module visibility -- that is,
// if it's got an include guard that is already defined, set to Import if it
Index: lib/Basic/SourceManager.cpp
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. See the nit about naming of an error, though
Comment at: include/clang/Basic/DiagnosticLexKinds.td:429
"error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_for_preamble : Error<
+ "main file cannot be included recursively when building a preamble">;
NIT: Maybe change to err_pp_including_mainfile_**in**_preamble?
Comment at: lib/Basic/SourceManager.cpp:1594
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
nik wrote:
> ilya-biryukov wrote:
> > nik wrote:
> > > ilya-biryukov wrote:
> > > > Can this actually be`null`?
> > > The comment for OrigEntry states that it might be null:
> > >
> > > /// Reference to the file entry representing this ContentCache.
> > > ///
> > > /// This reference does not own the FileEntry object.
> > > ///
> > > /// It is possible for this to be NULL if the ContentCache
> > > encapsulates
> > > /// an imaginary text buffer.
> > > const FileEntry *OrigEntry;
> > >
> > > Note also that further down in this function, a null check for
> > > MainContentCache->OrigEntry is done, so I believe that this was just
> > > forgotten here (since there is also no assert) and we are the first one
> > > running into this with the introduced call to
> > > SourceMgr.translateFile(File).
> > >
> > > I've tried to understand why we end up with a nullptr here, but this goes
> > > beyond me in the time I've taken for this. What I've observed is that
> > > module code path (LibclangReparseTest.ReparseWithModule vs
> > > LibclangReparseTest.Reparse) creates at least a MemoryBuffer more (in
> > > ModuleMap::parseModuleMapFile; possibly the one referred with "imaginary
> > > text buffer" from the comment) and I suspect this to be somehow related
> > > with the OrigEntry nullptr.
> > Should we check for equality of `MainContentCache->ContentsEntry` in that
> > case? I guess this is what should be set for "imaginary" files.
> Does not help in this particular case as ContentsEntry is also a nullptr. All
> the members of the ContentsCache seem to be either nullptr or 0 as that
> particular point of execution.
>
> How to continue?
>
> a) Accept as is.
> b) Ask someone knowledgeable for whom this could be a no-brainer that could
> respond hopefully soon. Any candidate?
> c) Or should I dig deeper? This can take quite some time as this area is new
> for me.
>
Wow, I guess I don't understand this code deep enough.
Your change looks ok, I'm happy to LGTM this. But if you're actually interested
in digging deeper, that would be super-useful.
Based on what you're describe, it feels like "imaginary" main file works by
accident rather than being properly handled in this function.
BTW, thanks for moving the check up, it definitely looks nicer this way.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik marked an inline comment as done.
nik added inline comments.
Comment at: lib/Basic/SourceManager.cpp:1594
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
ilya-biryukov wrote:
> nik wrote:
> > ilya-biryukov wrote:
> > > Can this actually be`null`?
> > The comment for OrigEntry states that it might be null:
> >
> > /// Reference to the file entry representing this ContentCache.
> > ///
> > /// This reference does not own the FileEntry object.
> > ///
> > /// It is possible for this to be NULL if the ContentCache encapsulates
> > /// an imaginary text buffer.
> > const FileEntry *OrigEntry;
> >
> > Note also that further down in this function, a null check for
> > MainContentCache->OrigEntry is done, so I believe that this was just
> > forgotten here (since there is also no assert) and we are the first one
> > running into this with the introduced call to SourceMgr.translateFile(File).
> >
> > I've tried to understand why we end up with a nullptr here, but this goes
> > beyond me in the time I've taken for this. What I've observed is that
> > module code path (LibclangReparseTest.ReparseWithModule vs
> > LibclangReparseTest.Reparse) creates at least a MemoryBuffer more (in
> > ModuleMap::parseModuleMapFile; possibly the one referred with "imaginary
> > text buffer" from the comment) and I suspect this to be somehow related
> > with the OrigEntry nullptr.
> Should we check for equality of `MainContentCache->ContentsEntry` in that
> case? I guess this is what should be set for "imaginary" files.
Does not help in this particular case as ContentsEntry is also a nullptr. All
the members of the ContentsCache seem to be either nullptr or 0 as that
particular point of execution.
How to continue?
a) Accept as is.
b) Ask someone knowledgeable for whom this could be a no-brainer that could
respond hopefully soon. Any candidate?
c) Or should I dig deeper? This can take quite some time as this area is new
for me.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
ilya-biryukov added inline comments.
Comment at: lib/Basic/SourceManager.cpp:1594
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
nik wrote:
> ilya-biryukov wrote:
> > Can this actually be`null`?
> The comment for OrigEntry states that it might be null:
>
> /// Reference to the file entry representing this ContentCache.
> ///
> /// This reference does not own the FileEntry object.
> ///
> /// It is possible for this to be NULL if the ContentCache encapsulates
> /// an imaginary text buffer.
> const FileEntry *OrigEntry;
>
> Note also that further down in this function, a null check for
> MainContentCache->OrigEntry is done, so I believe that this was just
> forgotten here (since there is also no assert) and we are the first one
> running into this with the introduced call to SourceMgr.translateFile(File).
>
> I've tried to understand why we end up with a nullptr here, but this goes
> beyond me in the time I've taken for this. What I've observed is that module
> code path (LibclangReparseTest.ReparseWithModule vs
> LibclangReparseTest.Reparse) creates at least a MemoryBuffer more (in
> ModuleMap::parseModuleMapFile; possibly the one referred with "imaginary text
> buffer" from the comment) and I suspect this to be somehow related with the
> OrigEntry nullptr.
Should we check for equality of `MainContentCache->ContentsEntry` in that case?
I guess this is what should be set for "imaginary" files.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik updated this revision to Diff 198799.
nik added a comment.
Moved the MainFile / MainContentCache->OrigEntry check a bit further up, for
consistency with the same test further down in SourceManager::translateFile().
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
Files:
include/clang/Basic/DiagnosticLexKinds.td
lib/Basic/SourceManager.cpp
lib/Lex/PPDirectives.cpp
test/Index/preamble-cyclic-include.cpp
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test
-test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a
preamble
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,18 @@
return {ImportAction::None};
}
+ // Check for circular inclusion of the main file.
+ // We can't generate a consistent preamble with regard to the conditional
+ // stack if the main file is included again as due to the preamble bounds
+ // some directives (e.g. #endif of a header guard) will never be seen.
+ // Since this will lead to confusing errors, avoid the inclusion.
+ if (File && PreambleConditionalStack.isRecording() &&
+ SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_for_preamble);
+return {ImportAction::None};
+ }
+
// Should we enter the source file? Set to Skip if either the source file is
// known to have no effect beyond its effect on module visibility -- that is,
// if it's got an include guard that is already defined, set to Import if it
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1582,7 +1582,7 @@
if (MainSLoc.isFile()) {
const ContentCache *MainContentCache
= MainSLoc.getFile().getContentCache();
- if (!MainContentCache) {
+ if (!MainContentCache || !MainContentCache->OrigEntry) {
// Can't do anything
} else if (MainContentCache->OrigEntry == SourceFile) {
FirstFID = MainFileID;
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -426,6 +426,8 @@
"did not find header '%0' in framework '%1' (loaded from '%2')">;
def err_pp_error_opening_file : Error<
"error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_for_preamble : Error<
+ "main file cannot be included recursively when building a preamble">;
def err_pp_empty_filename : Error<"empty filename">;
def err_pp_include_too_deep : Error<"#include nested too deeply">;
def err_pp_expects_filename : Error<"expected \"FILENAME\" or ">;
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a preamble
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,18 @@
return {ImportAction::None};
}
+ // Check for circular inclusion of the main file.
+ // We can't generate a consistent preamble with regard to the conditional
+ // stack if the main file is included again as due to the preamble bounds
+ // some directives (e.g. #endif of a header guard) will never be seen.
+ // Since this will lead to confusing errors, avoid the inclusion.
+ if (File && PreambleConditionalStack.isRecording() &&
+ SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_for_preamble);
+return {ImportAction::None};
+ }
+
// Should we enter the source file? Set to Skip if either the source file is
// known to have no effect beyond its effect on module visibility -- that is,
// if it's got an include guard that is already defined, set to Import if it
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik marked an inline comment as done.
nik added inline comments.
Comment at: lib/Basic/SourceManager.cpp:1594
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
ilya-biryukov wrote:
> Can this actually be`null`?
The comment for OrigEntry states that it might be null:
/// Reference to the file entry representing this ContentCache.
///
/// This reference does not own the FileEntry object.
///
/// It is possible for this to be NULL if the ContentCache encapsulates
/// an imaginary text buffer.
const FileEntry *OrigEntry;
Note also that further down in this function, a null check for
MainContentCache->OrigEntry is done, so I believe that this was just forgotten
here (since there is also no assert) and we are the first one running into this
with the introduced call to SourceMgr.translateFile(File).
I've tried to understand why we end up with a nullptr here, but this goes
beyond me in the time I've taken for this. What I've observed is that module
code path (LibclangReparseTest.ReparseWithModule vs
LibclangReparseTest.Reparse) creates at least a MemoryBuffer more (in
ModuleMap::parseModuleMapFile; possibly the one referred with "imaginary text
buffer" from the comment) and I suspect this to be somehow related with the
OrigEntry nullptr.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
ilya-biryukov added a comment.
Sorry for losing this.
Neat change, minimal and focused, thanks!
Just wanted to clarify why we need the change in `SourceManager.cpp`, will LGTM
as soon as we resolve this
Comment at: lib/Basic/SourceManager.cpp:1594
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
Can this actually be`null`?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik updated this revision to Diff 198654.
nik edited the summary of this revision.
nik added a comment.
Rebased for current trunk.
If I miss something obvious, please tell me. Otherwise I'm waiting.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
Files:
include/clang/Basic/DiagnosticLexKinds.td
lib/Basic/SourceManager.cpp
lib/Lex/PPDirectives.cpp
test/Index/preamble-cyclic-include.cpp
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test
-test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a
preamble
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,18 @@
return {ImportAction::None};
}
+ // Check for circular inclusion of the main file.
+ // We can't generate a consistent preamble with regard to the conditional
+ // stack if the main file is included again as due to the preamble bounds
+ // some directives (e.g. #endif of a header guard) will never be seen.
+ // Since this will lead to confusing errors, avoid the inclusion.
+ if (File && PreambleConditionalStack.isRecording() &&
+ SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_for_preamble);
+return {ImportAction::None};
+ }
+
// Should we enter the source file? Set to Skip if either the source file is
// known to have no effect beyond its effect on module visibility -- that is,
// if it's got an include guard that is already defined, set to Import if it
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1591,7 +1591,7 @@
// as the main file.
const FileEntry *MainFile = MainContentCache->OrigEntry;
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
if (SourceFileUID) {
if (Optional MainFileUID =
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -426,6 +426,8 @@
"did not find header '%0' in framework '%1' (loaded from '%2')">;
def err_pp_error_opening_file : Error<
"error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_for_preamble : Error<
+ "main file cannot be included recursively when building a preamble">;
def err_pp_empty_filename : Error<"empty filename">;
def err_pp_include_too_deep : Error<"#include nested too deeply">;
def err_pp_expects_filename : Error<"expected \"FILENAME\" or ">;
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a preamble
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,18 @@
return {ImportAction::None};
}
+ // Check for circular inclusion of the main file.
+ // We can't generate a consistent preamble with regard to the conditional
+ // stack if the main file is included again as due to the preamble bounds
+ // some directives (e.g. #endif of a header guard) will never be seen.
+ // Since this will lead to confusing errors, avoid the inclusion.
+ if (File && PreambleConditionalStack.isRecording() &&
+ SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_for_preamble);
+return {ImportAction::None};
+ }
+
// Should we enter the source file? Set to Skip if either the source file is
// known to have no effect beyond its
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik added a comment. Ping. Ilya? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
