Re: r280409 - When we reach the end of a #include of a header of a local submodule that we

[Manman via cfe-commits]

Hi Richard,

I fixed a regression caused by this commit in r291628.

Let me know if you see any problem!

Manman

> On Sep 1, 2016, at 1:15 PM, Richard Smith via cfe-commits 
>  wrote:
> 
> Author: rsmith
> Date: Thu Sep  1 15:15:25 2016
> New Revision: 280409
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=280409&view=rev
> Log:
> When we reach the end of a #include of a header of a local submodule that we
> textually included, create an ImportDecl just as we would if we reached a
> #include of any other modular header. This is necessary in order to correctly
> determine the set of variables to initialize for an imported module.
> 
> This should hopefully make the modules selfhost buildbot green again.
> 
> Added:
>cfe/trunk/test/Modules/global-init.cpp
> Modified:
>cfe/trunk/include/clang/Sema/Sema.h
>cfe/trunk/lib/Sema/SemaDecl.cpp
>cfe/trunk/tools/libclang/CXIndexDataConsumer.cpp
> 
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=280409&r1=280408&r2=280409&view=diff
> ==
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Sep  1 15:15:25 2016
> @@ -1884,6 +1884,7 @@ public:
>   /// \brief The parser has processed a module import translated from a
>   /// #include or similar preprocessing directive.
>   void ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
> +  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
> 
>   /// \brief The parsed has entered a submodule.
>   void ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod);
> 
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=280409&r1=280408&r2=280409&view=diff
> ==
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Sep  1 15:15:25 2016
> @@ -15312,7 +15312,10 @@ DeclResult Sema::ActOnModuleImport(Sourc
> 
> void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
>   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
> +  BuildModuleInclude(DirectiveLoc, Mod);
> +}
> 
> +void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
>   // Determine whether we're in the #include buffer for a module. The 
> #includes
>   // in that buffer do not qualify as module imports; they're just an
>   // implementation detail of us building the module.
> @@ -15352,20 +15355,27 @@ void Sema::ActOnModuleBegin(SourceLocati
>   VisibleModules.setVisible(Mod, DirectiveLoc);
> }
> 
> -void Sema::ActOnModuleEnd(SourceLocation DirectiveLoc, Module *Mod) {
> -  checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext);
> +void Sema::ActOnModuleEnd(SourceLocation EofLoc, Module *Mod) {
> +  checkModuleImportContext(*this, Mod, EofLoc, CurContext);
> 
>   if (getLangOpts().ModulesLocalVisibility) {
> -assert(!ModuleScopes.empty() && ModuleScopes.back().Module == Mod &&
> -   "left the wrong module scope");
> VisibleModules = std::move(ModuleScopes.back().OuterVisibleModules);
> -ModuleScopes.pop_back();
> -
> -VisibleModules.setVisible(Mod, DirectiveLoc);
> // Leaving a module hides namespace names, so our visible namespace cache
> // is now out of date.
> VisibleNamespaceCache.clear();
>   }
> +
> +  assert(!ModuleScopes.empty() && ModuleScopes.back().Module == Mod &&
> + "left the wrong module scope");
> +  ModuleScopes.pop_back();
> +
> +  // We got to the end of processing a #include of a local module. Create an
> +  // ImportDecl as we would for an imported module.
> +  FileID File = getSourceManager().getFileID(EofLoc);
> +  assert(File != getSourceManager().getMainFileID() &&
> + "end of submodule in main source file");
> +  SourceLocation DirectiveLoc = getSourceManager().getIncludeLoc(File);
> +  BuildModuleInclude(DirectiveLoc, Mod);
> }
> 
> void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation Loc,
> 
> Added: cfe/trunk/test/Modules/global-init.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/global-init.cpp?rev=280409&view=auto
> ==
> --- cfe/trunk/test/Modules/global-init.cpp (added)
> +++ cfe/trunk/test/Modules/global-init.cpp Thu Sep  1 15:15:25 2016
> @@ -0,0 +1,19 @@
> +// RUN: rm -rf %t
> +// RUN: mkdir %t
> +//
> +// RUN: echo '#pragma once' > %t/a.h
> +// RUN: echo 'struct A { A() {} int f() const; } const a;' >> %t/a.h
> +//
> +// RUN: echo '#include "a.h"' > %t/b.h
> +//
> +// RUN: echo 'module M { module b { header "b.h" export * } module a { 
> header "a.h" export * } }' > %t/map
> +//
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t 
> -fmodule-map-file=%t/map -I%t %s -emit-llvm

Re: 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  wrote:
> 
> 
>> On Jul 20, 2016, at 12:10 PM, Richard Smith via cfe-commits 
>>  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  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);

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r284577 - [modules] Do not report missing definitions of demoted constexpr variable templates.

[Manman via cfe-commits]

> On Oct 19, 2016, at 4:19 AM, Vassil Vassilev via cfe-commits 
>  wrote:
> 
> Author: vvassilev
> Date: Wed Oct 19 06:19:30 2016
> New Revision: 284577
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=284577&view=rev
> Log:
> [modules] Do not report missing definitions of demoted constexpr variable 
> templates.
> 
> This is a followup to regression introduced in r284284.
> 
> This should fix our libstdc++ modules builds.
> 
> https://reviews.llvm.org/D25678
> 
> Reviewed by Richard Smith!
> 
> Added:
>cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h
>cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h
>cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h
>cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap
>cfe/trunk/test/Modules/merge-var-template-def.cpp
> Modified:
>cfe/trunk/lib/Sema/SemaDecl.cpp
> 
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=284577&r1=284576&r2=284577&view=diff
> ==
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed Oct 19 06:19:30 2016
> @@ -10124,7 +10124,11 @@ void Sema::ActOnUninitializedDecl(Decl *
> // C++11 [dcl.constexpr]p1: The constexpr specifier shall be applied only 
> to
> // the definition of a variable [...] or the declaration of a static data
> // member.
> -if (Var->isConstexpr() && !Var->isThisDeclarationADefinition()) {
> +if (Var->isConstexpr() && !Var->isThisDeclarationADefinition() &&
> +!Var->isThisDeclarationADemotedDefinition()) {
> +  assert((!Var->isThisDeclarationADemotedDefinition() ||
> +  getLangOpts().Modules) &&
> + "Demoting decls is only in the contest of modules!”);

Just noticed the mismatch between this commit and the last reviewed patch :]
The assert is still here.

Manman

>   if (Var->isStaticDataMember()) {
> // C++1z removes the relevant rule; the in-class declaration is always
> // a definition there.
> 
> Added: cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h?rev=284577&view=auto
> ==
> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h (added)
> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h Wed Oct 19 
> 06:19:30 2016
> @@ -0,0 +1,8 @@
> +#ifndef A_H
> +#define A_H
> +template
> +struct S { static constexpr T value = v; };
> +template
> +constexpr T S::value;
> +
> +#endif
> 
> Added: cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h?rev=284577&view=auto
> ==
> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h (added)
> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h Wed Oct 19 
> 06:19:30 2016
> @@ -0,0 +1,9 @@
> +#ifndef B1_H
> +#define B1_H
> +template
> +struct S { static constexpr T value = v; };
> +template
> +constexpr T S::value;
> +
> +#include "a.h"
> +#endif
> 
> Added: cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h?rev=284577&view=auto
> ==
> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h (added)
> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h Wed Oct 19 
> 06:19:30 2016
> @@ -0,0 +1,9 @@
> +#ifndef B2_H
> +#define B2_H
> +
> +template
> +struct S { static constexpr T value = v; };
> +template
> +constexpr T S::value;
> +
> +#endif
> 
> Added: cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap?rev=284577&view=auto
> ==
> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap 
> (added)
> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap Wed 
> Oct 19 06:19:30 2016
> @@ -0,0 +1,5 @@
> +module a { header "a.h" export * }
> +module b {
> +  module b1 { header "b1.h" export * }
> +  module b2 { header "b2.h" export * }
> +}
> 
> Added: cfe/trunk/test/Modules/merge-var-template-def.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/merge-var-template-def.cpp?rev=284577&view=auto
> ==
> --- cfe/trunk/test/Modules/merge-var-template-def.cpp (added)
> +++ cfe/trunk/test/Modules/merge-var-template-def.cpp Wed Oct 19 06:19:

Re: [libcxx] r249738 - Split out of .

[Manman via cfe-commits]

> On Oct 17, 2016, at 2:11 PM, Bruno Cardoso Lopes via cfe-commits 
>  wrote:
> 
> Hi,
> 
> On Fri, Oct 14, 2016 at 3:09 PM, Richard Smith  wrote:
>> On Fri, Oct 14, 2016 at 11:44 AM, Bruno Cardoso Lopes
>>  wrote:
>>> 
>>> Hi Richard,
>>> 
>>> I have a patch on top of your suggested patch from a year ago, that
>>> break the cyclic dependency we're seeing, with this (and a few changes
>>> to the SDK) we can bootstrap clang with submodule local visibility on
>>> darwin. I've attached the patch with a reduced, standalone testcase
>>> that doesn't depend on the SDK. The issues that are not covered by
>>> your patch, that I cover in mine, are related to built-in and textual
>>> headers: they can be found in paths where they don't map to any
>>> modules, triggering other cycles. I fix that by looking further to
>>> find a matching module for the header in question, instead the first
>>> found header in header search.
>> 
>> 
>> It looks like the 0002 patch is working around a bug in the 0001 patch: with
>> 0001 applied, a module with [no_undeclared_includes] can still include a
>> textual header from another module on which it doesn't declare a dependency
>> (in the testcase, the libc module is incorrectly permitted to include the
>> textual header  from libc++). Rather than preferring non-modular
>> headers over modular headers as the 0002 patch does, I wonder if the issue
>> could instead be resolved by fixing that apparent bug.
> 
> Thanks for the fast answer and for the new patch :-)
> My intend with 0002 was actually to prefer modular headers instead of
> non-modular, but fallback to the later in case none is found.
> 
>> I gave that a go; the result is attached. I also had to change the way we
>> handle builtin headers -- instead of implicitly including (for instance) the
>> builtin  as a modular header in any module that provides its own
>> , I now only include it as a textual header (this also lets us use
>> the same approach for this case whether or not we're using local submodule
>> visibility).

@Richard,

I wonder why (prior to your patch) we need to use a different approach for 
builtin headers with local submodule visibility.

Thanks,

>> That exposed a couple of testcases that were (unreasonably, in
>> my opinion) failing to include_next the real builtin header from their
>> wrapper header.
> 
> I'm curious about these, are they from clang tests?
> 
>> The attached patch causes your testcase to pass; I'd be interested to know
>> if it works in practice on Darwin.
> 
> It works for building the Darwin module, but failed for "#include
> " on darwin (which was working under 0001+0002 patch):
> 
> While building module 'std' imported from all-headers.cpp:1:
> While building module 'Darwin' imported from
> /clang-install/include/c++/v1/string.h:61:
> In file included from :422:
> In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach.h:67:
> In file included from 
> /SDK/MacOSX10.12.sdk/usr/include/mach/mach_interface.h:42:
> In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/clock_priv.h:6:
> /clang-install/include/c++/v1/string.h:74:7: error: redefinition of
> '__libcpp_strchr'
> char* __libcpp_strchr(const char* __s, int __c) {return
> (char*)strchr(__s, __c);}
>  ^
> /clang-install/include/c++/v1/string.h:74:7: note: previous definition is here

@Bruno,

Can you try "-fdiagnostics-show-note-include-stack” so we know the other path 
that leads to string.h?

Manman

> char* __libcpp_strchr(const char* __s, int __c) {return
> (char*)strchr(__s, __c);}
>  ^
> While building module 'std' imported from all-headers.cpp:1:
> While building module 'Darwin' imported from
> /clang-install/include/c++/v1/string.h:61:
> In file included from :422:
> In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach.h:67:
> In file included from 
> /SDK/MacOSX10.12.sdk/usr/include/mach/mach_interface.h:42:
> In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/clock_priv.h:6:
> /clang-install/include/c++/v1/string.h:76:13: error: redefinition of 'strchr'
> const char* strchr(const char* __s, int __c) {return __libcpp_strchr(__s, 
> __c);}
>^
> /clang-install/include/c++/v1/string.h:76:13: note: previous definition is 
> here
> const char* strchr(const char* __s, int __c) {return __libcpp_strchr(__s, 
> __c);}
>^
> <...>
> While building module 'std' imported from all-headers.cpp:1:
> In file included from :1:
> In file included from /clang-install/include/c++/v1/algorithm:638:
> In file included from /clang-install/include/c++/v1/cstring:61:
> /clang-install/include/c++/v1/string.h:61:15: fatal error: could not
> build module 'Darwin'
> #include_next 
> ~^
> all-headers.cpp:1:10: fatal error: could not build module 'std'
> #include 
> ^
> 17 errors generated.
> 
> It looks like "#include_next " is poiting back to
> /clang-install/include/c++/v1/string.h
> FWIW, string.h is also in SDK/usr/include/string.h, under
> Darwin.C.string modul

Re: r276159 - [modules] Don't emit initializers for VarDecls within a module eagerly whenever

[Manman via cfe-commits]

Hi Richard,

Another follow-up commit r284263.

When we import the top-level module with “@import X;”, all the initializers in 
the submodule should be emitted.

Let me know if you see any problem,
Manman

> On Oct 13, 2016, at 4:03 PM, Richard Smith  wrote:
> 
> On Thu, Oct 13, 2016 at 11:52 AM, Manman via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Hi Richard,
> 
> I committed a follow-up patch in r284142 to fix issues with C/ObjC.
> 
> Let me know if you see any problem.
> 
> Looks good, thank you!
>  
> Manman
> 
> > On Jul 20, 2016, at 12:10 PM, Richard Smith via cfe-commits 
> > mailto:cfe-commits@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 
> > <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  global initializer in 
> > programs
> > that don't actually use iostreams, but do use other parts of the standard
> > library.
> >
> > Added:
> >cfe/trunk/test/Modules/Inputs/unused-global-init/
> >  - copied from r275623, 
> > cfe/trunk/test/Modules/Inputs/unused-global-init/
> >cfe/trunk/test/Modules/unused-global-init.cpp
> >  - copied, changed from r275623, 
> > cfe/trunk/test/Modules/unused-global-init.cpp
> > Modified:
> >cfe/trunk/include/clang/AST/ASTContext.h
> >cfe/trunk/include/clang/Sema/Sema.h
> >cfe/trunk/include/clang/Serialization/ASTBitCodes.h
> >cfe/trunk/lib/AST/ASTContext.cpp
> >cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> >cfe/trunk/lib/Sema/SemaDecl.cpp
> >cfe/trunk/lib/Sema/SemaLookup.cpp
> >cfe/trunk/lib/Serialization/ASTReader.cpp
> >cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> >cfe/trunk/lib/Serialization/ASTWriter.cpp
> >cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
> >cfe/trunk/test/Modules/Inputs/unused-global-init/used.h
> >cfe/trunk/test/Modules/odr.cpp
> >cfe/trunk/test/Modules/templates.mm <http://templates.mm/>
> >
> > Modified: cfe/trunk/include/clang/AST/ASTContext.h
> > URL: 
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=276159&r1=276158&r2=276159&view=diff
> >  
> > <http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=276159&r1=276158&r2=276159&view=diff>
> > ==
> > --- cfe/trunk/include/clang/AST/ASTContext.h (original)
> > +++ cfe/trunk/include/clang/AST/ASTContext.h Wed Jul 20 14:10:16 2016
> > @@ -312,6 +312,18 @@ class ASTContext : public RefCountedBase
> >   /// definitions of that entity.
> >   llvm::DenseMap> MergedDefModules;
> >
> > +  /// \brief Initializers for a module, in order. Each Decl will be either
> > +  /// something that has a semantic effect on startup (such as a variable 
> > with
> > +  /// a non-constant initializer), or an ImportDecl (which recursively 
> > triggers
> > +  /// initialization of another module).
> > +  struct PerModuleInitializers {
> > +llvm::SmallVector Initializers;
> > +llvm::SmallVector LazyInitializers;
> > +
> > +void resolve(ASTContext &Ctx);
> > +  };
> > +  llvm::DenseMap ModuleInitializers;
> > +
> > public:
> >   /// \brief A type synonym for the TemplateOrInstantiation mapping.
> >   typedef llvm::PointerUnion
> > @@ -883,6 +895,17 @@ public:
> > return MergedIt->second;
> >   }
> >
> > +  /// Add a declaration to the list of declarations that are initialized
> > +  /// for a module. This will typically be a global variable (with internal
> > +  /// linkage) that runs module initializers, such as the iostream 
> > initializer,
> > +  /// or an ImportDecl nominating another module that has initializers.
> > +  void addModuleInitializer(Module *M, Decl *Init);
> > +
> > +  void addLazyModuleInitializers(Module *M, ArrayRef IDs);
> > +
> > +  /// Get the initializations to perform when importing a module, if any.
>

Re: r276159 - [modules] Don't emit initializers for VarDecls within a module eagerly whenever

[Manman via cfe-commits]

Hi Richard,

I committed a follow-up patch in r284142 to fix issues with C/ObjC.

Let me know if you see any problem.

Manman

> On Jul 20, 2016, at 12:10 PM, Richard Smith via cfe-commits 
>  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  global initializer in programs
> that don't actually use iostreams, but do use other parts of the standard
> library.
> 
> Added:
>cfe/trunk/test/Modules/Inputs/unused-global-init/
>  - copied from r275623, cfe/trunk/test/Modules/Inputs/unused-global-init/
>cfe/trunk/test/Modules/unused-global-init.cpp
>  - copied, changed from r275623, 
> cfe/trunk/test/Modules/unused-global-init.cpp
> Modified:
>cfe/trunk/include/clang/AST/ASTContext.h
>cfe/trunk/include/clang/Sema/Sema.h
>cfe/trunk/include/clang/Serialization/ASTBitCodes.h
>cfe/trunk/lib/AST/ASTContext.cpp
>cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>cfe/trunk/lib/Sema/SemaDecl.cpp
>cfe/trunk/lib/Sema/SemaLookup.cpp
>cfe/trunk/lib/Serialization/ASTReader.cpp
>cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>cfe/trunk/lib/Serialization/ASTWriter.cpp
>cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
>cfe/trunk/test/Modules/Inputs/unused-global-init/used.h
>cfe/trunk/test/Modules/odr.cpp
>cfe/trunk/test/Modules/templates.mm
> 
> Modified: cfe/trunk/include/clang/AST/ASTContext.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=276159&r1=276158&r2=276159&view=diff
> ==
> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
> +++ cfe/trunk/include/clang/AST/ASTContext.h Wed Jul 20 14:10:16 2016
> @@ -312,6 +312,18 @@ class ASTContext : public RefCountedBase
>   /// definitions of that entity.
>   llvm::DenseMap> MergedDefModules;
> 
> +  /// \brief Initializers for a module, in order. Each Decl will be either
> +  /// something that has a semantic effect on startup (such as a variable 
> with
> +  /// a non-constant initializer), or an ImportDecl (which recursively 
> triggers
> +  /// initialization of another module).
> +  struct PerModuleInitializers {
> +llvm::SmallVector Initializers;
> +llvm::SmallVector LazyInitializers;
> +
> +void resolve(ASTContext &Ctx);
> +  };
> +  llvm::DenseMap ModuleInitializers;
> +
> public:
>   /// \brief A type synonym for the TemplateOrInstantiation mapping.
>   typedef llvm::PointerUnion
> @@ -883,6 +895,17 @@ public:
> return MergedIt->second;
>   }
> 
> +  /// Add a declaration to the list of declarations that are initialized
> +  /// for a module. This will typically be a global variable (with internal
> +  /// linkage) that runs module initializers, such as the iostream 
> initializer,
> +  /// or an ImportDecl nominating another module that has initializers.
> +  void addModuleInitializer(Module *M, Decl *Init);
> +
> +  void addLazyModuleInitializers(Module *M, ArrayRef IDs);
> +
> +  /// Get the initializations to perform when importing a module, if any.
> +  ArrayRef getModuleInitializers(Module *M);
> +
>   TranslationUnitDecl *getTranslationUnitDecl() const { return TUDecl; }
> 
>   ExternCContextDecl *getExternCContextDecl() const;
> 
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=276159&r1=276158&r2=276159&view=diff
> ==
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Wed Jul 20 14:10:16 2016
> @@ -1390,8 +1390,14 @@ private:
>   bool RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
>TypeDiagnoser *Diagnoser);
> 
> +  struct ModuleScope {
> +clang::Module *Module;
> +VisibleModuleSet OuterVisibleModules;
> +  };
> +  /// The modules we're currently parsing.
> +  llvm::SmallVector ModuleScopes;
> +
>   VisibleModuleSet VisibleModules;
> -  llvm::SmallVector VisibleModulesStack;
> 
>   Module *CachedFakeTopLevelModule;
> 
> 
> Modified: cfe/trunk/include/clang/Serialization/ASTBitCodes.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTBitCodes.h?rev=276159&r1=276158&r2=276159&view=diff
> ==
> --- cfe/trunk/include/clang/Serialization/ASTBitCodes.h (original)
> +++ cfe/trunk/include/clang/Serialization/ASTBitCodes.h Wed Jul 20 14:10:16 
> 2016
> @@ -684,6 +

Re: r284008 - Reinstate r283887 and r283882.

[Manman via cfe-commits]

> On Oct 12, 2016, at 1:39 PM, Vassil Vassilev  wrote:
> 
> Hi Manman,
> On 12/10/16 20:28, Manman wrote:
>> Hi Vassil,
>> 
>> Any change between this commit and “r283887 + r283882”?
> The only extra change is in:
> 
> +  bool isThisDeclarationADemotedDefinition() const {
> +return isa(this) ? false :
> +  NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
> +  }
> 
> The old patch failed because we read the IsThisDeclarationADemotedDefinition 
> bit of ParmVarDecls. We allow demoting only VarDecls, which are not 
> ParmVarDecls, and thus we serialize/deserialize this bit only for non 
> ParmVarDecls. Reading the IsThisDeclarationADemotedDefinition of ParmVarDecls 
> caused random behavior.

Thanks,

Manman

> 
> Cheers,
> Vassil
>> And what was the issue that caused the revert?
>> 
>> Thanks,
>> Manman
>> 
>>> On Oct 12, 2016, at 4:57 AM, Vassil Vassilev via cfe-commits 
>>>  wrote:
>>> 
>>> Author: vvassilev
>>> Date: Wed Oct 12 06:57:08 2016
>>> New Revision: 284008
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=284008&view=rev
>>> Log:
>>> Reinstate r283887 and r283882.
>>> 
>>> Original message:
>>> "[modules] PR28752: Do not instantiate variable declarations which are not 
>>> visible.
>>> 
>>> https://reviews.llvm.org/D24508
>>> 
>>> Patch developed in collaboration with Richard Smith!"
>>> 
>>> Added:
>>>cfe/trunk/test/Modules/Inputs/PR28752/
>>>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/
>>>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/b.h
>>>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/c.h
>>>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/module.modulemap
>>>cfe/trunk/test/Modules/Inputs/PR28752/a.h
>>>cfe/trunk/test/Modules/Inputs/PR28752/module.modulemap
>>>cfe/trunk/test/Modules/Inputs/PR28752/vector
>>>cfe/trunk/test/Modules/pr28752.cpp
>>> Modified:
>>>cfe/trunk/include/clang/AST/Decl.h
>>>cfe/trunk/lib/AST/Decl.cpp
>>>cfe/trunk/lib/Sema/SemaDecl.cpp
>>>cfe/trunk/lib/Sema/SemaTemplate.cpp
>>>cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>>>cfe/trunk/lib/Sema/SemaType.cpp
>>>cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
>>> 
>>> Modified: cfe/trunk/include/clang/AST/Decl.h
>>> URL: 
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=284008&r1=284007&r2=284008&view=diff
>>> ==
>>> --- cfe/trunk/include/clang/AST/Decl.h (original)
>>> +++ cfe/trunk/include/clang/AST/Decl.h Wed Oct 12 06:57:08 2016
>>> @@ -865,6 +865,11 @@ protected:
>>> 
>>> unsigned : NumVarDeclBits;
>>> 
>>> +// FIXME: We need something similar to CXXRecordDecl::DefinitionData.
>>> +/// \brief Whether this variable is a definition which was demoted due 
>>> to
>>> +/// module merge.
>>> +unsigned IsThisDeclarationADemotedDefinition : 1;
>>> +
>>> /// \brief Whether this variable is the exception variable in a C++ 
>>> catch
>>> /// or an Objective-C @catch statement.
>>> unsigned ExceptionVar : 1;
>>> @@ -1198,12 +1203,28 @@ public:
>>>   InitializationStyle getInitStyle() const {
>>> return static_cast(VarDeclBits.InitStyle);
>>>   }
>>> -
>>>   /// \brief Whether the initializer is a direct-initializer (list or call).
>>>   bool isDirectInit() const {
>>> return getInitStyle() != CInit;
>>>   }
>>> 
>>> +  /// \brief If this definition should pretend to be a declaration.
>>> +  bool isThisDeclarationADemotedDefinition() const {
>>> +return isa(this) ? false :
>>> +  NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
>>> +  }
>>> +
>>> +  /// \brief This is a definition which should be demoted to a declaration.
>>> +  ///
>>> +  /// In some cases (mostly module merging) we can end up with two visible
>>> +  /// definitions one of which needs to be demoted to a declaration to keep
>>> +  /// the AST invariants.
>>> +  void demoteThisDefinitionToDeclaration() {
>>> +assert (isThisDeclarationADefinition() && "Not a definition!");
>>> +assert (!isa(this) && "Cannot demote ParmVarDecls!");
>>> +NonParmVarDeclBits.IsThisDeclarationADemotedDefinition = 1;
>>> +  }
>>> +
>>>   /// \brief Determine whether this variable is the exception variable in a
>>>   /// C++ catch statememt or an Objective-C \@catch statement.
>>>   bool isExceptionVariable() const {
>>> @@ -1302,6 +1323,10 @@ public:
>>> NonParmVarDeclBits.PreviousDeclInSameBlockScope = Same;
>>>   }
>>> 
>>> +  /// \brief Retrieve the variable declaration from which this variable 
>>> could
>>> +  /// be instantiated, if it is an instantiation (rather than a 
>>> non-template).
>>> +  VarDecl *getTemplateInstantiationPattern() const;
>>> +
>>>   /// \brief If this variable is an instantiated static data member of a
>>>   /// class template specialization, returns the templated static data 
>>> member
>>>   /// from which it was instantiated.
>>> 
>>> Modified: cf

Re: r284008 - Reinstate r283887 and r283882.

[Manman via cfe-commits]

Hi Vassil,

Any change between this commit and “r283887 + r283882”?
And what was the issue that caused the revert?

Thanks,
Manman

> On Oct 12, 2016, at 4:57 AM, Vassil Vassilev via cfe-commits 
>  wrote:
> 
> Author: vvassilev
> Date: Wed Oct 12 06:57:08 2016
> New Revision: 284008
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=284008&view=rev
> Log:
> Reinstate r283887 and r283882.
> 
> Original message:
> "[modules] PR28752: Do not instantiate variable declarations which are not 
> visible.
> 
> https://reviews.llvm.org/D24508
> 
> Patch developed in collaboration with Richard Smith!"
> 
> Added:
>cfe/trunk/test/Modules/Inputs/PR28752/
>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/
>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/b.h
>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/c.h
>cfe/trunk/test/Modules/Inputs/PR28752/Subdir1/module.modulemap
>cfe/trunk/test/Modules/Inputs/PR28752/a.h
>cfe/trunk/test/Modules/Inputs/PR28752/module.modulemap
>cfe/trunk/test/Modules/Inputs/PR28752/vector
>cfe/trunk/test/Modules/pr28752.cpp
> Modified:
>cfe/trunk/include/clang/AST/Decl.h
>cfe/trunk/lib/AST/Decl.cpp
>cfe/trunk/lib/Sema/SemaDecl.cpp
>cfe/trunk/lib/Sema/SemaTemplate.cpp
>cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>cfe/trunk/lib/Sema/SemaType.cpp
>cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
> 
> Modified: cfe/trunk/include/clang/AST/Decl.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=284008&r1=284007&r2=284008&view=diff
> ==
> --- cfe/trunk/include/clang/AST/Decl.h (original)
> +++ cfe/trunk/include/clang/AST/Decl.h Wed Oct 12 06:57:08 2016
> @@ -865,6 +865,11 @@ protected:
> 
> unsigned : NumVarDeclBits;
> 
> +// FIXME: We need something similar to CXXRecordDecl::DefinitionData.
> +/// \brief Whether this variable is a definition which was demoted due to
> +/// module merge.
> +unsigned IsThisDeclarationADemotedDefinition : 1;
> +
> /// \brief Whether this variable is the exception variable in a C++ catch
> /// or an Objective-C @catch statement.
> unsigned ExceptionVar : 1;
> @@ -1198,12 +1203,28 @@ public:
>   InitializationStyle getInitStyle() const {
> return static_cast(VarDeclBits.InitStyle);
>   }
> -
>   /// \brief Whether the initializer is a direct-initializer (list or call).
>   bool isDirectInit() const {
> return getInitStyle() != CInit;
>   }
> 
> +  /// \brief If this definition should pretend to be a declaration.
> +  bool isThisDeclarationADemotedDefinition() const {
> +return isa(this) ? false :
> +  NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
> +  }
> +
> +  /// \brief This is a definition which should be demoted to a declaration.
> +  ///
> +  /// In some cases (mostly module merging) we can end up with two visible
> +  /// definitions one of which needs to be demoted to a declaration to keep
> +  /// the AST invariants.
> +  void demoteThisDefinitionToDeclaration() {
> +assert (isThisDeclarationADefinition() && "Not a definition!");
> +assert (!isa(this) && "Cannot demote ParmVarDecls!");
> +NonParmVarDeclBits.IsThisDeclarationADemotedDefinition = 1;
> +  }
> +
>   /// \brief Determine whether this variable is the exception variable in a
>   /// C++ catch statememt or an Objective-C \@catch statement.
>   bool isExceptionVariable() const {
> @@ -1302,6 +1323,10 @@ public:
> NonParmVarDeclBits.PreviousDeclInSameBlockScope = Same;
>   }
> 
> +  /// \brief Retrieve the variable declaration from which this variable could
> +  /// be instantiated, if it is an instantiation (rather than a 
> non-template).
> +  VarDecl *getTemplateInstantiationPattern() const;
> +
>   /// \brief If this variable is an instantiated static data member of a
>   /// class template specialization, returns the templated static data member
>   /// from which it was instantiated.
> 
> Modified: cfe/trunk/lib/AST/Decl.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=284008&r1=284007&r2=284008&view=diff
> ==
> --- cfe/trunk/lib/AST/Decl.cpp (original)
> +++ cfe/trunk/lib/AST/Decl.cpp Wed Oct 12 06:57:08 2016
> @@ -1926,6 +1926,9 @@ VarDecl::isThisDeclarationADefinition(AS
>   //
>   // FIXME: How do you declare (but not define) a partial specialization of
>   // a static data member template outside the containing class?
> +  if (isThisDeclarationADemotedDefinition())
> +return DeclarationOnly;
> +
>   if (isStaticDataMember()) {
> if (isOutOfLine() &&
> !(getCanonicalDecl()->isInline() &&
> @@ -2250,6 +2253,56 @@ bool VarDecl::checkInitIsICE() const {
>   return Eval->IsICE;
> }
> 
> +VarDecl *VarDecl::getTemplateInstantiationPattern() const {
> +  // If it's a variable template sp

Re: Modules: Suggestion on how to fix a regression caused by r259901

[Manman via cfe-commits]

Hi Richard,

Any suggestion on this?

Thanks,
Manman

> On Sep 7, 2016, at 12:40 PM, Manman  wrote:
> 
> Hi Richard,
> 
> We noticed a regression for this simple testing case:
> rm -rf tmp3
> clang -cc1 -fimplicit-module-maps -x objective-c -fmodules 
> -fmodules-cache-path=tmp3 -emit-obj standalone.c -I Inputs/
> —>
> standalone.c:4:6: error: variable has incomplete type 'void'
> void foo __P(());
> ^
> standalone.c:4:9: error: expected ';' after top level declarator
> void foo __P(());
>^
>;
> 2 errors generated.
> clang -cc1 -fimplicit-module-maps -x objective-c -fmodules 
> -fmodules-cache-path=tmp3 -emit-obj standalone.c -I Inputs/
> —> This runs fine.
> 
> cat standalone.c
> #import "C.h"
> #import "A.h"
> 
> void foo __P(());
> 
> cat Inputs/module.map 
> module X {
>  header "A.h"
>  export *
> }
> // Y imports X, it also uses “__P” as a parameter name
> module Y {
>  header "B.h"
>  export *
> }
> // Z imports X and Y
> module Z {
>  header "C.h”
> }
> 
> cat Inputs/A.h 
> #define __P(protos) ()
> 
> cat Inputs/B.h
> #import "A.h"
> #import "B2.h”
> 
> cat Inputs/B2.h 
> void test(int __P) {
> }
> 
> cat Inputs/C.h 
> #import "A.h"
> #import “B.h”
> 
> r259901 causes the compiler to write out identifier “__P” without the macro 
> information for module Y, which seems to be incorrect. Any suggestion on how 
> to fix this?
> Why the 2nd run works is related to global index. Global Index only considers 
> interesting identifiers, so it skips module Y when calling 
> ModuleManager::visit.
> 
> Cheers,
> Manman
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Submodule semantics with macro guard

[Manman via cfe-commits]

> On Sep 19, 2016, at 5:55 PM, Richard Smith via cfe-commits 
>  wrote:
> 
> Your c.h is not correct. It would introduce a definition of c in every file 
> where it's included, so it's not a modular header.
Hi Richard,

What do you mean by c.h is not correct? It is guarded by a macro, so if we are 
not using modules, it will work.

Also I thought the definition of a non-modular header is that it is not mapped 
to any module. Is that my misunderstanding? Is there a formal definition for 
modular header?

Thanks,
Manman
> 
>> On 19 Sep 2016 5:21 pm, "Manman via cfe-commits" 
>>  wrote:
>> 
>> Hi Richard & Ben,
>> 
>> Given a simple testing case, where we have two submodules X.A (A.h) and X.B 
>> (B.h, it includes C.h, and C.h is guarded with a macro), when we import X.A 
>> and then textually include a header C.h, we get redefinition error. This is 
>> because the macro guard is owned by module X.B that is not yet visible. I 
>> wonder what the fix is if this is considered a compiler issue. We can:
>> 1> do not emit the redefinition error if the earlier definition is not 
>> visible, or
>> 2> emit diagnostics to suggest user to import the whole module, when 
>> redefinition happens because the module includes a non-modular header and 
>> the non-modular header is also textually included, or
>> 3> other suggestions?
>> 
>> Note that if we textually include C.h, then import X.A, there are no 
>> compiler errors.
>> 
>> The testing case here:
>> clang -cc1 repro.c -fsyntax-only -I Inputs4/ -fmodules 
>> -fimplicit-module-maps -fmodules-cache-path=tmp
>> cat repro.c
>> #include "A.h" //modular header
>> #include "C.h" //non-modular header
>> 
>> cat Inputs4/module.map
>> module X {
>>   module A {
>> header "A.h"
>> export *
>>   }
>>   module B {
>> header "B.h"
>> export *
>>   }
>>   export *
>> }
>> cat Inputs4/A.h
>> #ifndef __A_h__
>> #define __A_h__
>> #endif
>> cat Inputs4/B.h
>> #ifndef __B_h__
>> #define __B_h__
>> #include "C.h"
>> #endif
>> cat Inputs4/C.h
>> #ifndef __C_h__
>> #define __C_h__
>> int c = 1;
>> const int d = 2;
>> #endif
>> 
>> Thanks,
>> Manman
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Submodule semantics with macro guard

[Manman via cfe-commits]

Hi Richard & Ben,

Given a simple testing case, where we have two submodules X.A (A.h) and X.B 
(B.h, it includes C.h, and C.h is guarded with a macro), when we import X.A and 
then textually include a header C.h, we get redefinition error. This is because 
the macro guard is owned by module X.B that is not yet visible. I wonder what 
the fix is if this is considered a compiler issue. We can:
1> do not emit the redefinition error if the earlier definition is not visible, 
or
2> emit diagnostics to suggest user to import the whole module, when 
redefinition happens because the module includes a non-modular header and the 
non-modular header is also textually included, or
3> other suggestions?

Note that if we textually include C.h, then import X.A, there are no compiler 
errors.

The testing case here:
clang -cc1 repro.c -fsyntax-only -I Inputs4/ -fmodules -fimplicit-module-maps 
-fmodules-cache-path=tmp
cat repro.c
#include "A.h" //modular header
#include "C.h" //non-modular header

cat Inputs4/module.map 
module X {
  module A {
header "A.h"
export *
  }
  module B {
header "B.h"
export *
  }
  export *
}
cat Inputs4/A.h
#ifndef __A_h__
#define __A_h__
#endif
cat Inputs4/B.h
#ifndef __B_h__
#define __B_h__
#include "C.h"
#endif
cat Inputs4/C.h
#ifndef __C_h__
#define __C_h__
int c = 1;
const int d = 2;
#endif

Thanks,
Manman
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r281351 - Add a class ObjCProtocolQualifiers to wrap APIs for ObjC protocol list.

[Manman via cfe-commits]

I checked in r281404. Hopefully it will fix the issue.

Let me know if it does not.

Thanks,
Manman

> On Sep 13, 2016, at 3:03 PM, Artem Belevich via cfe-commits 
>  wrote:
> 
> Manman,
> 
> FYI, It appears that some of your ObjC commits today trigger asan error.
> Sanitizer bots are broken by PR30341, so they don't report the issue yet.
> 
> --Artem
> 
> $ llvm/tools/clang/clang -cc1 -internal-isystem 
> llvm/tools/clang/staging/include -nostdsysteminc -fblocks -fsyntax-only 
> -Wnullable-to-nonnull-conversion 
> llvm/tools/clang/test/SemaObjC/parameterized_classes_subst.m -verify
> =
> ==223101==ERROR: AddressSanitizer: use-after-poison on address 0x62191288 
> at pc 0x01a02b3f bp 0x7f2996f0 sp 0x7f2996e8
> WRITE of size 4 at 0x62191288 thread T0
> #0 0x1a02b3e in 
> clang::ObjCTypeParamTypeLoc::setProtocolRAngleLoc(clang::SourceLocation) 
> third_party/llvm/llvm/tools/clang/include/clang/AST/TypeLoc.h:737:55
> #1 0x213f86b in 
> clang::Sema::actOnObjCTypeArgsAndProtocolQualifiers(clang::Scope*, 
> clang::SourceLocation, clang::OpaquePtr, 
> clang::SourceLocation, llvm::ArrayRef >, 
> clang::SourceLocation, clang::SourceLocation, llvm::ArrayRef, 
> llvm::ArrayRef, clang::SourceLocation) 
> third_party/llvm/llvm/tools/clang/lib/Sema/SemaType.cpp:1165:13
> #2 0x2060347 in 
> clang::Parser::parseObjCTypeArgsAndProtocolQualifiers(clang::SourceLocation, 
> clang::OpaquePtr, bool, clang::SourceLocation&) 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseObjc.cpp:1878:18
> #3 0x1ffca6f in 
> clang::Parser::TryAnnotateTypeOrScopeTokenAfterScopeSpec(bool, bool, 
> clang::CXXScopeSpec&, bool) 
> third_party/llvm/llvm/tools/clang/lib/Parse/Parser.cpp:1770:13
> #4 0x1ffd4fc in clang::Parser::TryAnnotateTypeOrScopeToken(bool, bool) 
> third_party/llvm/llvm/tools/clang/lib/Parse/Parser.cpp:1733:10
> #5 0x20df910 in clang::Parser::isTypeSpecifierQualifier() 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseDecl.cpp:4381:9
> #6 0x205ecf5 in clang::Parser::ParseObjCTypeName(clang::ObjCDeclSpec&, 
> clang::Declarator::TheContext, clang::ParsedAttributes*) 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseObjc.cpp:1274:7
> #7 0x205c8f9 in clang::Parser::ParseObjCMethodDecl(clang::SourceLocation, 
> clang::tok::TokenKind, clang::tok::ObjCKeywordKind, bool) 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseObjc.cpp:1428:22
> #8 0x2058e8b in 
> clang::Parser::ParseObjCInterfaceDeclList(clang::tok::ObjCKeywordKind, 
> clang::Decl*) third_party/llvm/llvm/tools/clang/lib/Parse/ParseObjc.cpp:633:11
> #9 0x2053154 in 
> clang::Parser::ParseObjCAtInterfaceDeclaration(clang::SourceLocation, 
> clang::ParsedAttributes&) 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseObjc.cpp:388:3
> #10 0x2051fa9 in clang::Parser::ParseObjCAtDirectives() 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseObjc.cpp:63:18
> #11 0x1ff7871 in 
> clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&,
>  clang::ParsingDeclSpec*) 
> third_party/llvm/llvm/tools/clang/lib/Parse/Parser.cpp:757:12
> #12 0x1ff6c61 in 
> clang::Parser::ParseTopLevelDecl(clang::OpaquePtr&) 
> third_party/llvm/llvm/tools/clang/lib/Parse/Parser.cpp:628:12
> #13 0x1ff016d in clang::ParseAST(clang::Sema&, bool, bool) 
> third_party/llvm/llvm/tools/clang/lib/Parse/ParseAST.cpp:147:18
> #14 0x1ab05d3 in clang::FrontendAction::Execute() 
> third_party/llvm/llvm/tools/clang/lib/Frontend/FrontendAction.cpp:458:8
> #15 0x184fbd6 in 
> clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) 
> third_party/llvm/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:871:11
> #16 0x5c6f47 in 
> clang::ExecuteCompilerInvocation(clang::CompilerInstance*) 
> third_party/llvm/llvm/tools/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:249:25
> #17 0x5a114d in cc1_main(llvm::ArrayRef, char const*, void*) 
> third_party/llvm/llvm/tools/clang/tools/driver/cc1_main.cpp:183:13
> #18 0x5bb3f6 in ExecuteCC1Tool(llvm::ArrayRef, 
> llvm::StringRef) 
> third_party/llvm/llvm/tools/clang/tools/driver/driver.cpp:299:12
> #19 0x5ba314 in main 
> third_party/llvm/llvm/tools/clang/tools/driver/driver.cpp:380:12
> 
> 0x62191288 is located 392 bytes inside of 4096-byte region 
> [0x62191100,0x62192100)
> allocated by thread T0 here:
> #0 0x58a52c in __interceptor_malloc 
> third_party/llvm/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3
> #1 0x5a5d0b in llvm::MallocAllocator::Allocate(unsigned long, unsigned 
> long) third_party/llvm/llvm/include/llvm/Support/Allocator.h:95:12
> #2 0x691b67 in llvm::BumpPtrAllocatorImpl 4096ul>::StartNewSlab() 
> third_party/llvm/llvm/include/llvm/Support/Allocator.h:324:31
> #3 0x6916aa in llvm::BumpPtrAllocatorImpl 4096ul>::Allocate(unsigned long, unsigned long) 
> third_party/llvm/llvm/include/llvm/Support/Allocator.h:249:5
> 

Modules: Suggestion on how to fix a regression caused by r259901

[Manman via cfe-commits]

Hi Richard,

We noticed a regression for this simple testing case:
rm -rf tmp3
clang -cc1 -fimplicit-module-maps -x objective-c -fmodules 
-fmodules-cache-path=tmp3 -emit-obj standalone.c -I Inputs/
—>
standalone.c:4:6: error: variable has incomplete type 'void'
void foo __P(());
 ^
standalone.c:4:9: error: expected ';' after top level declarator
void foo __P(());
^
;
2 errors generated.
clang -cc1 -fimplicit-module-maps -x objective-c -fmodules 
-fmodules-cache-path=tmp3 -emit-obj standalone.c -I Inputs/
—> This runs fine.

cat standalone.c
#import "C.h"
#import "A.h"

void foo __P(());

cat Inputs/module.map 
module X {
  header "A.h"
  export *
}
// Y imports X, it also uses “__P” as a parameter name
module Y {
  header "B.h"
  export *
}
// Z imports X and Y
module Z {
  header "C.h”
}

cat Inputs/A.h 
#define __P(protos) ()

cat Inputs/B.h
#import "A.h"
#import "B2.h”

cat Inputs/B2.h 
void test(int __P) {
}

cat Inputs/C.h 
#import "A.h"
#import “B.h”

r259901 causes the compiler to write out identifier “__P” without the macro 
information for module Y, which seems to be incorrect. Any suggestion on how to 
fix this?
Why the 2nd run works is related to global index. Global Index only considers 
interesting identifiers, so it skips module Y when calling ModuleManager::visit.

Cheers,
Manman

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] arc-repeated-use-of-weak should not warn about IBOutlet properties

[Manman via cfe-commits]

> On May 23, 2016, at 8:15 PM, Bob Wilson  wrote:
> 
> Revision r211132 was supposed to disable -Warc-repeated-use-of-weak for 
> Objective-C properties marked with the IBOutlet attribute. Those properties 
> are supposed to be weak but they are only accessed from the main thread so 
> there is no risk of asynchronous updates setting them to nil. That 
> combination makes -Warc-repeated-use-of-weak very noisy. The previous change 
> only handled one kind of access to weak IBOutlet properties. Instead of 
> trying to add checks for all the different kinds of property accesses, this 
> patch removes the previous special case check and adds a check at the point 
> where the diagnostic is reported.

The approach looks good to me in general. 
> diff --git lib/Sema/AnalysisBasedWarnings.cpp 
> lib/Sema/AnalysisBasedWarnings.cpp
> index 3f2c41b..eb45315 100644
> --- lib/Sema/AnalysisBasedWarnings.cpp
> +++ lib/Sema/AnalysisBasedWarnings.cpp
> @@ -1334,6 +1334,12 @@ static void diagnoseRepeatedUseOfWeak(Sema &S,
>  else
>llvm_unreachable("Unexpected weak object kind!");
>  
> +// Do not warn about IBOutlet weak property receivers being set to null
> +// since they are typically only used from the main thread.
> +if (const ObjCPropertyDecl *Prop = dyn_cast(D))
> +  if (Prop->hasAttr())
> +continue;
> +
>  // Show the first time the object was read.
>  S.Diag(FirstRead->getLocStart(), DiagKind)
><< int(ObjectKind) << D << int(FunctionKind)

Should we guard the call to diagnoseRepeatedUseOfWeak, instead of checking the 
decl inside a loop in diagnoseRepeatedUseOfWeak?

  if (S.getLangOpts().ObjCWeak &&
  !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, D->getLocStart()))
diagnoseRepeatedUseOfWeak(S, fscope, D, AC.getParentMap());
—> check IBOutlet here?

> diff --git lib/Sema/SemaPseudoObject.cpp lib/Sema/SemaPseudoObject.cpp
> index 62c823b..c93d800 100644
> --- lib/Sema/SemaPseudoObject.cpp
> +++ lib/Sema/SemaPseudoObject.cpp
> @@ -578,7 +578,7 @@ bool ObjCPropertyOpBuilder::isWeakProperty() const {
>if (RefExpr->isExplicitProperty()) {
>  const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty();
>  if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
> -  return !Prop->hasAttr();
> +  return true;
>  
>  T = Prop->getType();
>} else if (Getter) {

I wonder if this change is necessary to make the testing case pass, or is it 
introduced for clarity, to better reflect the function name isWeakProperty?

Thanks,
Manman

> rdar://problem/21366461
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits