[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai added a comment. Thanks everyone for reviewing the change. Repository: rL LLVM https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
This revision was automatically updated to reflect the committed changes. Closed by commit rL337953: [Preprocessor] Stop entering included files after hitting a fatal error. (authored by vsapsai, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D48786?vs=156911=157336#toc Repository: rL LLVM https://reviews.llvm.org/D48786 Files: cfe/trunk/lib/Lex/PPDirectives.cpp cfe/trunk/test/Preprocessor/Inputs/cycle/a.h cfe/trunk/test/Preprocessor/Inputs/cycle/b.h cfe/trunk/test/Preprocessor/Inputs/cycle/c.h cfe/trunk/test/Preprocessor/include-cycle.c Index: cfe/trunk/test/Preprocessor/Inputs/cycle/c.h === --- cfe/trunk/test/Preprocessor/Inputs/cycle/c.h +++ cfe/trunk/test/Preprocessor/Inputs/cycle/c.h @@ -0,0 +1 @@ +#include "a.h" Index: cfe/trunk/test/Preprocessor/Inputs/cycle/b.h === --- cfe/trunk/test/Preprocessor/Inputs/cycle/b.h +++ cfe/trunk/test/Preprocessor/Inputs/cycle/b.h @@ -0,0 +1 @@ +#include "a.h" Index: cfe/trunk/test/Preprocessor/Inputs/cycle/a.h === --- cfe/trunk/test/Preprocessor/Inputs/cycle/a.h +++ cfe/trunk/test/Preprocessor/Inputs/cycle/a.h @@ -0,0 +1,8 @@ +// Presence of 2 inclusion cycles +//b.h -> a.h -> b.h -> ... +//c.h -> a.h -> c.h -> ... +// makes it unfeasible to reach max inclusion depth in all possible ways. Need +// to stop earlier. + +#include "b.h" +#include "c.h" Index: cfe/trunk/test/Preprocessor/include-cycle.c === --- cfe/trunk/test/Preprocessor/include-cycle.c +++ cfe/trunk/test/Preprocessor/include-cycle.c @@ -0,0 +1,5 @@ +// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s + +// Test that preprocessing terminates even if we have inclusion cycles. + +#include "cycle/a.h" Index: cfe/trunk/lib/Lex/PPDirectives.cpp === --- cfe/trunk/lib/Lex/PPDirectives.cpp +++ cfe/trunk/lib/Lex/PPDirectives.cpp @@ -1896,6 +1896,12 @@ if (PPOpts->SingleFileParseMode) ShouldEnter = false; + // Any diagnostics after the fatal error will not be visible. As the + // compilation failed already and errors in subsequently included files won't + // be visible, avoid preprocessing those files. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) +ShouldEnter = false; + // Determine whether we should try to import the module for this #include, if // there is one. Don't do so if precompiled module support is disabled or we // are processing this module textually (because we're building the module). Index: cfe/trunk/test/Preprocessor/Inputs/cycle/c.h === --- cfe/trunk/test/Preprocessor/Inputs/cycle/c.h +++ cfe/trunk/test/Preprocessor/Inputs/cycle/c.h @@ -0,0 +1 @@ +#include "a.h" Index: cfe/trunk/test/Preprocessor/Inputs/cycle/b.h === --- cfe/trunk/test/Preprocessor/Inputs/cycle/b.h +++ cfe/trunk/test/Preprocessor/Inputs/cycle/b.h @@ -0,0 +1 @@ +#include "a.h" Index: cfe/trunk/test/Preprocessor/Inputs/cycle/a.h === --- cfe/trunk/test/Preprocessor/Inputs/cycle/a.h +++ cfe/trunk/test/Preprocessor/Inputs/cycle/a.h @@ -0,0 +1,8 @@ +// Presence of 2 inclusion cycles +//b.h -> a.h -> b.h -> ... +//c.h -> a.h -> c.h -> ... +// makes it unfeasible to reach max inclusion depth in all possible ways. Need +// to stop earlier. + +#include "b.h" +#include "c.h" Index: cfe/trunk/test/Preprocessor/include-cycle.c === --- cfe/trunk/test/Preprocessor/include-cycle.c +++ cfe/trunk/test/Preprocessor/include-cycle.c @@ -0,0 +1,5 @@ +// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s + +// Test that preprocessing terminates even if we have inclusion cycles. + +#include "cycle/a.h" Index: cfe/trunk/lib/Lex/PPDirectives.cpp === --- cfe/trunk/lib/Lex/PPDirectives.cpp +++ cfe/trunk/lib/Lex/PPDirectives.cpp @@ -1896,6 +1896,12 @@ if (PPOpts->SingleFileParseMode) ShouldEnter = false; + // Any diagnostics after the fatal error will not be visible. As the + // compilation failed already and errors in subsequently included files won't + // be visible, avoid preprocessing those files. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) +ShouldEnter = false; + // Determine whether we should try to import the module for this #include, if // there is one. Don't do so if precompiled module support is disabled or we // are processing this module textually (because we're building the module). ___ cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai updated this revision to Diff 156911. vsapsai added a comment. - Tweak the comment according to review comments. Undiscussed changes: we don't stop preprocessing entirely, only in included files; not using better-sounding "skip" deliberately as it might be confused with `FileSkipped` API. https://reviews.llvm.org/D48786 Files: clang/lib/Lex/PPDirectives.cpp clang/test/Preprocessor/Inputs/cycle/a.h clang/test/Preprocessor/Inputs/cycle/b.h clang/test/Preprocessor/Inputs/cycle/c.h clang/test/Preprocessor/include-cycle.c Index: clang/test/Preprocessor/include-cycle.c === --- /dev/null +++ clang/test/Preprocessor/include-cycle.c @@ -0,0 +1,5 @@ +// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s + +// Test that preprocessing terminates even if we have inclusion cycles. + +#include "cycle/a.h" Index: clang/test/Preprocessor/Inputs/cycle/c.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/c.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/b.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/b.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/a.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/a.h @@ -0,0 +1,8 @@ +// Presence of 2 inclusion cycles +//b.h -> a.h -> b.h -> ... +//c.h -> a.h -> c.h -> ... +// makes it unfeasible to reach max inclusion depth in all possible ways. Need +// to stop earlier. + +#include "b.h" +#include "c.h" Index: clang/lib/Lex/PPDirectives.cpp === --- clang/lib/Lex/PPDirectives.cpp +++ clang/lib/Lex/PPDirectives.cpp @@ -1871,6 +1871,12 @@ if (PPOpts->SingleFileParseMode) ShouldEnter = false; + // Any diagnostics after the fatal error will not be visible. As the + // compilation failed already and errors in subsequently included files won't + // be visible, avoid preprocessing those files. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) +ShouldEnter = false; + // Determine whether we should try to import the module for this #include, if // there is one. Don't do so if precompiled module support is disabled or we // are processing this module textually (because we're building the module). Index: clang/test/Preprocessor/include-cycle.c === --- /dev/null +++ clang/test/Preprocessor/include-cycle.c @@ -0,0 +1,5 @@ +// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s + +// Test that preprocessing terminates even if we have inclusion cycles. + +#include "cycle/a.h" Index: clang/test/Preprocessor/Inputs/cycle/c.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/c.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/b.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/b.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/a.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/a.h @@ -0,0 +1,8 @@ +// Presence of 2 inclusion cycles +//b.h -> a.h -> b.h -> ... +//c.h -> a.h -> c.h -> ... +// makes it unfeasible to reach max inclusion depth in all possible ways. Need +// to stop earlier. + +#include "b.h" +#include "c.h" Index: clang/lib/Lex/PPDirectives.cpp === --- clang/lib/Lex/PPDirectives.cpp +++ clang/lib/Lex/PPDirectives.cpp @@ -1871,6 +1871,12 @@ if (PPOpts->SingleFileParseMode) ShouldEnter = false; + // Any diagnostics after the fatal error will not be visible. As the + // compilation failed already and errors in subsequently included files won't + // be visible, avoid preprocessing those files. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) +ShouldEnter = false; + // Determine whether we should try to import the module for this #include, if // there is one. Don't do so if precompiled module support is disabled or we // are processing this module textually (because we're building the module). ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:1875 + // Stop further preprocessing if a fatal error has occurred. Any diagnostics + // we might have raised will not be visible. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) jkorous wrote: > vsapsai wrote: > > jkorous wrote: > > > I am not sure I understand this - does that mean that we are not > > > displaying diagnostics that were produced before "now"? > > I can see how the wording can cause the confusion. But I'm not entirely > > sure it is misguiding, I've copy-pasted it from > > [Sema::InstantiatingTemplate::InstantiatingTemplate > > ](https://github.com/llvm-mirror/clang/blob/580f7daabc7696d50ad09d9643b2afeadbd387d8/lib/Sema/SemaTemplateInstantiate.cpp#L218-L220). > > Let me explain my reasoning in a different way to see if it makes sense. > > Entering a file is observable if it produces diagnostics or some other > > build artifact (object file in most cases). So when we encounter a fatal > > error, there is no visible indication of entering subsequent files. That's > > why we can skip entering those files: no difference in output and less work > > to do. > Thanks for the explanation! I was not sure whether "only subsequent" OR "both > subsequent and some/all prior" raised diagnostics would be not visible (could > be just my shitty English though). > > Maybe something along this line "any eventual subsequent diagnostics will not > be visible" would be more clear? Curiously, the comment kinda means both. Some prior diagnostics might be invisible but in this case we care about subsequent diagnostics. I'll update the comment to make that more clear. https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
bruno accepted this revision. bruno added a comment. Thanks for working on this. LGTM with improvements in the comments as mentioned by @jkorous https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM as well https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM with just a nit about comment wording. Thanks for the patch! Comment at: clang/lib/Lex/PPDirectives.cpp:1875 + // Stop further preprocessing if a fatal error has occurred. Any diagnostics + // we might have raised will not be visible. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) vsapsai wrote: > jkorous wrote: > > I am not sure I understand this - does that mean that we are not displaying > > diagnostics that were produced before "now"? > I can see how the wording can cause the confusion. But I'm not entirely sure > it is misguiding, I've copy-pasted it from > [Sema::InstantiatingTemplate::InstantiatingTemplate > ](https://github.com/llvm-mirror/clang/blob/580f7daabc7696d50ad09d9643b2afeadbd387d8/lib/Sema/SemaTemplateInstantiate.cpp#L218-L220). > Let me explain my reasoning in a different way to see if it makes sense. > Entering a file is observable if it produces diagnostics or some other build > artifact (object file in most cases). So when we encounter a fatal error, > there is no visible indication of entering subsequent files. That's why we > can skip entering those files: no difference in output and less work to do. Thanks for the explanation! I was not sure whether "only subsequent" OR "both subsequent and some/all prior" raised diagnostics would be not visible (could be just my shitty English though). Maybe something along this line "any eventual subsequent diagnostics will not be visible" would be more clear? https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:1875 + // Stop further preprocessing if a fatal error has occurred. Any diagnostics + // we might have raised will not be visible. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) jkorous wrote: > I am not sure I understand this - does that mean that we are not displaying > diagnostics that were produced before "now"? I can see how the wording can cause the confusion. But I'm not entirely sure it is misguiding, I've copy-pasted it from [Sema::InstantiatingTemplate::InstantiatingTemplate ](https://github.com/llvm-mirror/clang/blob/580f7daabc7696d50ad09d9643b2afeadbd387d8/lib/Sema/SemaTemplateInstantiate.cpp#L218-L220). Let me explain my reasoning in a different way to see if it makes sense. Entering a file is observable if it produces diagnostics or some other build artifact (object file in most cases). So when we encounter a fatal error, there is no visible indication of entering subsequent files. That's why we can skip entering those files: no difference in output and less work to do. https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
jkorous added a reviewer: jkorous. jkorous added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:1875 + // Stop further preprocessing if a fatal error has occurred. Any diagnostics + // we might have raised will not be visible. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) I am not sure I understand this - does that mean that we are not displaying diagnostics that were produced before "now"? https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai added a comment. Ping. https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai added a comment. Ping. https://reviews.llvm.org/D48786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.
vsapsai created this revision. vsapsai added reviewers: bruno, rsmith. Herald added a subscriber: dexonsmith. Fixes a problem when we have multiple inclusion cycles and try to enumerate all possible ways to reach the max inclusion depth. rdar://problem/38871876 https://reviews.llvm.org/D48786 Files: clang/lib/Lex/PPDirectives.cpp clang/test/Preprocessor/Inputs/cycle/a.h clang/test/Preprocessor/Inputs/cycle/b.h clang/test/Preprocessor/Inputs/cycle/c.h clang/test/Preprocessor/include-cycle.c Index: clang/test/Preprocessor/include-cycle.c === --- /dev/null +++ clang/test/Preprocessor/include-cycle.c @@ -0,0 +1,5 @@ +// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s + +// Test that preprocessing terminates even if we have inclusion cycles. + +#include "cycle/a.h" Index: clang/test/Preprocessor/Inputs/cycle/c.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/c.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/b.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/b.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/a.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/a.h @@ -0,0 +1,8 @@ +// Presence of 2 inclusion cycles +//b.h -> a.h -> b.h -> ... +//c.h -> a.h -> c.h -> ... +// makes it unfeasible to reach max inclusion depth in all possible ways. Need +// to stop earlier. + +#include "b.h" +#include "c.h" Index: clang/lib/Lex/PPDirectives.cpp === --- clang/lib/Lex/PPDirectives.cpp +++ clang/lib/Lex/PPDirectives.cpp @@ -1871,6 +1871,11 @@ if (PPOpts->SingleFileParseMode) ShouldEnter = false; + // Stop further preprocessing if a fatal error has occurred. Any diagnostics + // we might have raised will not be visible. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) +ShouldEnter = false; + // Determine whether we should try to import the module for this #include, if // there is one. Don't do so if precompiled module support is disabled or we // are processing this module textually (because we're building the module). Index: clang/test/Preprocessor/include-cycle.c === --- /dev/null +++ clang/test/Preprocessor/include-cycle.c @@ -0,0 +1,5 @@ +// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s + +// Test that preprocessing terminates even if we have inclusion cycles. + +#include "cycle/a.h" Index: clang/test/Preprocessor/Inputs/cycle/c.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/c.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/b.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/b.h @@ -0,0 +1 @@ +#include "a.h" Index: clang/test/Preprocessor/Inputs/cycle/a.h === --- /dev/null +++ clang/test/Preprocessor/Inputs/cycle/a.h @@ -0,0 +1,8 @@ +// Presence of 2 inclusion cycles +//b.h -> a.h -> b.h -> ... +//c.h -> a.h -> c.h -> ... +// makes it unfeasible to reach max inclusion depth in all possible ways. Need +// to stop earlier. + +#include "b.h" +#include "c.h" Index: clang/lib/Lex/PPDirectives.cpp === --- clang/lib/Lex/PPDirectives.cpp +++ clang/lib/Lex/PPDirectives.cpp @@ -1871,6 +1871,11 @@ if (PPOpts->SingleFileParseMode) ShouldEnter = false; + // Stop further preprocessing if a fatal error has occurred. Any diagnostics + // we might have raised will not be visible. + if (ShouldEnter && Diags->hasFatalErrorOccurred()) +ShouldEnter = false; + // Determine whether we should try to import the module for this #include, if // there is one. Don't do so if precompiled module support is disabled or we // are processing this module textually (because we're building the module). ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
