Regression caused by r276159 - [modules] Don't emit initializers for VarDecls within a module eagerly whenever

[Manman via cfe-commits]

> On Jul 20, 2016, at 12:10 PM, Richard Smith via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 
> Author: rsmith
> Date: Wed Jul 20 14:10:16 2016
> New Revision: 276159
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=276159&view=rev
> Log:
> [modules] Don't emit initializers for VarDecls within a module eagerly whenever
> we first touch any part of that module. Instead, defer them until the first
> time that module is (transitively) imported. The initializer step for a module
> then recursively initializes modules that its own headers imported.
> 
> For example, this avoids running the <iostream> global initializer in programs
> that don't actually use iostreams, but do use other parts of the standard
> library.
> 
> Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=276159&r1=276158&r2=276159&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Wed Jul 20 14:10:16 2016
> @@ -1017,6 +1017,7 @@ void ASTWriter::WriteBlockInfoBlock() {
>   RECORD(SUBMODULE_PRIVATE_HEADER);
>   RECORD(SUBMODULE_TEXTUAL_HEADER);
>   RECORD(SUBMODULE_PRIVATE_TEXTUAL_HEADER);
> +  RECORD(SUBMODULE_INITIALIZERS);
> 
>   // Comments Block.
>   BLOCK(COMMENTS_BLOCK);
> @@ -2417,7 +2418,9 @@ unsigned ASTWriter::getLocalOrImportedSu
>   if (Known != SubmoduleIDs.end())
>     return Known->second;
> 
> -  if (Mod->getTopLevelModule() != WritingModule)
> +  auto *Top = Mod->getTopLevelModule();
> +  if (Top != WritingModule &&
> +      !Top->fullModuleNameIs(StringRef(getLangOpts().CurrentModule)))
>     return 0;
> 
>   return SubmoduleIDs[Mod] = NextSubmoduleID++;
> @@ -2649,6 +2652,13 @@ void ASTWriter::WriteSubmodules(Module *
>       Stream.EmitRecordWithBlob(ConfigMacroAbbrev, Record, CM);
>     }
> 
> +    // Emit the initializers, if any.
> +    RecordData Inits;
> +    for (Decl *D : Context->getModuleInitializers(Mod))
> +      Inits.push_back(GetDeclRef(D));
> +    if (!Inits.empty())
> +      Stream.EmitRecord(SUBMODULE_INITIALIZERS, Inits);
> +
>     // Queue up the submodules of this module.
>     for (auto *M : Mod->submodules())
>       Q.push(M);
> @@ -4514,6 +4524,17 @@ uint64_t ASTWriter::WriteASTCore(Sema &S
>   // If we're emitting a module, write out the submodule information.  
>   if (WritingModule)
>     WriteSubmodules(WritingModule);
> +  else if (!getLangOpts().CurrentModule.empty()) {
> +    // If we're building a PCH in the implementation of a module, we may need
> +    // the description of the current module.

Hi Richard,

Sorry to dig up this old commit. We are seeing a regression with the following simplified testing case.
cat test.h 
#include "A.h"
cat test.m 
#include "C.h"
cat run.sh 
rm -rf tmp
clang -cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=tmp -I Inputs -emit-pch -o tmp-A.pch test.h -fmodule-name=Contacts -x objective-c-header
clang -cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=tmp -I Inputs -include-pch tmp-A.pch test.m -fsyntax-only -fmodule-name=Contacts
test.m:1:2: fatal error: module 'Contacts' is defined in both '/Users/Ren/projects/module/screen/r29542516/tmp/2BSZ9VNPW89EZ/Contacts-389KKPMFZ1XO7.pcm' and 'tmp-A.pch'
#include "C.h"
 ^
test.m:1:2: note: imported by module 'CloudKit' in '/Users/Ren/projects/module/screen/r29542516/tmp/2BSZ9VNPW89EZ/CloudKit-389KKPMFZ1XO7.pcm'
1 error generated.

cat Inputs/module.modulemap 
module CloudKit {
  header "C.h"
  export *
}

module Contacts {
  header "D.h"
  export *
}
cat Inputs/A.h
// in pch
cat Inputs/C.h
#include "D.h"
cat Inputs/D.h
//empty

We used to have a different option "-fmodule-implementation-of" which was unified with "-fmodule-name”.

When building the pch, we pass in “-fmodule-implementation-of Contacts”, because of this part of the commit, we will say the pch defines module Contacts.
The TU also pulls in module CloudKit, which imports module Contacts. Thus we have this error:
module 'Contacts' is defined in both '/Users/Ren/projects/module/screen/r29542516/tmp/2BSZ9VNPW89EZ/Contacts-389KKPMFZ1XO7.pcm' and 'tmp-A.pch’

I wonder why we want to say that a pch defines a module. Is this related to lazily evaluating initializers?

Thanks,
Manman

> +    //
> +    // FIXME: We may need other modules that we did not load from an AST file,
> +    // such as if a module declares a 'conflicts' on a different module.
> +    Module *M = PP.getHeaderSearchInfo().getModuleMap().findModule(
> +        getLangOpts().CurrentModule);
> +    if (M && !M->IsFromModuleFile)
> +      WriteSubmodules(M);
> +  }
> 
>   Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes);
> 
>