[clang] Always allow including builtin headers in [no_undeclared_headers] modules.

[mboehme]

Summary

Previously, this would fail if the builtin headers had been "claimed" by a different module that wraps these builtin headers. libc++ does this, for example.

This change adds a test demonstrating this situation; the test fails without the fix.

[mboehme]

Temporarily retracting from review as I've realized there's another case I need to take care of.

[mboehme]

When testing with the real libc++ and glibc, I realized that I was still getting a cyclic dependency between the two modules because the builtin stddef.h that glibc was including was still being ascribed to libc++.

This modified version fixes this and also updates the test to more closely reflect the real libc++ and glibc so that my previous version of the fix also fails with a cyclic dependency.

[mboehme]

Committed as https://github.com/llvm/llvm-project/commit/8d74de9de6d6cca552d7de7d0bfd36b6dd7d58dc.

I thought I had added a "Differential Revision:" to the commit message, but something must have gone wrong.

clang/include/clang/Lex/ModuleMap.h

public:
 
   /// Get the directory that contains Clang-supplied include files.
   const DirectoryEntry *getBuiltinDir() const {
     return BuiltinIncludeDir;
   }
 
   /// Is this a compiler builtin header?
   static bool isBuiltinHeader(StringRef FileName);
+  bool isBuiltinHeader(const FileEntry *File);
 
   /// Add a module map callback.
   void addModuleMapCallbacks(std::unique_ptr<ModuleMapCallbacks> Callback) {
     Callbacks.push_back(std::move(Callback));
   }
 
   /// Retrieve the module that owns the given header file, if any. Note that
   /// this does not implicitly load module maps, except for builtin headers,

clang/lib/Lex/HeaderSearch.cpp

auto TryEnterImported = [&]() -> bool {
     //
     //    module stddef {
     //      header "/path_to_builtin_dirs/stddef.h"
     //      textual "stddef.h"
     //    }
     //
     // It's common that libc++ and system modules will both define such
     // submodules. Make sure cached results for a builtin header won't
-    // prevent other builtin modules to potentially enter the builtin header.
-    // Note that builtins are header guarded and the decision to actually
-    // enter them is postponed to the controlling macros logic below.
+    // prevent other builtin modules from potentially entering the builtin
+    // header. Note that builtins are header guarded and the decision to
+    // actually enter them is postponed to the controlling macros logic below.
     bool TryEnterHdr = false;
     if (FileInfo.isCompilingModuleHeader && FileInfo.isModuleHeader)
-      TryEnterHdr = File->getDir() == ModMap.getBuiltinDir() &&
-                    ModuleMap::isBuiltinHeader(
-                        llvm::sys::path::filename(File->getName()));
+      TryEnterHdr = ModMap.isBuiltinHeader(File);
 
     // Textual headers can be #imported from different modules. Since ObjC
     // headers find in the wild might rely only on #import and do not contain
     // controlling macros, be conservative and only try to enter textual headers
     // if such macro is present.
     if (!FileInfo.isModuleHeader &&
         FileInfo.getControllingMacro(ExternalLookup))
       TryEnterHdr = true;
HeaderSearch::findAllModulesForHeader(const FileEntry *File) const {
   return ModMap.findAllModulesForHeader(File);
 }
 
 static bool suggestModule(HeaderSearch &HS, const FileEntry *File,
                           Module *RequestingModule,
                           ModuleMap::KnownHeader *SuggestedModule) {
   ModuleMap::KnownHeader Module =
       HS.findModuleForHeader(File, /*AllowTextual*/true);
-  if (SuggestedModule)
-    *SuggestedModule = (Module.getRole() & ModuleMap::TextualHeader)
-                           ? ModuleMap::KnownHeader()
-                           : Module;
 
   // If this module specifies [no_undeclared_includes], we cannot find any
   // file that's in a non-dependency module.
   if (RequestingModule && Module && RequestingModule->NoUndeclaredIncludes) {
-    HS.getModuleMap().resolveUses(RequestingModule, /*Complain*/false);
+    HS.getModuleMap().resolveUses(RequestingModule, /*Complain*/ false);
     if (!RequestingModule->directlyUses(Module.getModule())) {
+      // Builtin headers are a special case. Multiple modules can use the same
+      // builtin as a modular header (see also comment in
+      // ShouldEnterIncludeFile()), so the builtin header may have been
+      // "claimed" by an unrelated module. This shouldn't prevent us from
+      // including the builtin header textually in this module.
+      if (HS.getModuleMap().isBuiltinHeader(File)) {
+        if (SuggestedModule)
+          *SuggestedModule = ModuleMap::KnownHeader();
+        return true;
+      }
       return false;
     }
   }
 
+  if (SuggestedModule)
+    *SuggestedModule = (Module.getRole() & ModuleMap::TextualHeader)
+                           ? ModuleMap::KnownHeader()
+                           : Module;
+
   return true;
 }
 
 bool HeaderSearch::findUsableModuleForHeader(
     const FileEntry *File, const DirectoryEntry *Root, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) {
   if (File && needModuleLookup(RequestingModule, SuggestedModule)) {
     // If there is a module that corresponds to this header, suggest it.

clang/lib/Lex/ModuleMap.cpp

return llvm::StringSwitch<bool>(FileName)
            .Case("stdbool.h", true)
            .Case("stddef.h", true)
            .Case("stdint.h", true)
            .Case("tgmath.h", true)
            .Case("unwind.h", true)
            .Default(false);
 }
 
+bool ModuleMap::isBuiltinHeader(const FileEntry *File) {
+  return File->getDir() == BuiltinIncludeDir &&
+         ModuleMap::isBuiltinHeader(llvm::sys::path::filename(File->getName()));
+}
+
 ModuleMap::HeadersMap::iterator
 ModuleMap::findKnownHeader(const FileEntry *File) {
   resolveHeaderDirectives(File);
   HeadersMap::iterator Known = Headers.find(File);
   if (HeaderInfo.getHeaderSearchOpts().ImplicitModuleMaps &&
-      Known == Headers.end() && File->getDir() == BuiltinIncludeDir &&
-      ModuleMap::isBuiltinHeader(llvm::sys::path::filename(File->getName()))) {
+      Known == Headers.end() && ModuleMap::isBuiltinHeader(File)) {
     HeaderInfo.loadTopLevelSystemModules();
     return Headers.find(File);
   }
   return Known;
 }
 
 ModuleMap::KnownHeader
 ModuleMap::findHeaderInUmbrellaDirs(const FileEntry *File,

clang/test/Modules/Inputs/no-undeclared-includes-builtins/glibc/module.modulemap

+module glibc [system] [no_undeclared_includes] {
+  // glibc relies on the builtin stddef.h, so it has no stddef.h of its own.
+  header "stdio.h"
+  export *
+}

clang/test/Modules/Inputs/no-undeclared-includes-builtins/glibc/stdio.h

+#include <stddef.h>

clang/test/Modules/Inputs/no-undeclared-includes-builtins/libcxx/module.modulemap

+module libcxx [system] {
+  header "stddef.h"
+  header "stdio.h"
+  export *
+}

clang/test/Modules/Inputs/no-undeclared-includes-builtins/libcxx/stddef.h

+#include_next <stddef.h>

clang/test/Modules/Inputs/no-undeclared-includes-builtins/libcxx/stdio.h

+#include_next <stdio.h>

clang/test/Modules/no-undeclared-includes-builtins.cpp

+// Test that a [no_undeclared_headers] module can include builtin headers, even
+// if these have been "claimed" by a different module that wraps these builtin
+// headers. libc++ does this, for example.
+//
+// The test inputs used here replicates the relationship between libc++ and
+// glibc. When modularizing glibc, [no_undeclared_headers] must be used to
+// prevent glibc from including the libc++ versions of the C standard library
+// headers.
+
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/no-undeclared-includes-builtins/libcxx -I %S/Inputs/no-undeclared-includes-builtins/glibc %s
+// expected-no-diagnostics
+
+#include <stddef.h>