Enforce module decl-use restrictions and private header restrictions in textual headers

[rsmith]

Summary

Per the documentation, these restrictions were intended to apply to textual headers but previously this didn't work because we decided there was no requesting module when the #include was in a textual header.

A -cc1 flag is provided to restore the old behavior for transitionary purposes.

[rsmith]

Add release notes.

[aaron.ballman]

The changes look reasonable to me, though if you think the flag is a temporary one, we might want to consider changes to document that explicitly.

clang/docs/ReleaseNotes.rst

141

You should mention -fno-modules-validate-textual-header-includes here so that users know there's a transition flag available. Do you expect we'd like to remove that flag in the future, or do you expect we'll need to carry it around "forever"?

[rsmith]

Document the flag to undo the behavior, and mention that it's going away.

clang/docs/ReleaseNotes.rst

141

I'm hopeful that we only need to keep the flag around while the affected users are transitioning. We know we need it inside Google because with this patch we find a bunch of layering issues that we thought we were already diagnosing, but weren't; I don't know how many other Clang users are actually making use of this part of the module maps functionality. My aim would be to remove the flag after the next release, if possible.

Extended documentation to mention the flag and that it's going away.

[aaron.ballman]

LGTM with a suggestion to also document the command line reference. Thanks!

clang/include/clang/Driver/Options.td

2280

[rsmith]

• Help text update from Aaron.

[jansvoboda11]

Hi @rsmith, this commit makes it possible for HeaderInfo::LookupFile() to be called with different RequestingModule within single CompilerInstance. This is problematic, since some modules may see headers other modules can't (due to [no_undeclared_includes]). This can permanently mess up contents of the lookup cache (HeaderSearch::LookupFileCache) that uses only the lookup name as the key. You can see the minimal reproducer below. On our side, we can work around this by using -fno-modules-validate-textual-header-includes, but I think this will need to be fixed before that options goes away.

// RUN: rm -rf %t
// RUN: split-file %s %t

//--- include/module.modulemap
module A [no_undeclared_includes] { textual header "A.h" }
module B { header "B.h" }
//--- include/A.h
#if __has_include(<B.h>)
#error Even textual headers within module A now inherit [no_undeclared_includes] \
       and thus do not have that include.
#endif
//--- include/B.h

//--- tu.c
#if !__has_include(<B.h>)
#error Main TU does have that include.
#endif

#include "A.h"

#if !__has_include(<B.h>)
#error Main TU still has that include.
// We hit the above because the unsuccessful __has_include check in A.h taints
// lookup cache (HeaderSearch::LookupFileCache) of this CompilerInstance.
#endif

// RUN: %clang_cc1 -I %t/include -fmodules -fimplicit-module-maps \
// RUN:   -fmodules-cache-path=%t/cache -fsyntax-only %t/tu.c

[rsmith]

In D132779#4523783, @jansvoboda11 wrote:

Hi @rsmith, this commit makes it possible for HeaderInfo::LookupFile() to be called with different RequestingModule within single CompilerInstance. This is problematic, since some modules may see headers other modules can't (due to [no_undeclared_includes]). This can permanently mess up contents of the lookup cache (HeaderSearch::LookupFileCache) that uses only the lookup name as the key.

Hm, interesting. I wonder if this was (subtly) broken before, given that it was previously possible to call LookupFile with different RequestingModules in a single compilation -- once with a null RequestingModule and once with the compilation's current module. In any case, presumably we should either ensure that the cached data is independent of the requesting module (eg, apply the decluse restriction after computing the iterator pair), or add the requesting module to the key, or only cache data for one specific value of RequestingModule. Maybe the simplest thing would be to use a single-item LRU cache here -- store the RequestingModule in the cached result and ignore the existing cached result if it doesn't match the current RequestingModule.

clang/docs/ReleaseNotes.rst

- no_sanitize("...") on a global variable for known but not relevant sanitizers
   is now just a warning. It now says that this will be ignored instead of
   incorrectly saying no_sanitize only applies to functions and methods.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
+- When using header modules, inclusion of a private header and violations of
+  the `use-declaration rules
+  <https://clang.llvm.org/docs/Modules.html#use-declaration>`_ are now
+  diagnosed even when the includer is a textual header. This change can be
+  temporarily reversed with ``-Xclang
+  -fno-modules-validate-textual-header-includes``, but this flag will be
+  removed in a future Clang release.
 
 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags
 -------------------------
 
 Modified Compiler Flags

clang/include/clang/Basic/LangOptions.def

BENIGN_ENUM_LANGOPT(CompilingModule, CompilingModuleKind, 3, CMK_None,
                     "compiling a module interface")
 BENIGN_LANGOPT(CompilingPCH, 1, 0, "building a pch")
 BENIGN_LANGOPT(BuildingPCHWithObjectFile, 1, 0, "building a pch which has a corresponding object file")
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
 BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "instantiate templates while building a PCH")
 COMPATIBLE_LANGOPT(ModulesDeclUse    , 1, 0, "require declaration of module uses")
 BENIGN_LANGOPT(ModulesSearchAll  , 1, 1, "searching even non-imported modules to find unresolved references")
 COMPATIBLE_LANGOPT(ModulesStrictDeclUse, 1, 0, "requiring declaration of module uses and all headers to be in modules")
+COMPATIBLE_LANGOPT(ModulesValidateTextualHeaderIncludes, 1, 1, "validation of textual header includes")
 BENIGN_LANGOPT(ModulesErrorRecovery, 1, 1, "automatically importing modules as needed when performing error recovery")
 BENIGN_LANGOPT(ImplicitModules, 1, 1, "building modules that are not specified via -fmodule-file")
 COMPATIBLE_LANGOPT(ModulesLocalVisibility, 1, 0, "local submodule visibility")
 COMPATIBLE_LANGOPT(Optimize          , 1, 0, "__OPTIMIZE__ predefined macro")
 COMPATIBLE_LANGOPT(OptimizeSize      , 1, 0, "__OPTIMIZE_SIZE__ predefined macro")
 COMPATIBLE_LANGOPT(Static            , 1, 0, "__STATIC__ predefined macro (as opposed to __DYNAMIC__)")
 VALUE_LANGOPT(PackStruct  , 32, 0,
               "default struct packing maximum alignment")

clang/include/clang/Driver/Options.td

 def fmodules_disable_diagnostic_validation : Flag<["-"], "fmodules-disable-diagnostic-validation">,
   Group<i_Group>, Flags<[CC1Option]>,
   HelpText<"Disable validation of the diagnostic options when loading the module">,
   MarshallingInfoNegativeFlag<HeaderSearchOpts<"ModulesValidateDiagnosticOptions">>;
 defm modules_validate_system_headers : BoolOption<"f", "modules-validate-system-headers",
   HeaderSearchOpts<"ModulesValidateSystemHeaders">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Validate the system headers that a module depends on when loading the module">,
   NegFlag<SetFalse, [NoXarchOption]>>, Group<i_Group>;
+def fno_modules_validate_textual_header_includes :
+  Flag<["-"], "fno-modules-validate-textual-header-includes">,
+  Group<f_Group>, Flags<[CC1Option, NoXarchOption]>,
+  MarshallingInfoNegativeFlag<LangOpts<"ModulesValidateTextualHeaderIncludes">>,
+  HelpText<"Do not enforce -fmodules-decluse and private header restrictions for textual headers. "
+           "This flag will be removed in a future Clang release.">;
 
 def fvalidate_ast_input_files_content:
   Flag <["-"], "fvalidate-ast-input-files-content">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Compute and store the hash of input files used to build an AST."
            " Files with mismatching mtime's are considered valid"
            " if both contents is identical">,
   MarshallingInfoFlag<HeaderSearchOpts<"ValidateASTInputFilesContent">>;

clang/include/clang/Lex/Preprocessor.h

public:
   static bool checkModuleIsAvailable(const LangOptions &LangOpts,
                                      const TargetInfo &TargetInfo,
                                      DiagnosticsEngine &Diags, Module *M);
 
   // Module inclusion testing.
   /// Find the module that owns the source or header file that
   /// \p Loc points to. If the location is in a file that was included
   /// into a module, or is outside any module, returns nullptr.
-  Module *getModuleForLocation(SourceLocation Loc);
+  Module *getModuleForLocation(SourceLocation Loc, bool AllowTextual);
 
   /// We want to produce a diagnostic at location IncLoc concerning an
   /// unreachable effect at location MLoc (eg, where a desired entity was
   /// declared or defined). Determine whether the right way to make MLoc
   /// reachable is by #include, and if so, what header should be included.
   ///
   /// This is not necessarily fast, and might load unexpected module maps, so
   /// should only be called by code that intends to produce an error.

clang/lib/Lex/PPDirectives.cpp

void Preprocessor::SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
   if (Callbacks && (Tok.isNot(tok::eof) || !isRecordingPreamble()))
     Callbacks->SourceRangeSkipped(
         SourceRange(HashTokenLoc, endLoc.isValid()
                                       ? endLoc
                                       : CurPPLexer->getSourceLocation()),
         Tok.getLocation());
 }
 
-Module *Preprocessor::getModuleForLocation(SourceLocation Loc) {
+Module *Preprocessor::getModuleForLocation(SourceLocation Loc,
+                                           bool AllowTextual) {
   if (!SourceMgr.isInMainFile(Loc)) {
     // Try to determine the module of the include directive.
     // FIXME: Look into directly passing the FileEntry from LookupFile instead.
     FileID IDOfIncl = SourceMgr.getFileID(SourceMgr.getExpansionLoc(Loc));
     if (const FileEntry *EntryOfIncl = SourceMgr.getFileEntryForID(IDOfIncl)) {
       // The include comes from an included file.
       return HeaderInfo.getModuleMap()
-          .findModuleForHeader(EntryOfIncl)
+          .findModuleForHeader(EntryOfIncl, AllowTextual)
           .getModule();
     }
   }
 
   // This is either in the main file or not in a file at all. It belongs
   // to the current module, if there is one.
   return getLangOpts().CurrentModule.empty()
              ? nullptr
              : HeaderInfo.lookupModule(getLangOpts().CurrentModule, Loc);
 }
 
 const FileEntry *
 Preprocessor::getHeaderToIncludeForDiagnostics(SourceLocation IncLoc,
                                                SourceLocation Loc) {
-  Module *IncM = getModuleForLocation(IncLoc);
+  Module *IncM = getModuleForLocation(
+      IncLoc, LangOpts.ModulesValidateTextualHeaderIncludes);
 
   // Walk up through the include stack, looking through textual headers of M
   // until we hit a non-textual header that we can #include. (We assume textual
   // headers of a module with non-textual headers aren't meant to be used to
   // import entities from the module.)
   auto &SM = getSourceManager();
   while (!Loc.isInvalid() && !SM.isInMainFile(Loc)) {
     auto ID = SM.getFileID(SM.getExpansionLoc(Loc));
Optional<FileEntryRef> Preprocessor::LookupFile(
     ConstSearchDirIterator FromDir, const FileEntry *FromFile,
     ConstSearchDirIterator *CurDirArg, SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
     ModuleMap::KnownHeader *SuggestedModule, bool *IsMapped,
     bool *IsFrameworkFound, bool SkipCache, bool OpenFile, bool CacheFailures) {
   ConstSearchDirIterator CurDirLocal = nullptr;
   ConstSearchDirIterator &CurDir = CurDirArg ? *CurDirArg : CurDirLocal;
 
-  Module *RequestingModule = getModuleForLocation(FilenameLoc);
+  Module *RequestingModule = getModuleForLocation(
+      FilenameLoc, LangOpts.ModulesValidateTextualHeaderIncludes);
   bool RequestingModuleIsModuleInterface = !SourceMgr.isInMainFile(FilenameLoc);
 
   // If the header lookup mechanism may be relative to the current inclusion
   // stack, record the parent #includes.
   SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 16>
       Includers;
   bool BuildSystemModule = false;
   if (!FromDir && !FromFile) {

clang/test/Modules/Inputs/declare-use/module.map

 module XN {
   module sub {
     header "sub.h"
   }
 }
 
 module XS {
 }
+
+module Textual {
+  textual header "textual.h"
+}

clang/test/Modules/Inputs/declare-use/textual.h

+#include "a.h"

clang/test/Modules/declare-use-textual.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fimplicit-module-maps -fmodules-cache-path=%t -fmodules-decluse -fmodule-name=Textual -I %S/Inputs/declare-use %s -verify
+// RUN: %clang_cc1 -fimplicit-module-maps -fmodules-cache-path=%t -fmodules-decluse -fmodule-name=Textual -I %S/Inputs/declare-use %s -fno-modules-validate-textual-header-includes
+
+// expected-error@textual.h:* {{module Textual does not depend on a module exporting 'a.h'}}
+#include "textual.h"