Sun Dec 4 20:44:53 PST 2016

On 4 Dec 2016 2:44 pm, "Daniel Jasper via cfe-commits" <
cfe-commits at lists.llvm.org> wrote:

Author: djasper
Date: Sun Dec  4 16:34:37 2016
New Revision: 288626

URL: http://llvm.org/viewvc/llvm-project?rev=288626&view=rev
Log:
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."

This reverts commit r288449.

I believe that this is currently faulty wrt. modules being imported
inside namespaces. Adding these lines to the new test:

  namespace n {
  #include "foo.h"
  }

Makes it break with

  fatal error: import of module 'M' appears within namespace 'n'

However, I believe it should fail with

  error: redundant #include of module 'M' appears within namespace 'n'


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.

Mind if I revert your revert?

I have tracked this down to us now inserting a tok::annot_module_begin
instead of a tok::annot_module_include in
Preprocessor::HandleIncludeDirective() and then later in
Parser::parseMisplacedModuleImport(), we hit the code path for
tok::annot_module_begin, which doesn't set FromInclude of
checkModuleImportContext to true (thus leading to the "wrong"
diagnostic).

Removed:
    cfe/trunk/test/Modules/config-mismatch.cpp
Modified:
    cfe/trunk/include/clang/Lex/ModuleLoader.h
    cfe/trunk/lib/Frontend/CompilerInstance.cpp
    cfe/trunk/lib/Lex/PPDirectives.cpp

Modified: cfe/trunk/include/clang/Lex/ModuleLoader.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/
clang/Lex/ModuleLoader.h?rev=288626&r1=288625&r2=288626&view=diff
============================================================
==================

--- cfe/trunk/include/clang/Lex/ModuleLoader.h (original)
+++ cfe/trunk/include/clang/Lex/ModuleLoader.h Sun Dec  4 16:34:37 2016
@@ -31,22 +31,13 @@ typedef ArrayRef<std::pair<IdentifierInf

 /// \brief Describes the result of attempting to load a module.
 class ModuleLoadResult {
-public:
-  enum LoadResultKind {
-    // We either succeeded or failed to load the named module.
-    Normal,
-    // The module exists, but does not actually contain the named
submodule.
-    // This should only happen if the named submodule was inferred from an
-    // umbrella directory, but not actually part of the umbrella header.
-    MissingExpected,
-    // The module exists but cannot be imported due to a configuration
mismatch.
-    ConfigMismatch
-  };
-  llvm::PointerIntPair<Module *, 2, LoadResultKind> Storage;
+  llvm::PointerIntPair<Module *, 1, bool> Storage;

+public:
   ModuleLoadResult() : Storage() { }
-  ModuleLoadResult(Module *M) : Storage(M, Normal) {}
-  ModuleLoadResult(LoadResultKind Kind) : Storage(nullptr, Kind) {}
+
+  ModuleLoadResult(Module *module, bool missingExpected)
+    : Storage(module, missingExpected) { }

   operator Module *() const { return Storage.getPointer(); }

@@ -54,11 +45,7 @@ public:
   /// actually a submodule that we expected to see (based on implying the
   /// submodule from header structure), but didn't materialize in the
actual
   /// module.
-  bool isMissingExpected() const { return Storage.getInt() ==
MissingExpected; }
-
-  /// \brief Determines whether the module failed to load due to a
configuration
-  /// mismatch with an explicitly-named .pcm file from the command line.
-  bool isConfigMismatch() const { return Storage.getInt() ==
ConfigMismatch; }
+  bool isMissingExpected() const { return Storage.getInt(); }
 };

 /// \brief Abstract interface for a module loader.

Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/
Frontend/CompilerInstance.cpp?rev=288626&r1=288625&r2=288626&view=diff
============================================================
==================
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Sun Dec  4 16:34:37 2016
@@ -1393,21 +1393,8 @@ bool CompilerInstance::loadModuleFile(St
         if (Module *M = CI.getPreprocessor()
                             .getHeaderSearchInfo()
                             .getModuleMap()
-                            .findModule(II->getName())) {
+                            .findModule(II->getName()))
           M->HasIncompatibleModuleFile = true;
-
-          // Mark module as available if the only reason it was unavailable
-          // was missing headers.
-          SmallVector<Module *, 2> Stack;
-          Stack.push_back(M);
-          while (!Stack.empty()) {
-            Module *Current = Stack.pop_back_val();
-            if (Current->IsMissingRequirement) continue;
-            Current->IsAvailable = true;
-            Stack.insert(Stack.end(),
-                         Current->submodule_begin(),
Current->submodule_end());
-          }
-        }
       }
       LoadedModules.clear();
     }
@@ -1511,7 +1498,7 @@ CompilerInstance::loadModule(SourceLocat
       if (Module && Module->HasIncompatibleModuleFile) {
         // We tried and failed to load a module file for this module. Fall
         // back to textual inclusion for its headers.
-        return ModuleLoadResult::ConfigMismatch;
+        return ModuleLoadResult(nullptr, /*missingExpected*/true);
       }

       getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_
disabled)
@@ -1718,7 +1705,7 @@ CompilerInstance::loadModule(SourceLocat
         << Module->getFullModuleName()
         << SourceRange(Path.front().second, Path.back().second);

-      return ModuleLoadResult::MissingExpected;
+      return ModuleLoadResult(nullptr, true);
     }

     // Check whether this module is available.
@@ -1752,7 +1739,7 @@ CompilerInstance::loadModule(SourceLocat
   }

   LastModuleImportLoc = ImportLoc;
-  LastModuleImportResult = ModuleLoadResult(Module);
+  LastModuleImportResult = ModuleLoadResult(Module, false);
   return LastModuleImportResult;
 }


Modified: cfe/trunk/lib/Lex/PPDirectives.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/
PPDirectives.cpp?rev=288626&r1=288625&r2=288626&view=diff
============================================================
==================
--- cfe/trunk/lib/Lex/PPDirectives.cpp (original)
+++ cfe/trunk/lib/Lex/PPDirectives.cpp Sun Dec  4 16:34:37 2016
@@ -1866,7 +1866,10 @@ void Preprocessor::HandleIncludeDirectiv
     // unavailable, diagnose the situation and bail out.
     // FIXME: Remove this; loadModule does the same check (but produces
     // slightly worse diagnostics).
-    if (!SuggestedModule.getModule()->isAvailable()) {
+    if (!SuggestedModule.getModule()->isAvailable() &&
+        !SuggestedModule.getModule()
+             ->getTopLevelModule()
+             ->HasIncompatibleModuleFile) {
       Module::Requirement Requirement;
       Module::UnresolvedHeaderDirective MissingHeader;
       Module *M = SuggestedModule.getModule();
@@ -1915,12 +1918,9 @@ void Preprocessor::HandleIncludeDirectiv
     else if (Imported.isMissingExpected()) {
       // We failed to find a submodule that we assumed would exist
(because it
       // was in the directory of an umbrella header, for instance), but no
-      // actual module containing it exists (because the umbrella header is
+      // actual module exists for it (because the umbrella header is
       // incomplete).  Treat this as a textual inclusion.
       SuggestedModule = ModuleMap::KnownHeader();
-    } else if (Imported.isConfigMismatch()) {
-      // On a configuration mismatch, enter the header textually. We still
know
-      // that it's part of the corresponding module.
     } else {
       // We hit an error processing the import. Bail out.
       if (hadModuleLoaderFatalFailure()) {

Removed: cfe/trunk/test/Modules/config-mismatch.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
Modules/config-mismatch.cpp?rev=288625&view=auto
============================================================
==================
--- cfe/trunk/test/Modules/config-mismatch.cpp (original)
+++ cfe/trunk/test/Modules/config-mismatch.cpp (removed)
@@ -1,10 +0,0 @@
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo 'module M { header "foo.h" header "bar.h" }' > %t/map
-// RUN: echo 'template<typename T> void f(T t) { int n; t.f(n); }' >
%t/foo.h
-// RUN: touch %t/bar.h
-// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -x c++
%t/map -emit-module -fmodule-name=M -o %t/pcm
-// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility
-fmodule-map-file=%t/map -fmodule-file=%t/pcm -I%t %s -fsyntax-only
-fexceptions -Wno-module-file-config-mismatch
-// RUN: rm %t/bar.h
-// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility
-fmodule-map-file=%t/map -fmodule-file=%t/pcm -I%t %s -fsyntax-only
-fexceptions -Wno-module-file-config-mismatch
-#include "foo.h"


_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161204/4d3f6716/attachment-0001.html>
