Re: [clang] 9791b58 - [C++20 Modules] Don't create global module fragment for extern linkage declaration in GMF already

2021-12-14 Thread chuanqi.xcq via cfe-commits 
Hi Richard,

   Sorry for disturbing. I sent https://reviews.llvm.org/D115610 to try to fix 
this. 

BTW, the example you posted might not work with libstdc++, here is a bug I 
reported: https://github.com/llvm/llvm-project/issues/51873

Thanks,
Chuanqi



--
From:chuanqi.xcq 
Send Time:2021年12月13日(星期一) 10:14
To:Chuanqi Xu ; Richard Smith 
Cc:cfe-commits 
Subject:Re: [clang] 9791b58 - [C++20 Modules] Don't create global module 
fragment for extern linkage declaration in GMF already

Hi Richard,

Thanks for reporting this! I found the reason that why it didn't get tested 
previously is that we didn't use `extern "C"/"C++"` actually since we 
implemented partitions internally. Sorry for confusing. I would try to work on 
it.

Thanks,
Chuanqi

--
From:Richard Smith 
Send Time:2021年12月11日(星期六) 08:28
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
Subject:Re: [clang] 9791b58 - [C++20 Modules] Don't create global module 
fragment for extern linkage declaration in GMF already

On Wed, 8 Dec 2021 at 21:56, Chuanqi Xu via cfe-commits 
 wrote:

 Author: Chuanqi Xu
 Date: 2021-12-09T13:55:15+08:00
 New Revision: 9791b589516b644a6273607b46a9c6661993d667

 URL: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667
 DIFF: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667.diff

 LOG: [C++20 Modules] Don't create global module fragment for extern linkage 
declaration in GMF already

 Previously we would create global module fragment for extern linkage
 declaration which is alreday in global module fragment. However, it is
 clearly redundant to do so. This patch would check if the extern linkage
 declaration are already in GMF before we create a GMF for it.

I'm still seeing some problems even after this patch -- linkage spec 
declarations within the purview of the module still create new global module 
fragments, and then the serialization code seems to sometimes get confused 
because we have multiple submodules with the same name "". Perhaps we 
should reuse an existing global module fragment if there is one, even when 
inside the purview of the module?

Here's an example end-to-end testcase for which I'm seeing an assertion 
failure; I've not got a reduced testcase yet:

$ cat say_hello.cppm
module;
#include 
#include 
export module Hello;
export void SayHello
  (std::string_view const )
{
  std::cout << "Hello " << name << "!\n";
}

$ cat hello.cpp
#include 
import Hello;
int main() {
  SayHello("world");
  return 0;
}

$ ./build/bin/clang++ say_hello.cppm --precompile -o say_hello.pcm -std=c++20
$ ./build/bin/clang++ say_hello.cppm hello.cpp -fmodule-file=say_hello.pcm 
-std=c++20

 Added: 
 clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 clang/test/CXX/module/module.unit/p7/t7.cpp

 Modified: 
 clang/include/clang/Sema/Sema.h
 clang/lib/Sema/SemaDeclCXX.cpp

 Removed: 



 

 diff  --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
 index a12dd4db66c13..73c5ad1dd7acf 100644
 --- a/clang/include/clang/Sema/Sema.h
 +++ b/clang/include/clang/Sema/Sema.h
 @@ -,6 +,12 @@ class Sema final {
  return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
}

 +  /// Helper function to judge if we are in module purview.
 +  /// Return false if we are not in a module.
 +  bool isCurrentModulePurview() const {
 +return getCurrentModule() ? getCurrentModule()->isModulePurview() : false;
 +  }
 +
/// Enter the scope of the global module.
Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);
/// Leave the scope of the global module.

 diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
 index 4a0eda2a700fe..3f6bde1b9ed6a 100644
 --- a/clang/lib/Sema/SemaDeclCXX.cpp
 +++ b/clang/lib/Sema/SemaDeclCXX.cpp
 @@ -16153,7 +16153,10 @@ Decl *Sema::ActOnStartLinkageSpecification(Scope *S, 
SourceLocation ExternLoc,
///   - ...
///   - appears within a linkage-specification,
///   it is attached to the global module.
 -  if (getLangOpts().CPlusPlusModules && getCurrentModule()) {
 +  ///
 +  /// If the declaration is already in global module fragment, we don't
 +  /// need to attach it again.
 +  if (getLangOpts().CPlusPlusModules && isCurrentModulePurview()) {
  Module *GlobalModule =
  PushGlobalModuleFragment(ExternLoc, /*IsImplicit=*/true);
  D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
 @@ -16177,7 +16180,11 @@ Decl *Sema::ActOnFinishLinkageSpecification(Scope *S,
  LSDecl->setRBraceLoc(RBraceLoc);
}

 -  if (getLangOpts().CPlusPlusModules && getCurrentModule())
 +

Re: [clang] 9791b58 - [C++20 Modules] Don't create global module fragment for extern linkage declaration in GMF already

2021-12-13 Thread chuanqi.xcq via cfe-commits 
Hi Richard,

Thanks for reporting this! I found the reason that why it didn't get tested 
previously is that we didn't use `extern "C"/"C++"` actually since we 
implemented partitions internally. Sorry for confusing. I would try to work on 
it.

Thanks,
Chuanqi


--
From:Richard Smith 
Send Time:2021年12月11日(星期六) 08:28
To:Chuanqi Xu ; Chuanqi Xu 
Cc:cfe-commits 
Subject:Re: [clang] 9791b58 - [C++20 Modules] Don't create global module 
fragment for extern linkage declaration in GMF already

On Wed, 8 Dec 2021 at 21:56, Chuanqi Xu via cfe-commits 
 wrote:

 Author: Chuanqi Xu
 Date: 2021-12-09T13:55:15+08:00
 New Revision: 9791b589516b644a6273607b46a9c6661993d667

 URL: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667
 DIFF: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667.diff

 LOG: [C++20 Modules] Don't create global module fragment for extern linkage 
declaration in GMF already

 Previously we would create global module fragment for extern linkage
 declaration which is alreday in global module fragment. However, it is
 clearly redundant to do so. This patch would check if the extern linkage
 declaration are already in GMF before we create a GMF for it.

I'm still seeing some problems even after this patch -- linkage spec 
declarations within the purview of the module still create new global module 
fragments, and then the serialization code seems to sometimes get confused 
because we have multiple submodules with the same name "". Perhaps we 
should reuse an existing global module fragment if there is one, even when 
inside the purview of the module?

Here's an example end-to-end testcase for which I'm seeing an assertion 
failure; I've not got a reduced testcase yet:

$ cat say_hello.cppm
module;
#include 
#include 
export module Hello;
export void SayHello
  (std::string_view const )
{
  std::cout << "Hello " << name << "!\n";
}

$ cat hello.cpp
#include 
import Hello;
int main() {
  SayHello("world");
  return 0;
}

$ ./build/bin/clang++ say_hello.cppm --precompile -o say_hello.pcm -std=c++20
$ ./build/bin/clang++ say_hello.cppm hello.cpp -fmodule-file=say_hello.pcm 
-std=c++20

 Added: 
 clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 clang/test/CXX/module/module.unit/p7/t7.cpp

 Modified: 
 clang/include/clang/Sema/Sema.h
 clang/lib/Sema/SemaDeclCXX.cpp

 Removed: 



 

 diff  --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
 index a12dd4db66c13..73c5ad1dd7acf 100644
 --- a/clang/include/clang/Sema/Sema.h
 +++ b/clang/include/clang/Sema/Sema.h
 @@ -,6 +,12 @@ class Sema final {
  return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
}

 +  /// Helper function to judge if we are in module purview.
 +  /// Return false if we are not in a module.
 +  bool isCurrentModulePurview() const {
 +return getCurrentModule() ? getCurrentModule()->isModulePurview() : false;
 +  }
 +
/// Enter the scope of the global module.
Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);
/// Leave the scope of the global module.

 diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
 index 4a0eda2a700fe..3f6bde1b9ed6a 100644
 --- a/clang/lib/Sema/SemaDeclCXX.cpp
 +++ b/clang/lib/Sema/SemaDeclCXX.cpp
 @@ -16153,7 +16153,10 @@ Decl *Sema::ActOnStartLinkageSpecification(Scope *S, 
SourceLocation ExternLoc,
///   - ...
///   - appears within a linkage-specification,
///   it is attached to the global module.
 -  if (getLangOpts().CPlusPlusModules && getCurrentModule()) {
 +  ///
 +  /// If the declaration is already in global module fragment, we don't
 +  /// need to attach it again.
 +  if (getLangOpts().CPlusPlusModules && isCurrentModulePurview()) {
  Module *GlobalModule =
  PushGlobalModuleFragment(ExternLoc, /*IsImplicit=*/true);
  D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
 @@ -16177,7 +16180,11 @@ Decl *Sema::ActOnFinishLinkageSpecification(Scope *S,
  LSDecl->setRBraceLoc(RBraceLoc);
}

 -  if (getLangOpts().CPlusPlusModules && getCurrentModule())
 +  // If the current module doesn't has Parent, it implies that the
 +  // LinkageSpec isn't in the module created by itself. So we don't
 +  // need to pop it.
 +  if (getLangOpts().CPlusPlusModules && getCurrentModule() &&
 +  getCurrentModule()->isGlobalModule() && getCurrentModule()->Parent)
  PopGlobalModuleFragment();

PopDeclContext();

 diff  --git a/clang/test/CXX/module/module.unit/p7/Inputs/h7.h 
b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
 new file mode 100644
 index 0..cd3b67

Re: [clang] 9791b58 - [C++20 Modules] Don't create global module fragment for extern linkage declaration in GMF already

2021-12-10 Thread Richard Smith via cfe-commits 
On Wed, 8 Dec 2021 at 21:56, Chuanqi Xu via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

>
> Author: Chuanqi Xu
> Date: 2021-12-09T13:55:15+08:00
> New Revision: 9791b589516b644a6273607b46a9c6661993d667
>
> URL:
> https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667
> DIFF:
> https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667.diff
>
> LOG: [C++20 Modules] Don't create global module fragment for extern
> linkage declaration in GMF already
>
> Previously we would create global module fragment for extern linkage
> declaration which is alreday in global module fragment. However, it is
> clearly redundant to do so. This patch would check if the extern linkage
> declaration are already in GMF before we create a GMF for it.
>

I'm still seeing some problems even after this patch -- linkage spec
declarations within the purview of the module still create new global
module fragments, and then the serialization code seems to sometimes get
confused because we have multiple submodules with the same name "".
Perhaps we should reuse an existing global module fragment if there is one,
even when inside the purview of the module?

Here's an example end-to-end testcase for which I'm seeing an assertion
failure; I've not got a reduced testcase yet:

$ cat say_hello.cppm
module;
#include 
#include 
export module Hello;
export void SayHello
  (std::string_view const )
{
  std::cout << "Hello " << name << "!\n";
}

$ cat hello.cpp
#include 
import Hello;
int main() {
  SayHello("world");
  return 0;
}

$ ./build/bin/clang++ say_hello.cppm --precompile -o say_hello.pcm
-std=c++20
$ ./build/bin/clang++ say_hello.cppm hello.cpp -fmodule-file=say_hello.pcm
-std=c++20

Added:
> clang/test/CXX/module/module.unit/p7/Inputs/h7.h
> clang/test/CXX/module/module.unit/p7/t7.cpp
>
> Modified:
> clang/include/clang/Sema/Sema.h
> clang/lib/Sema/SemaDeclCXX.cpp
>
> Removed:
>
>
>
>
> 
> diff  --git a/clang/include/clang/Sema/Sema.h
> b/clang/include/clang/Sema/Sema.h
> index a12dd4db66c13..73c5ad1dd7acf 100644
> --- a/clang/include/clang/Sema/Sema.h
> +++ b/clang/include/clang/Sema/Sema.h
> @@ -,6 +,12 @@ class Sema final {
>  return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
>}
>
> +  /// Helper function to judge if we are in module purview.
> +  /// Return false if we are not in a module.
> +  bool isCurrentModulePurview() const {
> +return getCurrentModule() ? getCurrentModule()->isModulePurview() :
> false;
> +  }
> +
>/// Enter the scope of the global module.
>Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool
> IsImplicit);
>/// Leave the scope of the global module.
>
> diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp
> b/clang/lib/Sema/SemaDeclCXX.cpp
> index 4a0eda2a700fe..3f6bde1b9ed6a 100644
> --- a/clang/lib/Sema/SemaDeclCXX.cpp
> +++ b/clang/lib/Sema/SemaDeclCXX.cpp
> @@ -16153,7 +16153,10 @@ Decl *Sema::ActOnStartLinkageSpecification(Scope
> *S, SourceLocation ExternLoc,
>///   - ...
>///   - appears within a linkage-specification,
>///   it is attached to the global module.
> -  if (getLangOpts().CPlusPlusModules && getCurrentModule()) {
> +  ///
> +  /// If the declaration is already in global module fragment, we don't
> +  /// need to attach it again.
> +  if (getLangOpts().CPlusPlusModules && isCurrentModulePurview()) {
>  Module *GlobalModule =
>  PushGlobalModuleFragment(ExternLoc, /*IsImplicit=*/true);
>  D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
> @@ -16177,7 +16180,11 @@ Decl *Sema::ActOnFinishLinkageSpecification(Scope
> *S,
>  LSDecl->setRBraceLoc(RBraceLoc);
>}
>
> -  if (getLangOpts().CPlusPlusModules && getCurrentModule())
> +  // If the current module doesn't has Parent, it implies that the
> +  // LinkageSpec isn't in the module created by itself. So we don't
> +  // need to pop it.
> +  if (getLangOpts().CPlusPlusModules && getCurrentModule() &&
> +  getCurrentModule()->isGlobalModule() && getCurrentModule()->Parent)
>  PopGlobalModuleFragment();
>
>PopDeclContext();
>
> diff  --git a/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
> b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
> new file mode 100644
> index 0..cd3b6706d561c
> --- /dev/null
> +++ b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
> @@ -0,0 +1,10 @@
> +#ifndef H7_H
> +#define H7_H
> +
> +extern "C++" {
> +class A {};
> +}
> +
> +class B : public A {};
> +
> +#endif
>
> diff  --git a/clang/test/CXX/module/module.unit/p7/t7.cpp
> b/clang/test/CXX/module/module.unit/p7/t7.cpp
> new file mode 100644
> index 0..7ce0cbb964131
> --- /dev/null
> +++ b/clang/test/CXX/module/module.unit/p7/t7.cpp
> @@ -0,0 +1,7 @@
> +// RUN: rm -fr %t
> +// RUN: mkdir %t
> +// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ %s -verify
> +//

[clang] 9791b58 - [C++20 Modules] Don't create global module fragment for extern linkage declaration in GMF already

2021-12-08 Thread Chuanqi Xu via cfe-commits 

Author: Chuanqi Xu
Date: 2021-12-09T13:55:15+08:00
New Revision: 9791b589516b644a6273607b46a9c6661993d667

URL: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667
DIFF: 
https://github.com/llvm/llvm-project/commit/9791b589516b644a6273607b46a9c6661993d667.diff

LOG: [C++20 Modules] Don't create global module fragment for extern linkage 
declaration in GMF already

Previously we would create global module fragment for extern linkage
declaration which is alreday in global module fragment. However, it is
clearly redundant to do so. This patch would check if the extern linkage
declaration are already in GMF before we create a GMF for it.

Added: 
clang/test/CXX/module/module.unit/p7/Inputs/h7.h
clang/test/CXX/module/module.unit/p7/t7.cpp

Modified: 
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDeclCXX.cpp

Removed: 




diff  --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a12dd4db66c13..73c5ad1dd7acf 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -,6 +,12 @@ class Sema final {
 return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
   }
 
+  /// Helper function to judge if we are in module purview.
+  /// Return false if we are not in a module.
+  bool isCurrentModulePurview() const {
+return getCurrentModule() ? getCurrentModule()->isModulePurview() : false;
+  }
+
   /// Enter the scope of the global module.
   Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);
   /// Leave the scope of the global module.

diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 4a0eda2a700fe..3f6bde1b9ed6a 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -16153,7 +16153,10 @@ Decl *Sema::ActOnStartLinkageSpecification(Scope *S, 
SourceLocation ExternLoc,
   ///   - ...
   ///   - appears within a linkage-specification,
   ///   it is attached to the global module.
-  if (getLangOpts().CPlusPlusModules && getCurrentModule()) {
+  ///
+  /// If the declaration is already in global module fragment, we don't
+  /// need to attach it again.
+  if (getLangOpts().CPlusPlusModules && isCurrentModulePurview()) {
 Module *GlobalModule =
 PushGlobalModuleFragment(ExternLoc, /*IsImplicit=*/true);
 D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
@@ -16177,7 +16180,11 @@ Decl *Sema::ActOnFinishLinkageSpecification(Scope *S,
 LSDecl->setRBraceLoc(RBraceLoc);
   }
 
-  if (getLangOpts().CPlusPlusModules && getCurrentModule())
+  // If the current module doesn't has Parent, it implies that the
+  // LinkageSpec isn't in the module created by itself. So we don't
+  // need to pop it.
+  if (getLangOpts().CPlusPlusModules && getCurrentModule() &&
+  getCurrentModule()->isGlobalModule() && getCurrentModule()->Parent)
 PopGlobalModuleFragment();
 
   PopDeclContext();

diff  --git a/clang/test/CXX/module/module.unit/p7/Inputs/h7.h 
b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
new file mode 100644
index 0..cd3b6706d561c
--- /dev/null
+++ b/clang/test/CXX/module/module.unit/p7/Inputs/h7.h
@@ -0,0 +1,10 @@
+#ifndef H7_H
+#define H7_H
+
+extern "C++" {
+class A {};
+}
+
+class B : public A {};
+
+#endif

diff  --git a/clang/test/CXX/module/module.unit/p7/t7.cpp 
b/clang/test/CXX/module/module.unit/p7/t7.cpp
new file mode 100644
index 0..7ce0cbb964131
--- /dev/null
+++ b/clang/test/CXX/module/module.unit/p7/t7.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ %s -verify
+// expected-no-diagnostics
+module;
+#include "h7.h"
+export module X;



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