[PATCH] D134654: [clang] Detect header loops

Tue Oct 11 05:03:19 PDT 2022

urnathan added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:323
   "whitespace recommended after macro name">;
+def warn_include_cycle : Warning<"#include cycle">,
+   InGroup<DiagGroup<"include-cycle">>, DefaultIgnore;
----------------
philnik wrote:
> aaron.ballman wrote:
> > urnathan wrote:
> > > aaron.ballman wrote:
> > > > This diagnostic doesn't really seem like it's going to help the user to know what's wrong with their code or how to fix it. (Also, the notes @erichkeane was hoping were emitted don't seem to be, at least according to the new test cases.) I think we need to give users a bit more guidance here.
> > > The test infrastructire ignores the 'included from ' chain, which is emitted.  What do you suggest?
> > Even posting the "included from" doesn't help clarify what's wrong with the code though, if I'm understanding the tests properly. My concern is with code like:
> > ```
> > // self.h
> > #ifndef GUARD
> > #define GUARD
> > 
> > #include "self.h" // expected-warning {{#include cycle}}
> > #endif
> > ```
> > (This is effectively the same test as `warn-loop-macro-1.h`.) There is no include *cycle* ("a series of events that are regularly repeated in the same order.") in this code -- the header is included for the first time, `GUARD` is not defined, then `GUARD` is defined and the header is included for the second time. This time `GUARD` is defined and no further cycles into `self.h` occurs.
> > 
> > But I think I'm seeing now what you're trying to get at (correct me if I'm wrong): when processing an include stack, opening the same header file twice is a problem (regardless of the contents of the header or how you got into the same file twice)? If that's accurate, would it make more sense to diagnose this as:
> > 
> > `including the same header (%0) more than once in an include stack causes %select{incorrect behavior of Clang modules|the header to not be a C++ module header unit}1`
> > 
> > or something along those lines? Basically, I'd like to avoid saying "cycle" because that starts to sound like "you have potentially infinite recursion in the include stack" and I'd like to add information about what's actually wrong instead of pointing out what the code does and hoping the user knows why that's bad.
> FWIW in libc++ we have a script to check that we don't include the header more than once in a stack and also call it a cycle, so there is some precedent of calling it an include cycle, even thought it's technically not a cycle.
Aaron, correct, the problem is with clang-modules/c++ header units.  The loop means that the header file does not expand to the same sequence of tokens at every inclusion.  This is particularly problematic with module maps, because the compiler will automatically start turning a header into a clang-module upon the first #include -- which might not be at the outermost level, (if it starts with a header at some other position in the loop).  Then when it starts with this header, it'll think it already has converted and loaded it.  One ends up with very obscure failure modes (such as link errors concerning missing template instantations).

I wonder if (in addition to allow detecting this in general), the warning should be enabled by default when building a header-unit/clang-module and one reincludes a header corresponding to a module currently being built.  That's the same check, because there can be a stack of modules being built, but I think we need some additional checking to avoid triggering on 'textual headers' from the module map.

IMHO it is still a cycle, it's a closed loop that executes exactly once :)  I can see the confusion though.  I mean, 'for (auto i = 0; i < 1; i++) {}' is still a loop, right?  just not an unbounded one. 

 Anyway, I think your diagagnostic text is better, thanks!

================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:323
   "whitespace recommended after macro name">;
+def warn_include_cycle : Warning<"#include cycle">,
+   InGroup<DiagGroup<"include-cycle">>, DefaultIgnore;
----------------
urnathan wrote:
> philnik wrote:
> > aaron.ballman wrote:
> > > urnathan wrote:
> > > > aaron.ballman wrote:
> > > > > This diagnostic doesn't really seem like it's going to help the user to know what's wrong with their code or how to fix it. (Also, the notes @erichkeane was hoping were emitted don't seem to be, at least according to the new test cases.) I think we need to give users a bit more guidance here.
> > > > The test infrastructire ignores the 'included from ' chain, which is emitted.  What do you suggest?
> > > Even posting the "included from" doesn't help clarify what's wrong with the code though, if I'm understanding the tests properly. My concern is with code like:
> > > ```
> > > // self.h
> > > #ifndef GUARD
> > > #define GUARD
> > > 
> > > #include "self.h" // expected-warning {{#include cycle}}
> > > #endif
> > > ```
> > > (This is effectively the same test as `warn-loop-macro-1.h`.) There is no include *cycle* ("a series of events that are regularly repeated in the same order.") in this code -- the header is included for the first time, `GUARD` is not defined, then `GUARD` is defined and the header is included for the second time. This time `GUARD` is defined and no further cycles into `self.h` occurs.
> > > 
> > > But I think I'm seeing now what you're trying to get at (correct me if I'm wrong): when processing an include stack, opening the same header file twice is a problem (regardless of the contents of the header or how you got into the same file twice)? If that's accurate, would it make more sense to diagnose this as:
> > > 
> > > `including the same header (%0) more than once in an include stack causes %select{incorrect behavior of Clang modules|the header to not be a C++ module header unit}1`
> > > 
> > > or something along those lines? Basically, I'd like to avoid saying "cycle" because that starts to sound like "you have potentially infinite recursion in the include stack" and I'd like to add information about what's actually wrong instead of pointing out what the code does and hoping the user knows why that's bad.
> > FWIW in libc++ we have a script to check that we don't include the header more than once in a stack and also call it a cycle, so there is some precedent of calling it an include cycle, even thought it's technically not a cycle.
> Aaron, correct, the problem is with clang-modules/c++ header units.  The loop means that the header file does not expand to the same sequence of tokens at every inclusion.  This is particularly problematic with module maps, because the compiler will automatically start turning a header into a clang-module upon the first #include -- which might not be at the outermost level, (if it starts with a header at some other position in the loop).  Then when it starts with this header, it'll think it already has converted and loaded it.  One ends up with very obscure failure modes (such as link errors concerning missing template instantations).
> 
> I wonder if (in addition to allow detecting this in general), the warning should be enabled by default when building a header-unit/clang-module and one reincludes a header corresponding to a module currently being built.  That's the same check, because there can be a stack of modules being built, but I think we need some additional checking to avoid triggering on 'textual headers' from the module map.
> 
> IMHO it is still a cycle, it's a closed loop that executes exactly once :)  I can see the confusion though.  I mean, 'for (auto i = 0; i < 1; i++) {}' is still a loop, right?  just not an unbounded one. 
> 
>  Anyway, I think your diagagnostic text is better, thanks!
philnik, that's good to know.  

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134654/new/

https://reviews.llvm.org/D134654