[PATCH] D159483: [Modules] Add a flag to control builtin headers being in system modules

[Ian Anderson via Phabricator via cfe-commits]

iana added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:2944
           [NoXarchOption], [ClangOption, CLOption]>>;
+def fbuiltin_headers_in_system_modules : Flag <["-"], "fbuiltin-headers-in-system-modules">,
+  Group<f_Group>,
----------------
I don't love this flag name, but I couldn't come up with anything more succinct. It actually does two things.

  # Turns on `ModuleMap::isBuiltinHeader` behavior, that is the builtin headers get added into the system modules.
  # Causes the module map parser to ignore all of the new builtin modules added in D159064. This is needed before the modules are added to assist with Apple internal staging with the Swift compiler.



================
Comment at: clang/include/clang/Driver/Options.td:2945-2946
+def fbuiltin_headers_in_system_modules : Flag <["-"], "fbuiltin-headers-in-system-modules">,
+  Group<f_Group>,
+  Flags<[NoXarchOption]>,
+  Visibility<[CC1Option]>,
----------------
I don't fully understand what these two do, but I think they apply here?


================
Comment at: clang/lib/Headers/__stddef_ptrdiff_t.h:10
 
-#if !defined(_PTRDIFF_T) || __has_feature(modules)
-/* Always define ptrdiff_t when modules are available. */
-#if !__has_feature(modules)
+#if !defined(_PTRDIFF_T) || __has_feature(builtin_headers_in_system_modules)
 #define _PTRDIFF_T
----------------
This is to support patterns like the Modules/stddef.c test. The module map looks like this.

```
module StdDef {
  module Other {
    header "other.h" // includes stddef.h
  }
  module PtrDiffT {
    header "ptrdiff_t.h" // defines __needs_ptrdiff_t and includes stddef.h
  }
  module IncludeAgain {
    header "include_again.h" // includes stddef.h
  }
}
```
With builtin_headers_in_system_modules, __stddef_ptrdiff_t.h will not be in a module. It will first be seen in other.h and define its header guard. When it's seen again in ptrdiff_t.h, it needs to declare ptrdiff_t again, so it needs to ignore its header guard.

Without builtin_headers_in_system_modules, __stddef_ptrdiff_t.h will be in the `_Builtin_stddef` module. It needs its header guard there so that it's only included once when building the module. Even though ptrdiff_t.h will see the header guard, it will still import `_Builtin_stddef.ptrdiff_t` and get the declaration.


================
Comment at: clang/lib/Headers/stddef.h:10
 
-#if !defined(__STDDEF_H) || defined(__need_ptrdiff_t) ||                       \
-    defined(__need_size_t) || defined(__need_rsize_t) ||                       \
-    defined(__need_wchar_t) || defined(__need_NULL) ||                         \
-    defined(__need_nullptr_t) || defined(__need_unreachable) ||                \
-    defined(__need_max_align_t) || defined(__need_offsetof) ||                 \
-    defined(__need_wint_t) ||                                                  \
-    (defined(__STDC_WANT_LIB_EXT1__) && __STDC_WANT_LIB_EXT1__ >= 1)
+#if !defined(__STDDEF_H) || __has_feature(modules) ||                          \
+    (defined(__STDC_WANT_LIB_EXT1__) && __STDC_WANT_LIB_EXT1__ >= 1) ||        \
----------------
As a `textual` header, this needs to ignore its header guard in module mode, even without builtin_headers_in_system_modules. Going back to the Modules/stddef.c test.

```
module StdDef {
  module Other {
    header "other.h" // includes stddef.h
    export *
  }
  module PtrDiffT {
    header "ptrdiff_t.h" // defines __needs_ptrdiff_t and includes stddef.h
  }
  module IncludeAgain {
    header "include_again.h" // includes stddef.h
    export *
  }
}
```

When other.h compiles, __STDDEF_H will get defined and `_Builtin_stddef.ptrdiff_t` will be imported. But when include_again.h compiles, __STDDEF_H will already be defined and `_Builtin_stddef.ptrdiff_t` won't be imported because the include will be skipped in stddef.h. That's ok when include_again.h is building because it will see ptrdiff_t from when other.h included it. But a client that only includes include_again.h won't see any types because `StdDef.IncludeAgain` skipped the contents of stddef.h and skipped importing `_Builtin_stddef.ptrdiff_t`, and so there's nothing for `export *` to export.

The other solution is to never define __STDDEF_H in module mode, but I think that's strange behavior. Several headers in the Apple SDKs test if a header was included by checking for its header guard (questionable, but it's what they do). stddef.h isn't one of those headers, but it still seems like it should define everything in modules mode that it defines without modules.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:259
+/// headers.
+static bool isBuiltinHeaderName(StringRef FileName) {
+  return llvm::StringSwitch<bool>(FileName)
----------------
This had to be moved up just because it's used by methods under here.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1540
+    /// or added to Map's Modules/ModuleScopeIDs).
+    bool IgnoreModules = false;
+
----------------
This an everything under it is gross, but the only other thing I could think of was to duplicate the module map and decided to load a special one for BuiltinHeadersInSystemModules. That seemed weirder.


================
Comment at: clang/test/Modules/Werror-Wsystem-headers.m:6
 // Initial module build
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN:     -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules \
+// RUN:     -fmodules-cache-path=%t -fdisable-module-hash -isystem %S/Inputs/System/usr/include \
----------------
Most of these tests aren't really affected by -fbuiltin-headers-in-system-modules, they just coincidentally use Inputs/System/usr/include and that needs -fbuiltin-headers-in-system-modules.


================
Comment at: clang/test/Modules/shadowed-submodule.m:2
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/shadowed-submodule/Foo -I %S/Inputs/shadowed-submodule/A2 %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I %S/Inputs/shadowed-submodule/Foo -I %S/Inputs/shadowed-submodule/A2 %s -verify
 
----------------
This could probably be redone to not use stdarg.h as the shadow and then it wouldn't require -fbuiltin-headers-in-system-modules, but it's probably ok as-is?


================
Comment at: clang/test/Modules/stddef.c:2
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify -fno-modules-error-recovery
 
----------------
This one and the next one will be updated in the next review to test without -fbuiltin-headers-in-system-modules too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483