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

[Manman via cfe-commits]

> On Dec 14, 2016, at 1:20 PM, Manman <mren at apple.com> wrote:
> 
> 
>> 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?

Hi Richard,

Can you comment on why we want to say that a PCH defines a module? Any suggestion on fixing this regression?

Manman

> 
> 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);
>> 
>> 
>