[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D10423: [modules] PR20507: Avoid silent textual inclusion.
From: Sean Silva <chisophugis () gmail ! com>
Date: 2015-07-20 21:49:08
Message-ID: c021689ade9aec516b98582d6aa46217 () localhost ! localdomain
[Download RAW message or body]
silvas updated this revision to Diff 30196.
silvas added a comment.
Updating based on Richard's feedback.
Richard, is this something like you imagined?
The PPCallbacks is still not being called, like the existing branch
`// We hit an error processing the import. Bail out.`. The fix will be
similar for both, so this at least isn't making it any harder to fix the
situation.
http://reviews.llvm.org/D10423
Files:
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/DiagnosticFrontendKinds.td
include/clang/Basic/DiagnosticLexKinds.td
lib/Lex/ModuleMap.cpp
lib/Lex/PPDirectives.cpp
test/Modules/Inputs/auto-import-unavailable/missing_header/not_missing.h
test/Modules/Inputs/auto-import-unavailable/missing_requirement.h
test/Modules/Inputs/auto-import-unavailable/module.modulemap
test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/no=
t_missing.h
test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/re=
quires_feature_you_dont_have.h
test/Modules/Inputs/available-is-better/available-is-better.h
test/Modules/Inputs/available-is-better/module.modulemap
test/Modules/Inputs/declare-use/module.map
test/Modules/Inputs/macro-reexport/module.modulemap
test/Modules/auto-import-unavailable.cpp
test/Modules/available-is-better.cpp
["D10423.30196.patch" (text/x-patch)]
Index: test/Modules/available-is-better.cpp
===================================================================
--- /dev/null
+++ test/Modules/available-is-better.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -Rmodule-build -fmodules -fimplicit-module-maps \
-fmodules-cache-path=%t -I %S/Inputs/available-is-better %s 2>&1 | FileCheck %s +
+#include "available-is-better.h"
+// CHECK: remark: building module
Index: test/Modules/auto-import-unavailable.cpp
===================================================================
--- /dev/null
+++ test/Modules/auto-import-unavailable.cpp
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: not %clang_cc1 -x c++ -Rmodule-build -DMISSING_HEADER -fmodules \
-fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/auto-import-unavailable \
%s 2>&1 | FileCheck %s --check-prefix=MISSING-HEADER +// RUN: %clang_cc1 -x c++ \
-Rmodule-build -DNONREQUIRED_MISSING_HEADER -fmodules -fimplicit-module-maps \
-fmodules-cache-path=%t -I %S/Inputs/auto-import-unavailable %s 2>&1 | FileCheck %s \
--check-prefix=NONREQUIRED-MISSING-HEADER +// RUN: not %clang_cc1 -x c++ \
-Rmodule-build -DMISSING_REQUIREMENT -fmodules -fimplicit-module-maps \
-fmodules-cache-path=%t -I %S/Inputs/auto-import-unavailable %s 2>&1 | FileCheck %s \
--check-prefix=MISSING-REQUIREMENT +
+#ifdef MISSING_HEADER
+
+// Even if the header we ask for is not missing, if the top-level module
+// containing it has a missing header, then the whole top-level is
+// unavailable and we issue an error.
+
+// MISSING-HEADER: module.modulemap:2:27: error: header 'missing_header/missing.h' \
not found +// MISSING-HEADER-DAG: auto-import-unavailable.cpp:[[@LINE+1]]:10: note: \
submodule of top-level module 'missing_header' implicitly imported here +#include \
"missing_header/not_missing.h" +
+// We should not attempt to build the module.
+// MISSING-HEADER-NOT: remark: building module
+
+#endif // #ifdef MISSING_HEADER
+
+
+#ifdef NONREQUIRED_MISSING_HEADER
+
+// However, if the missing header is dominated by an unsatisfied
+// `requires`, then that is acceptable.
+// This also tests that an unsatisfied `requires` elsewhere in the
+// top-level module doesn't affect an available module.
+
+// NONREQUIRED-MISSING-HEADER: auto-import-unavailable.cpp:[[@LINE+2]]:10: remark: \
building module 'nonrequired_missing_header' +// NONREQUIRED-MISSING-HEADER: \
auto-import-unavailable.cpp:[[@LINE+1]]:10: remark: finished building module \
'nonrequired_missing_header' +#include "nonrequired_missing_header/not_missing.h"
+
+#endif // #ifdef NONREQUIRED_MISSING_HEADER
+
+
+#ifdef MISSING_REQUIREMENT
+
+// If the header is unavailable due to a missing requirement, an error
+// should be emitted if a user tries to include it.
+
+// MISSING-REQUIREMENT:module.modulemap:16:8: error: module 'missing_requirement' \
requires feature 'nonexistent_feature' +// MISSING-REQUIREMENT: \
auto-import-unavailable.cpp:[[@LINE+1]]:10: note: submodule of top-level module \
'missing_requirement' implicitly imported here +#include "missing_requirement.h"
+
+// MISSING-REQUIREMENT-NOT: remark: building module
+
+#endif // #ifdef MISSING_REQUIREMENT
Index: test/Modules/Inputs/macro-reexport/module.modulemap
===================================================================
--- test/Modules/Inputs/macro-reexport/module.modulemap
+++ test/Modules/Inputs/macro-reexport/module.modulemap
@@ -19,5 +19,4 @@
}
module f {
module f1 { header "f1.h" export * }
- module f2 { header "f2.h" export * }
}
Index: test/Modules/Inputs/declare-use/module.map
===================================================================
--- test/Modules/Inputs/declare-use/module.map
+++ test/Modules/Inputs/declare-use/module.map
@@ -20,14 +20,12 @@
module XE {
header "e.h"
- header "unavailable.h"
use XA
use XB
}
module XF {
header "f.h"
- header "unavailable.h"
use XA
use XB
}
Index: test/Modules/Inputs/available-is-better/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/available-is-better/module.modulemap
@@ -0,0 +1,17 @@
+// There is some order-dependence to how clang chooses modules, so make
+// sure that both the first and last modules here are ones that would
+// cause a test failure if they were picked.
+
+module unavailable_before {
+ requires nonexistent_feature
+ header "available-is-better.h"
+}
+
+module available {
+ header "available-is-better.h"
+}
+
+module unavailable_after {
+ requires nonexistent_feature
+ header "available-is-better.h"
+}
Index: test/Modules/Inputs/available-is-better/available-is-better.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/available-is-better/available-is-better.h
@@ -0,0 +1,2 @@
+#pragma once
+int available;
Index: test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/requires_feature_you_dont_have.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/requires_feature_you_dont_have.h
@@ -0,0 +1 @@
+// nonrequired_missing_header/requires_feature_you_dont_have.h
Index: test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/not_missing.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/not_missing.h
@@ -0,0 +1 @@
+// nonrequired_missing_header/not_missing.h
Index: test/Modules/Inputs/auto-import-unavailable/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/auto-import-unavailable/module.modulemap
@@ -0,0 +1,19 @@
+module missing_header {
+ module missing { header "missing_header/missing.h" }
+ module error_importing_this { header "missing_header/not_missing.h" }
+}
+
+module nonrequired_missing_header {
+ module unsatisfied_requires {
+ requires nonexistent_feature
+ header "nonrequired_missing_header/missing.h"
+ }
+ module fine_to_import {
+ header "nonrequired_missing_header/not_missing.h"
+ }
+}
+
+module missing_requirement {
+ requires nonexistent_feature
+ header "missing_requirement.h"
+}
Index: test/Modules/Inputs/auto-import-unavailable/missing_requirement.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/auto-import-unavailable/missing_requirement.h
@@ -0,0 +1 @@
+// missing_requirement.h
Index: test/Modules/Inputs/auto-import-unavailable/missing_header/not_missing.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/auto-import-unavailable/missing_header/not_missing.h
@@ -0,0 +1 @@
+// missing_header/not_missing.h
Index: lib/Lex/PPDirectives.cpp
===================================================================
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1674,6 +1674,29 @@
getLangOpts().CurrentModule &&
SuggestedModule.getModule()->getTopLevelModuleName() !=
getLangOpts().ImplementationOfModule) {
+
+ // If this include corresponds to a module but that module is
+ // unavailable, diagnose the situation and bail out.
+ if (!SuggestedModule.getModule()->isAvailable()) {
+ clang::Module::Requirement Requirement;
+ clang::Module::UnresolvedHeaderDirective MissingHeader;
+ Module *M = SuggestedModule.getModule();
+ // Identify the cause.
+ (void)M->isAvailable(getLangOpts(), getTargetInfo(), Requirement,
+ MissingHeader);
+ if (MissingHeader.FileNameLoc.isValid()) {
+ Diag(MissingHeader.FileNameLoc, diag::err_module_header_missing)
+ << MissingHeader.IsUmbrella << MissingHeader.FileName;
+ } else {
+ Diag(M->DefinitionLoc, diag::err_module_unavailable)
+ << M->getFullModuleName() << Requirement.second << Requirement.first;
+ }
+ Diag(FilenameTok.getLocation(),
+ diag::note_implicit_top_level_module_import_here)
+ << M->getTopLevelModuleName();
+ return;
+ }
+
// Compute the module access path corresponding to this module.
// FIXME: Should we have a second loadModule() overload to avoid this
// extra lookup step?
Index: lib/Lex/ModuleMap.cpp
===================================================================
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -320,6 +320,10 @@
static bool isBetterKnownHeader(const ModuleMap::KnownHeader &New,
const ModuleMap::KnownHeader &Old) {
+ // Prefer available modules.
+ if (New.getModule()->isAvailable() && !Old.getModule()->isAvailable())
+ return true;
+
// Prefer a public header over a private header.
if ((New.getRole() & ModuleMap::PrivateHeader) !=
(Old.getRole() & ModuleMap::PrivateHeader))
@@ -349,9 +353,6 @@
// Prefer a header from the current module over all others.
if (H.getModule()->getTopLevelModule() == CompilingModule)
return MakeResult(H);
- // Cannot use a module if it is unavailable.
- if (!H.getModule()->isAvailable())
- continue;
if (!Result || isBetterKnownHeader(H, Result))
Result = H;
}
Index: include/clang/Basic/DiagnosticLexKinds.td
===================================================================
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -622,6 +622,8 @@
def warn_auto_module_import : Warning<
"treating #%select{include|import|include_next|__include_macros}0 as an "
"import of module '%1'">, InGroup<AutoImport>, DefaultIgnore;
+def note_implicit_top_level_module_import_here : Note<
+ "submodule of top-level module '%0' implicitly imported here">;
def warn_uncovered_module_header : Warning<
"umbrella header for module '%0' does not include header '%1'">,
InGroup<IncompleteUmbrella>;
Index: include/clang/Basic/DiagnosticFrontendKinds.td
===================================================================
--- include/clang/Basic/DiagnosticFrontendKinds.td
+++ include/clang/Basic/DiagnosticFrontendKinds.td
@@ -173,10 +173,6 @@
"no submodule named %0 in module '%1'; did you mean '%2'?">;
def warn_missing_submodule : Warning<"missing submodule '%0'">,
InGroup<IncompleteUmbrella>;
-def err_module_unavailable : Error<
- "module '%0' %select{is incompatible with|requires}1 feature '%2'">;
-def err_module_header_missing : Error<
- "%select{|umbrella }0header '%1' not found">;
def err_module_cannot_create_includes : Error<
"cannot create includes file for module %0: %1">;
def warn_module_config_macro_undef : Warning<
Index: include/clang/Basic/DiagnosticCommonKinds.td
===================================================================
--- include/clang/Basic/DiagnosticCommonKinds.td
+++ include/clang/Basic/DiagnosticCommonKinds.td
@@ -84,6 +84,10 @@
def err_module_build_disabled: Error<
"module '%0' is needed but has not been provided, and implicit use of module "
"files is disabled">, DefaultFatal;
+def err_module_unavailable : Error<
+ "module '%0' %select{is incompatible with|requires}1 feature '%2'">;
+def err_module_header_missing : Error<
+ "%select{|umbrella }0header '%1' not found">;
def err_module_lock_failure : Error<
"could not acquire lock file for module '%0'">, DefaultFatal;
def err_module_lock_timeout : Error<
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic