Mon Dec 5 16:50:40 PST 2016

On 4 December 2016 at 20:44, Richard Smith <richard at metafoo.co.uk> wrote:

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

Wrong diagnostic fixed in r288737, revert reverted in revert r288741 (with
the above addition to the test).

> 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/PPDire
> ctives.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/20161205/f4b6b6ee/attachment-0001.html>