<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 4 December 2016 at 20:44, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><span class="gmail-"><div><div class="gmail_extra"><div class="gmail_quote">On 4 Dec 2016 2:44 pm, "Daniel Jasper via cfe-commits" <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br type="attribution"><blockquote class="gmail-m_7764805212293459477quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: djasper<br>
Date: Sun Dec 4 16:34:37 2016<br>
New Revision: 288626<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=288626&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=288626&view=rev</a><br>
Log:<br>
Revert "Recover better from an incompatible .pcm file being provided by -fmodule-file=. We try to include the headers of the module textually in this case, still enforcing the modules semantic rules. In order to make that work, we need to still track that we're entering and leaving the module. Also, if the module was also marked as unavailable (perhaps because it was missing a file), we shouldn't mark the module unavailable -- we don't need the module to be complete if we're going to enter it textually."<br>
<br>
This reverts commit r288449.<br>
<br>
I believe that this is currently faulty wrt. modules being imported<br>
inside namespaces. Adding these lines to the new test:<br>
<br>
namespace n {<br>
#include "foo.h"<br>
}<br>
<br>
Makes it break with<br>
<br>
fatal error: import of module 'M' appears within namespace 'n'<br>
<br>
However, I believe it should fail with<br>
<br>
error: redundant #include of module 'M' appears within namespace 'n'<br></blockquote></div></div></div><div dir="auto"><br></div></span><div dir="auto">Is that really worse than the broken internal state and asserts we get in the same situation without this patch? The ability to continue like this after a config mismatch is best-effort as is (and we're working towards removing it...); I don't think we should be all that picky about which error diagnostic we get here.</div><div dir="auto"><br></div><div dir="auto">Mind if I revert your revert?</div></div></blockquote><div><br></div><div>Wrong diagnostic fixed in r288737, revert reverted in revert r288741 (with the above addition to the test).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div><div class="gmail-h5"><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail-m_7764805212293459477quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I have tracked this down to us now inserting a tok::annot_module_begin<br>
instead of a tok::annot_module_include in<br>
Preprocessor::HandleIncludeDir<wbr>ective() and then later in<br>
Parser::parseMisplacedModuleIm<wbr>port(), we hit the code path for<br>
tok::annot_module_begin, which doesn't set FromInclude of<br>
checkModuleImportContext to true (thus leading to the "wrong"<br>
diagnostic).<br>
<br>
Removed:<br>
cfe/trunk/test/Modules/config-<wbr>mismatch.cpp<br>
Modified:<br>
cfe/trunk/include/clang/Lex/Mo<wbr>duleLoader.h<br>
cfe/trunk/lib/Frontend/Compile<wbr>rInstance.cpp<br>
cfe/trunk/lib/Lex/PPDirectives<wbr>.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Lex/Mo<wbr>duleLoader.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/ModuleLoader.h?rev=288626&r1=288625&r2=288626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/include/clang/<wbr>Lex/ModuleLoader.h?rev=288626&<wbr>r1=288625&r2=288626&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/Lex/Mo<wbr>duleLoader.h (original)<br>
+++ cfe/trunk/include/clang/Lex/Mo<wbr>duleLoader.h Sun Dec 4 16:34:37 2016<br>
@@ -31,22 +31,13 @@ typedef ArrayRef<std::pair<IdentifierI<wbr>nf<br>
<br>
/// \brief Describes the result of attempting to load a module.<br>
class ModuleLoadResult {<br>
-public:<br>
- enum LoadResultKind {<br>
- // We either succeeded or failed to load the named module.<br>
- Normal,<br>
- // The module exists, but does not actually contain the named submodule.<br>
- // This should only happen if the named submodule was inferred from an<br>
- // umbrella directory, but not actually part of the umbrella header.<br>
- MissingExpected,<br>
- // The module exists but cannot be imported due to a configuration mismatch.<br>
- ConfigMismatch<br>
- };<br>
- llvm::PointerIntPair<Module *, 2, LoadResultKind> Storage;<br>
+ llvm::PointerIntPair<Module *, 1, bool> Storage;<br>
<br>
+public:<br>
ModuleLoadResult() : Storage() { }<br>
- ModuleLoadResult(Module *M) : Storage(M, Normal) {}<br>
- ModuleLoadResult(LoadResultKin<wbr>d Kind) : Storage(nullptr, Kind) {}<br>
+<br>
+ ModuleLoadResult(Module *module, bool missingExpected)<br>
+ : Storage(module, missingExpected) { }<br>
<br>
operator Module *() const { return Storage.getPointer(); }<br>
<br>
@@ -54,11 +45,7 @@ public:<br>
/// actually a submodule that we expected to see (based on implying the<br>
/// submodule from header structure), but didn't materialize in the actual<br>
/// module.<br>
- bool isMissingExpected() const { return Storage.getInt() == MissingExpected; }<br>
-<br>
- /// \brief Determines whether the module failed to load due to a configuration<br>
- /// mismatch with an explicitly-named .pcm file from the command line.<br>
- bool isConfigMismatch() const { return Storage.getInt() == ConfigMismatch; }<br>
+ bool isMissingExpected() const { return Storage.getInt(); }<br>
};<br>
<br>
/// \brief Abstract interface for a module loader.<br>
<br>
Modified: cfe/trunk/lib/Frontend/Compile<wbr>rInstance.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=288626&r1=288625&r2=288626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/lib/Frontend/<wbr>CompilerInstance.cpp?rev=<wbr>288626&r1=288625&r2=288626&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Frontend/Compile<wbr>rInstance.cpp (original)<br>
+++ cfe/trunk/lib/Frontend/Compile<wbr>rInstance.cpp Sun Dec 4 16:34:37 2016<br>
@@ -1393,21 +1393,8 @@ bool CompilerInstance::loadModuleFi<wbr>le(St<br>
if (Module *M = CI.getPreprocessor()<br>
.getHeaderSearchInfo()<br>
.getModuleMap()<br>
- .findModule(II->getName())) {<br>
+ .findModule(II->getName()))<br>
M->HasIncompatibleModuleFile = true;<br>
-<br>
- // Mark module as available if the only reason it was unavailable<br>
- // was missing headers.<br>
- SmallVector<Module *, 2> Stack;<br>
- Stack.push_back(M);<br>
- while (!Stack.empty()) {<br>
- Module *Current = Stack.pop_back_val();<br>
- if (Current->IsMissingRequirement<wbr>) continue;<br>
- Current->IsAvailable = true;<br>
- Stack.insert(Stack.end(),<br>
- Current->submodule_begin(), Current->submodule_end());<br>
- }<br>
- }<br>
}<br>
LoadedModules.clear();<br>
}<br>
@@ -1511,7 +1498,7 @@ CompilerInstance::loadModule(S<wbr>ourceLocat<br>
if (Module && Module->HasIncompatibleModuleF<wbr>ile) {<br>
// We tried and failed to load a module file for this module. Fall<br>
// back to textual inclusion for its headers.<br>
- return ModuleLoadResult::ConfigMismat<wbr>ch;<br>
+ return ModuleLoadResult(nullptr, /*missingExpected*/true);<br>
}<br>
<br>
getDiagnostics().Report(Modul<wbr>eNameLoc, diag::err_module_build_disable<wbr>d)<br>
@@ -1718,7 +1705,7 @@ CompilerInstance::loadModule(S<wbr>ourceLocat<br>
<< Module->getFullModuleName()<br>
<< SourceRange(Path.front().secon<wbr>d, Path.back().second);<br>
<br>
- return ModuleLoadResult::MissingExpec<wbr>ted;<br>
+ return ModuleLoadResult(nullptr, true);<br>
}<br>
<br>
// Check whether this module is available.<br>
@@ -1752,7 +1739,7 @@ CompilerInstance::loadModule(S<wbr>ourceLocat<br>
}<br>
<br>
LastModuleImportLoc = ImportLoc;<br>
- LastModuleImportResult = ModuleLoadResult(Module);<br>
+ LastModuleImportResult = ModuleLoadResult(Module, false);<br>
return LastModuleImportResult;<br>
}<br>
<br>
<br>
Modified: cfe/trunk/lib/Lex/PPDirectives<wbr>.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDirectives.cpp?rev=288626&r1=288625&r2=288626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/lib/Lex/PPDire<wbr>ctives.cpp?rev=288626&r1=<wbr>288625&r2=288626&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Lex/PPDirectives<wbr>.cpp (original)<br>
+++ cfe/trunk/lib/Lex/PPDirectives<wbr>.cpp Sun Dec 4 16:34:37 2016<br>
@@ -1866,7 +1866,10 @@ void Preprocessor::HandleIncludeDir<wbr>ectiv<br>
// unavailable, diagnose the situation and bail out.<br>
// FIXME: Remove this; loadModule does the same check (but produces<br>
// slightly worse diagnostics).<br>
- if (!SuggestedModule.getModule()-<wbr>>isAvailable()) {<br>
+ if (!SuggestedModule.getModule()-<wbr>>isAvailable() &&<br>
+ !SuggestedModule.getModule()<br>
+ ->getTopLevelModule()<br>
+ ->HasIncompatibleModuleFile) {<br>
Module::Requirement Requirement;<br>
Module::UnresolvedHeaderDirec<wbr>tive MissingHeader;<br>
Module *M = SuggestedModule.getModule();<br>
@@ -1915,12 +1918,9 @@ void Preprocessor::HandleIncludeDir<wbr>ectiv<br>
else if (Imported.isMissingExpected()) {<br>
// We failed to find a submodule that we assumed would exist (because it<br>
// was in the directory of an umbrella header, for instance), but no<br>
- // actual module containing it exists (because the umbrella header is<br>
+ // actual module exists for it (because the umbrella header is<br>
// incomplete). Treat this as a textual inclusion.<br>
SuggestedModule = ModuleMap::KnownHeader();<br>
- } else if (Imported.isConfigMismatch()) {<br>
- // On a configuration mismatch, enter the header textually. We still know<br>
- // that it's part of the corresponding module.<br>
} else {<br>
// We hit an error processing the import. Bail out.<br>
if (hadModuleLoaderFatalFailure()<wbr>) {<br>
<br>
Removed: cfe/trunk/test/Modules/config-<wbr>mismatch.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/config-mismatch.cpp?rev=288625&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Modules/<wbr>config-mismatch.cpp?rev=<wbr>288625&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Modules/config-<wbr>mismatch.cpp (original)<br>
+++ cfe/trunk/test/Modules/config-<wbr>mismatch.cpp (removed)<br>
@@ -1,10 +0,0 @@<br>
-// RUN: rm -rf %t<br>
-// RUN: mkdir %t<br>
-// RUN: echo 'module M { header "foo.h" header "bar.h" }' > %t/map<br>
-// RUN: echo 'template<typename T> void f(T t) { int n; t.f(n); }' > %t/foo.h<br>
-// RUN: touch %t/bar.h<br>
-// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visi<wbr>bility -x c++ %t/map -emit-module -fmodule-name=M -o %t/pcm<br>
-// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visi<wbr>bility -fmodule-map-file=%t/map -fmodule-file=%t/pcm -I%t %s -fsyntax-only -fexceptions -Wno-module-file-config-mismat<wbr>ch<br>
-// RUN: rm %t/bar.h<br>
-// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visi<wbr>bility -fmodule-map-file=%t/map -fmodule-file=%t/pcm -I%t %s -fsyntax-only -fexceptions -Wno-module-file-config-mismat<wbr>ch<br>
-#include "foo.h"<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></div></div></div>
</blockquote></div><br></div></div>