[PP] Handle #include_next after include found relative to current one same as GCC

[DmitryPolukhin]

Summary

GCC clears current search position in the include directories list used
for #include_next if current header was found using relative paths.
Before this patch Clang kept current position from previous header
included from directories list. Both approaches make sense but I thin it
is better to be GCC compatible in this question. See added test-case for
the example.

Also update warning text to give better understanding what happened.
Previous one about absolute path was incorrect and confusing.

[DmitryPolukhin]

Richard, friendly ping, PTAL!

[rsmith]

I'm a little concerned about the possibility of this breaking uses of this feature on platforms where Clang is the system compiler. For instance, this pattern would be broken by your change:

// stddef.h
#include "stddef-helper.h"

// stddef-helper.h
#include_next <stddef.h>

Conversely, I don't think any important library is likely to be relying on the GCC behavior, because compilations with gcc -I- would effectively get the current Clang behavior (because relative-looking paths would be found in the relevant include search path rather than as relative paths).

Is there some way we can gain confidence we're not breaking things here?

include/clang/Basic/DiagnosticLexKinds.td

268–269

Can you distinguish between the two cases here and give better diagnostics for both?

"#include_next in file found by relative path, searching from [...]"
"#include_next with absolute path, searching from [...]"

would be an improvement on this.

[DmitryPolukhin]

In D18641#413277, @rsmith wrote:

I'm a little concerned about the possibility of this breaking uses of this feature on platforms where Clang is the system compiler. For instance, this pattern would be broken by your change:

// stddef.h
#include "stddef-helper.h"

// stddef-helper.h
#include_next <stddef.h>

Conversely, I don't think any important library is likely to be relying on the GCC behavior, because compilations with gcc -I- would effectively get the current Clang behavior (because relative-looking paths would be found in the relevant include search path rather than as relative paths).

The same is true for Clang with my patch i.e. use clang -iquote to get almost old behavior. So IMHO on the system where Clang is a replacement for GCC it is better to be GCC compatible in such basic things as include behavior.

Is there some way we can gain confidence we're not breaking things here?

I don't know how we can get such confidence without committing code and see if any issues arrive. On systems where GCC is default compiler I tested my patch on wide range of scenarios and there were no problems.

Please let me know if you would like to proceed with this patch or I should abandon it.

[DmitryPolukhin]

It seems that there is no interest in this patch, abandon.

include/clang/Basic/DiagnosticLexKinds.td

 def err_pp_hash_error : Error<"%0">;
 }
 
 def pp_include_next_in_primary : Warning<
   "#include_next in primary source file">,
   InGroup<DiagGroup<"include-next-outside-header">>;
 def pp_include_macros_out_of_predefines : Error<
   "the #__include_macros directive is only for internal use by -imacros">;
-def pp_include_next_absolute_path : Warning<
-  "#include_next with absolute path">,
+def pp_include_next_without_state : Warning<
+  "#include_next without previous state, searching from the beginning of the include directories list">,
   InGroup<DiagGroup<"include-next-absolute-path">>;
 def ext_c99_whitespace_required_after_macro_name : ExtWarn<
   "ISO C99 requires whitespace after the macro name">, InGroup<C99>;
 def ext_missing_whitespace_after_macro_name : ExtWarn<
   "whitespace required after macro name">;
 def warn_missing_whitespace_after_macro_name : Warning<
   "whitespace recommended after macro name">;
   

lib/Lex/HeaderSearch.cpp

const FileEntry *HeaderSearch::LookupFile(
     StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
     const DirectoryLookup *FromDir, const DirectoryLookup *&CurDir,
     ArrayRef<std::pair<const FileEntry *, const DirectoryEntry *>> Includers,
     SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath,
     Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
     bool SkipCache) {
   if (SuggestedModule)
     *SuggestedModule = ModuleMap::KnownHeader();
-    
-  // If 'Filename' is absolute, check to see if it exists and no searching.
-  if (llvm::sys::path::is_absolute(Filename)) {
-    CurDir = nullptr;
+  // Reset CurDir for subsequent include_next searches. It needs to be done in
+  // both cases absolute patch and ""-include, GCC does the same.
 
+  // If 'Filename' is absolute, check to see if it exists and no searching.
+  if (llvm::sys::path::is_absolute(Filename)) {
     // If this was an #include_next "/absolute/file", fail.
     if (FromDir) return nullptr;
 
     if (SearchPath)
       SearchPath->clear();
     if (RelativePath) {
       RelativePath->clear();
       RelativePath->append(Filename.begin(), Filename.end());
for (const auto &IncluderAndDir : Includers) {
           }
           break;
         }
       }
       First = false;
     }
   }
 
-  CurDir = nullptr;
-
   // If this is a system #include, ignore the user #include locs.
   unsigned i = isAngled ? AngledDirIdx : 0;
 
   // If this is a #include_next request, start searching after the directory the
   // file was found in.
   if (FromDir)
     i = FromDir-&SearchDirs[0];
 

lib/Lex/PPDirectives.cpp

if (isInPrimaryFile()) {
     Diag(IncludeNextTok, diag::pp_include_next_in_primary);
   } else if (CurSubmodule) {
     // Start looking up in the directory *after* the one in which the current
     // file would be found, if any.
     assert(CurPPLexer && "#include_next directive in macro?");
     LookupFromFile = CurPPLexer->getFileEntry();
     Lookup = nullptr;
   } else if (!Lookup) {
-    Diag(IncludeNextTok, diag::pp_include_next_absolute_path);
+    Diag(IncludeNextTok, diag::pp_include_next_without_state);
   } else {
     // Start looking up in the next directory.
     ++Lookup;
   }
 
   return HandleIncludeDirective(HashLoc, IncludeNextTok, Lookup,
                                 LookupFromFile);
 }

lib/Lex/PPMacroExpansion.cpp

if (PP.isInPrimaryFile()) {
     PP.Diag(Tok, diag::pp_include_next_in_primary);
   } else if (PP.getCurrentSubmodule()) {
     // Start looking up in the directory *after* the one in which the current
     // file would be found, if any.
     assert(PP.getCurrentLexer() && "#include_next directive in macro?");
     LookupFromFile = PP.getCurrentLexer()->getFileEntry();
     Lookup = nullptr;
   } else if (!Lookup) {
-    PP.Diag(Tok, diag::pp_include_next_absolute_path);
+    PP.Diag(Tok, diag::pp_include_next_without_state);
   } else {
     // Start looking up in the next directory.
     ++Lookup;
   }
 
   return EvaluateHasIncludeCommon(Tok, II, PP, Lookup, LookupFromFile);
 }
 

test/Modules/Inputs/include_next/x/d.h

+enum { dx = 7 };

test/Modules/Inputs/include_next/y/c.h

+#include "d.h"
+enum { cy = 5 };

test/Modules/Inputs/include_next/y/d.h

+#include_next <d.h>
+// expected-warning@-1 {{#include_next without previous state, searching from the beginning of the include directories list}}
+enum { dy = 6 };

test/Modules/include_next.c

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -I%S/Inputs/include_next/x -I%S/Inputs/include_next/y -verify %s
 // RUN: %clang_cc1 -I%S/Inputs/include_next/x -I%S/Inputs/include_next/y -verify %s -fmodules -fimplicit-module-maps -fmodules-cache-path=%t
 
-// expected-no-diagnostics
 #include "a.h"
 #include "subdir/b.h"
+#include <c.h>
+
 _Static_assert(ax == 1, "");
 _Static_assert(ay == 2, "");
 _Static_assert(bx == 3, "");
 _Static_assert(by == 4, "");
+_Static_assert(cy == 5, "");
+_Static_assert(dy == 6, "");
+_Static_assert(dx == 7, "");