[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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 `RequestingModule`s 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

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()
  #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()
  #error Main TU does have that include.
  #endif
  
  #include "A.h"
  
  #if !__has_include()
  #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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa002063de37c: Enforce module decl-use restrictions and 
private header restrictions in textual… (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D132779?vs=458312&id=458324#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Modules/Inputs/declare-use/module.map
  clang/test/Modules/Inputs/declare-use/textual.h
  clang/test/Modules/declare-use-textual.cpp

Index: clang/test/Modules/declare-use-textual.cpp
===
--- /dev/null
+++ clang/test/Modules/declare-use-textual.cpp
@@ -0,0 +1,6 @@
+// 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"
Index: clang/test/Modules/Inputs/declare-use/textual.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/declare-use/textual.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/declare-use/module.map
===
--- clang/test/Modules/Inputs/declare-use/module.map
+++ clang/test/Modules/Inputs/declare-use/module.map
@@ -73,3 +73,7 @@
 
 module XS {
 }
+
+module Textual {
+  textual header "textual.h"
+}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -856,7 +856,8 @@
 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.
@@ -864,7 +865,7 @@
 if (const FileEntry *EntryOfIncl = SourceMgr.getFileEntryForID(IDOfIncl)) {
   // The include comes from an included file.
   return HeaderInfo.getModuleMap()
-  .findModuleForHeader(EntryOfIncl)
+  .findModuleForHeader(EntryOfIncl, AllowTextual)
   .getModule();
 }
   }
@@ -879,7 +880,8 @@
 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
@@ -953,7 +955,8 @@
   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
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2551,7 +2551,7 @@
   /// 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
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2273,6 +2273,12 @@
   HeaderSearchOpts<"ModulesValidateSystemHeaders">, DefaultFalse,
   PosFlag,
   NegFlag>, Group;
+def fno_modules_validate_text

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 458312.
rsmith edited the summary of this revision.
rsmith added a comment.

- Help text update from Aaron.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Modules/Inputs/declare-use/module.map
  clang/test/Modules/Inputs/declare-use/textual.h
  clang/test/Modules/declare-use-textual.cpp

Index: clang/test/Modules/declare-use-textual.cpp
===
--- /dev/null
+++ clang/test/Modules/declare-use-textual.cpp
@@ -0,0 +1,6 @@
+// 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"
Index: clang/test/Modules/Inputs/declare-use/textual.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/declare-use/textual.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/declare-use/module.map
===
--- clang/test/Modules/Inputs/declare-use/module.map
+++ clang/test/Modules/Inputs/declare-use/module.map
@@ -73,3 +73,7 @@
 
 module XS {
 }
+
+module Textual {
+  textual header "textual.h"
+}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -856,7 +856,8 @@
 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.
@@ -864,7 +865,7 @@
 if (const FileEntry *EntryOfIncl = SourceMgr.getFileEntryForID(IDOfIncl)) {
   // The include comes from an included file.
   return HeaderInfo.getModuleMap()
-  .findModuleForHeader(EntryOfIncl)
+  .findModuleForHeader(EntryOfIncl, AllowTextual)
   .getModule();
 }
   }
@@ -879,7 +880,8 @@
 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
@@ -953,7 +955,8 @@
   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
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2546,7 +2546,7 @@
   /// 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
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2283,6 +2283,12 @@
   HeaderSearchOpts<"ModulesValidateSystemHeaders">, DefaultFalse,
   PosFlag,
   NegFlag>, Group;
+def fno_modules_validate_textual_header_includes :
+  Flag<["-"], "fno-modules-validate-textual-header-includes">,
+  Group, Flags<[CC1Option, NoXarchOption]>,
+  MarshallingInfoNegativeFlag>,
+  HelpText<"Do not enforce -fmodules-decl

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang/include/clang/Driver/Options.td:2290
+  
MarshallingInfoNegativeFlag>,
+  HelpText<"Do not enforce -fmodules-decluse and private header restrictions 
for textual headers.">;
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:126
+  `_ are now
+  diagnosed even when the includer is a textual header.
 

aaron.ballman wrote:
> 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"?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 457742.
rsmith added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Modules/Inputs/declare-use/module.map
  clang/test/Modules/Inputs/declare-use/textual.h
  clang/test/Modules/declare-use-textual.cpp

Index: clang/test/Modules/declare-use-textual.cpp
===
--- /dev/null
+++ clang/test/Modules/declare-use-textual.cpp
@@ -0,0 +1,6 @@
+// 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"
Index: clang/test/Modules/Inputs/declare-use/textual.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/declare-use/textual.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/declare-use/module.map
===
--- clang/test/Modules/Inputs/declare-use/module.map
+++ clang/test/Modules/Inputs/declare-use/module.map
@@ -73,3 +73,7 @@
 
 module XS {
 }
+
+module Textual {
+  textual header "textual.h"
+}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -856,7 +856,8 @@
 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.
@@ -864,7 +865,7 @@
 if (const FileEntry *EntryOfIncl = SourceMgr.getFileEntryForID(IDOfIncl)) {
   // The include comes from an included file.
   return HeaderInfo.getModuleMap()
-  .findModuleForHeader(EntryOfIncl)
+  .findModuleForHeader(EntryOfIncl, AllowTextual)
   .getModule();
 }
   }
@@ -879,7 +880,8 @@
 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
@@ -953,7 +955,8 @@
   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
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2546,7 +2546,7 @@
   /// 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
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2283,6 +2283,11 @@
   HeaderSearchOpts<"ModulesValidateSystemHeaders">, DefaultFalse,
   PosFlag,
   NegFlag>, Group;
+def fno_modules_validate_textual_header_includes :
+  Flag<["-"], "fno-modules-validate-textual-header-includes">,
+  Group, Flags<[CC1Option, NoXarchOption]>,
+  MarshallingInfoNegativeFlag>,
+  HelpText<"Do not enforce -fmodules-declu

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: clang/docs/ReleaseNotes.rst:126
+  `_ are now
+  diagnosed even when the includer is a textual header.
 

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"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 456077.
rsmith added a comment.

Add release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132779

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Modules/Inputs/declare-use/module.map
  clang/test/Modules/Inputs/declare-use/textual.h
  clang/test/Modules/declare-use-textual.cpp

Index: clang/test/Modules/declare-use-textual.cpp
===
--- /dev/null
+++ clang/test/Modules/declare-use-textual.cpp
@@ -0,0 +1,6 @@
+// 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"
Index: clang/test/Modules/Inputs/declare-use/textual.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/declare-use/textual.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/declare-use/module.map
===
--- clang/test/Modules/Inputs/declare-use/module.map
+++ clang/test/Modules/Inputs/declare-use/module.map
@@ -73,3 +73,7 @@
 
 module XS {
 }
+
+module Textual {
+  textual header "textual.h"
+}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -856,7 +856,8 @@
 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.
@@ -864,7 +865,7 @@
 if (const FileEntry *EntryOfIncl = SourceMgr.getFileEntryForID(IDOfIncl)) {
   // The include comes from an included file.
   return HeaderInfo.getModuleMap()
-  .findModuleForHeader(EntryOfIncl)
+  .findModuleForHeader(EntryOfIncl, AllowTextual)
   .getModule();
 }
   }
@@ -879,7 +880,8 @@
 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
@@ -953,7 +955,8 @@
   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
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2546,7 +2546,7 @@
   /// 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
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2283,6 +2283,11 @@
   HeaderSearchOpts<"ModulesValidateSystemHeaders">, DefaultFalse,
   PosFlag,
   NegFlag>, Group;
+def fno_modules_validate_textual_header_includes :
+  Flag<["-"], "fno-modules-validate-textual-header-includes">,
+  Group, Flags<[CC1Option, NoXarchOption]>,
+  MarshallingInfoNegativeFlag>,
+  HelpText<"Do not enforce -fmodules-decluse and private header restrictions for textual headers.

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added reviewers: Bigcheese, aaron.ballman.
Herald added a project: All.
rsmith requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132779

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Modules/Inputs/declare-use/module.map
  clang/test/Modules/Inputs/declare-use/textual.h
  clang/test/Modules/declare-use-textual.cpp

Index: clang/test/Modules/declare-use-textual.cpp
===
--- /dev/null
+++ clang/test/Modules/declare-use-textual.cpp
@@ -0,0 +1,6 @@
+// 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"
Index: clang/test/Modules/Inputs/declare-use/textual.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/declare-use/textual.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/declare-use/module.map
===
--- clang/test/Modules/Inputs/declare-use/module.map
+++ clang/test/Modules/Inputs/declare-use/module.map
@@ -73,3 +73,7 @@
 
 module XS {
 }
+
+module Textual {
+  textual header "textual.h"
+}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -856,7 +856,8 @@
 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.
@@ -864,7 +865,7 @@
 if (const FileEntry *EntryOfIncl = SourceMgr.getFileEntryForID(IDOfIncl)) {
   // The include comes from an included file.
   return HeaderInfo.getModuleMap()
-  .findModuleForHeader(EntryOfIncl)
+  .findModuleForHeader(EntryOfIncl, AllowTextual)
   .getModule();
 }
   }
@@ -879,7 +880,8 @@
 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
@@ -953,7 +955,8 @@
   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
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2546,7 +2546,7 @@
   /// 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
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2283,6 +2283,11 @@
   HeaderSearchOpts<"ModulesValidateSystemHeaders