[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG847466860887: [C++20] [Modules] Make the linkage consistent 
for template and its (authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

Files:
  clang/lib/AST/Decl.cpp
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -171,3 +171,26 @@
   selectFirst("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+export module m;
+export template 
+void f() {}
+
+template <>
+void f() {})");
+
+  auto AST =
+  tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext  = AST->getASTContext();
+
+  llvm::SmallVector Funcs =
+  match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_EQ(Funcs.size(), 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_EQ(TemplateF->getLinkageInternal(),
+SpecializedF->getLinkageInternal());
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), 
computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -171,3 +171,26 @@
   selectFirst("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+export module m;
+export template 
+void f() {}
+
+template <>
+void f() {})");
+
+  auto AST =
+  tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext  = AST->getASTContext();
+
+  llvm::SmallVector Funcs =
+  match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_EQ(Funcs.size(), 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_EQ(TemplateF->getLinkageInternal(),
+SpecializedF->getLinkageInternal());
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D120397#3401380 , @ChuanqiXu wrote:

> In D120397#3399808 , @aaron.ballman 
> wrote:
>
>> Can you also add a release note that explains we've fixed the crash?
>
> My thought is to edit a release note standalone. Since there are already many 
> works in modules now. I guess it might be better to try to give a summarize.

SGTM!

In D120397#3402081 , @iains wrote:

> for the record, I experimented with adding the linkage as an output for AST 
> dumps (adding to TextNodeDumper / DeclPrinter), since it seems that this 
> could be generally useful.
> this works fine, but, it would create a lot of testsuite churn - around 350 
> tests would need amendment, so I've punted on it for now (perhaps unit tests 
> can always accomplish the same).

Yeah, our AST dumping tests are very fragile to changes to the AST dump, 
unfortunately. It's very reasonable to punt on that, but thank you for looking 
into it!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

for the record, I experimented with adding the linkage as an output for AST 
dumps (adding to TextNodeDumper / DeclPrinter), since it seems that this could 
be generally useful.
this works fine, but, it would create a lot of testsuite churn - around 350 
tests would need amendment, so I've punted on it for now (perhaps unit tests 
can always accomplish the same).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

In D120397#3399808 , @aaron.ballman 
wrote:

> Can you also add a release note that explains we've fixed the crash?

My thought is to edit a release note standalone. Since there are already many 
works in modules now. I guess it might be better to try to give a summarize.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 417479.
ChuanqiXu added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

Files:
  clang/lib/AST/Decl.cpp
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -171,3 +171,26 @@
   selectFirst("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+export module m;
+export template 
+void f() {}
+
+template <>
+void f() {})");
+
+  auto AST =
+  tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext  = AST->getASTContext();
+
+  llvm::SmallVector Funcs =
+  match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_EQ(Funcs.size(), 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_EQ(TemplateF->getLinkageInternal(),
+SpecializedF->getLinkageInternal());
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), 
computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -171,3 +171,26 @@
   selectFirst("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+export module m;
+export template 
+void f() {}
+
+template <>
+void f() {})");
+
+  auto AST =
+  tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext  = AST->getASTContext();
+
+  llvm::SmallVector Funcs =
+  match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_EQ(Funcs.size(), 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_EQ(TemplateF->getLinkageInternal(),
+SpecializedF->getLinkageInternal());
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGenCXX/inconsistent-export-template.cpp:1-11
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple 
-disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+

Is this test worth keeping now that we've got the unit test? It's not testing 
the linkage, by the looks of it & @aaron.ballman made a fair point that this 
code change doesn't touch codegen, so the AST unit test is probably more 
suitable.



Comment at: clang/unittests/AST/DeclTest.cpp:194-195
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_TRUE(TemplateF->getLinkageInternal() ==
+  SpecializedF->getLinkageInternal());
+}

Maybe use EXPECT_EQ to get better error messages if this fails (that way the 
error message can include the values - whereas written with TRUE+== the error 
message can only include "false") - similarly with the size() == 2, prefer 
EXPECT_EQ(size(), 2)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D120397#3398562 , @ChuanqiXu wrote:

> Both `ast-dump` and `ast-dump=json` couldn't solve this. And I feel 
> `static_assert` is hard to implement. But from your wording, I feel a 
> unittest could match the intention. @dblaikie @aaron.ballman I thought the 
> key point here is to test the linkage actually. And I feel the unittest is 
> really good for this.

I like that solution, great idea!

The changes LGTM. Can you also add a release note that explains we've fixed the 
crash?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Both `ast-dump` and `ast-dump=json` couldn't solve this. And I feel 
`static_assert` is hard to implement. But from your wording, I feel a unittest 
could match the intention. @dblaikie @aaron.ballman I thought the key point 
here is to test the linkage actually. And I feel the unittest is really good 
for this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 417175.
ChuanqiXu added a comment.

Use Unittest instead of `expected-no-diagnostic` tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

Files:
  clang/lib/AST/Decl.cpp
  clang/test/CodeGenCXX/inconsistent-export-template.cpp
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -171,3 +171,26 @@
   selectFirst("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+export module m;
+export template 
+void f() {}
+
+template <>
+void f() {})");
+
+  auto AST =
+  tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext  = AST->getASTContext();
+
+  llvm::SmallVector Funcs =
+  match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_TRUE(Funcs.size() == 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_TRUE(TemplateF->getLinkageInternal() ==
+  SpecializedF->getLinkageInternal());
+}
Index: clang/test/CodeGenCXX/inconsistent-export-template.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inconsistent-export-template.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple 
-disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+
+// CHECK: void @_Z1fIiEvv
+template <>
+void f() {
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), 
computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -171,3 +171,26 @@
   selectFirst("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+export module m;
+export template 
+void f() {}
+
+template <>
+void f() {})");
+
+  auto AST =
+  tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext  = AST->getASTContext();
+
+  llvm::SmallVector Funcs =
+  match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_TRUE(Funcs.size() == 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs("f");
+  EXPECT_TRUE(TemplateF->getLinkageInternal() ==
+  SpecializedF->getLinkageInternal());
+}
Index: clang/test/CodeGenCXX/inconsistent-export-template.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inconsistent-export-template.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple -disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+
+// CHECK: void @_Z1fIiEvv
+template <>
+void f() {
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the 

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: clang-language-wg.
aaron.ballman added a comment.

Oops, accidentally dropped the language wg somehow, adding them back.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman removed a reviewer: clang-language-wg.
aaron.ballman added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;

dblaikie wrote:
> ChuanqiXu wrote:
> > urnathan wrote:
> > > dblaikie wrote:
> > > > This test doesn't appear to test anything - it verifies that this file 
> > > > produces no diagnostics, but not that it has any other/particular 
> > > > behavior.
> > > > 
> > > > Should this be testing codegen to verify that the correct linkage was 
> > > > used on the resulting IR functions?
> > > > 
> > > > Are there other ways of observing the particular language-level linkage 
> > > > of the entities to confirm it's as expected?
> > > yes, this should be a codegen test 
> > > (clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?
> > The compiler would crash at the test before the patch landed. So I send the 
> > patch here to show that the test wouldn't cause the compiler crash.
> My concern is that a "does anything other than crash" is a fairly vague 
> requirement - the existence of a crash points to an unverified codepath, but 
> "doesn't crash" isn't the test that was missing - the test that was missing 
> was for the functionality we'd expect after the crash (that functionality 
> couldn't be/wasn't previously tested, due to the presence of the crash).
> 
> @aaron.ballman perhaps you've got some thoughts/quick opinion here - given 
> the patch, what would you find to be suitable testing (perhaps I'm off-base 
> here and a "no diagnostic" test is suitable, given how various constraints 
> are enforced at AST building time - perhaps the absence of any violation of 
> those constraints adequately tests this patch?) (& if you've got thoughts on 
> the issue of adding test coverage for issues not fixed yet, as discussed 
> below, open to those too)
I think that's a valid concern. We do have more than a handful of tests which 
are literally "does clang no longer crash" tests, but those tend to be older 
tests and a bit lower quality over time compared to tests that validate the 
behavior beyond just crashing. e.g. if the crash is a diagnostic-related crash, 
we verify the diagnostics are correctly emitted (or not longer incorrectly 
emitted); if it's related to codegen, we verify that the codegen is valid, etc.

I don't think this should have a CodeGen based on my assumption that this test 
skips codegen but used to crash. So there was a frontend crash somewhere. Given 
that the changes relate to setting the linkage for a declaration, I wonder if 
we can use `static_assert` along with some type traits to determine whether the 
linkage is set properly purely within the frontend? Alternatively, is the 
information reflected in an `-ast-dump` (or `-ast-dump=json`) dump so that we 
could test the linkage that way?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;

ChuanqiXu wrote:
> urnathan wrote:
> > dblaikie wrote:
> > > This test doesn't appear to test anything - it verifies that this file 
> > > produces no diagnostics, but not that it has any other/particular 
> > > behavior.
> > > 
> > > Should this be testing codegen to verify that the correct linkage was 
> > > used on the resulting IR functions?
> > > 
> > > Are there other ways of observing the particular language-level linkage 
> > > of the entities to confirm it's as expected?
> > yes, this should be a codegen test 
> > (clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?
> The compiler would crash at the test before the patch landed. So I send the 
> patch here to show that the test wouldn't cause the compiler crash.
My concern is that a "does anything other than crash" is a fairly vague 
requirement - the existence of a crash points to an unverified codepath, but 
"doesn't crash" isn't the test that was missing - the test that was missing was 
for the functionality we'd expect after the crash (that functionality couldn't 
be/wasn't previously tested, due to the presence of the crash).

@aaron.ballman perhaps you've got some thoughts/quick opinion here - given the 
patch, what would you find to be suitable testing (perhaps I'm off-base here 
and a "no diagnostic" test is suitable, given how various constraints are 
enforced at AST building time - perhaps the absence of any violation of those 
constraints adequately tests this patch?) (& if you've got thoughts on the 
issue of adding test coverage for issues not fixed yet, as discussed below, 
open to those too)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a subscriber: urnathan.
ChuanqiXu added a comment.

Oh, @urnathan resigned from all my patches and I don't know why. @iains 
@dblaikie could you help to review this one?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

dblaikie wrote:
> urnathan wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > iains wrote:
> > > > > ChuanqiXu wrote:
> > > > > > urnathan wrote:
> > > > > > > I don't think we should be testing for ill-formed code here.  We 
> > > > > > > want to verify that explicit instantiations, explicit 
> > > > > > > specializations and implicit instantiations all get the expected 
> > > > > > > linkage -- both external linkage on exported entities, module 
> > > > > > > linkage on non-exported module-purview entities.
> > > > > > I think it could add an `expected-error` once we complete the check 
> > > > > > in compiler so I added the FIXME here.
> > > > > would it be possible to find a suitable place in the source for the 
> > > > > FIXME?
> > > > > I would be concerned that it could get forgotten in the test-case.
> > > > or maybe just file a PR for accepts invalid code?
> > > I would like to send an issue after the patch to remind me not forgetting 
> > > it. The issue is needed after the patch since it would crash too. I 
> > > prefer to remain the FIXME here so that it would look like a pre-commit 
> > > test case, which should be good.
> > The way to not forget about this is to file a bug and assign it to 
> > yourself.  Not add a known-wrong testcase.
> FWIW, I do sometimes put in known-bad test cases to document existing issues 
> and also to make it easier to find existing test coverage/a good home for the 
> test case (often I/we end up adding a new test file, because it's not obvious 
> where related existing test coverage is).
> 
> If this is the right place for the future test case, and it documents a 
> limitation with the existing behavior, I wouldn't be totally against 
> including it here, with a FIXME and reference to a filed bug, ideally (though 
> I think I've done this without a bug filed too).
Agree with @dblaikie here, I filed an issue 
(https://github.com/llvm/llvm-project/issues/54189) assigned to myself and a 
reference it. @urnathan , a key reason why I don't move the known-wrong test 
case is that this case would crash now. And I think it is good to test it 
wouldn't crash. Crash shouldn't be good all the time.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 412886.
ChuanqiXu added a comment.

Address comments:

- File an issue and add a reference to it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

Files:
  clang/lib/AST/Decl.cpp
  clang/test/CodeGenCXX/inconsistent-export-template.cpp
  clang/test/Modules/inconsist-export-template.cpp


Index: clang/test/Modules/inconsist-export-template.cpp
===
--- /dev/null
+++ clang/test/Modules/inconsist-export-template.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
+export template 
+void f() {
+}
+
+template <>
+void f() {
+}
+
+template 
+void f1() {
+}
+
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+// See https://llvm.org/PR54189 for details.
+export template <>
+void f1() {
+}
+
+export template 
+class C {
+};
+
+template <>
+class C {
+public:
+  void M(){};
+};
+
+void Use(C ) { p.M(); }
Index: clang/test/CodeGenCXX/inconsistent-export-template.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inconsistent-export-template.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple 
-disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+
+// CHECK: void @_Z1fIiEvv
+template <>
+void f() {
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), 
computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;


Index: clang/test/Modules/inconsist-export-template.cpp
===
--- /dev/null
+++ clang/test/Modules/inconsist-export-template.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
+export template 
+void f() {
+}
+
+template <>
+void f() {
+}
+
+template 
+void f1() {
+}
+
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+// See https://llvm.org/PR54189 for details.
+export template <>
+void f1() {
+}
+
+export template 
+class C {
+};
+
+template <>
+class C {
+public:
+  void M(){};
+};
+
+void Use(C ) { p.M(); }
Index: clang/test/CodeGenCXX/inconsistent-export-template.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inconsistent-export-template.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple -disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+
+// CHECK: void @_Z1fIiEvv
+template <>
+void f() {
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

urnathan wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > iains wrote:
> > > > ChuanqiXu wrote:
> > > > > urnathan wrote:
> > > > > > I don't think we should be testing for ill-formed code here.  We 
> > > > > > want to verify that explicit instantiations, explicit 
> > > > > > specializations and implicit instantiations all get the expected 
> > > > > > linkage -- both external linkage on exported entities, module 
> > > > > > linkage on non-exported module-purview entities.
> > > > > I think it could add an `expected-error` once we complete the check 
> > > > > in compiler so I added the FIXME here.
> > > > would it be possible to find a suitable place in the source for the 
> > > > FIXME?
> > > > I would be concerned that it could get forgotten in the test-case.
> > > or maybe just file a PR for accepts invalid code?
> > I would like to send an issue after the patch to remind me not forgetting 
> > it. The issue is needed after the patch since it would crash too. I prefer 
> > to remain the FIXME here so that it would look like a pre-commit test case, 
> > which should be good.
> The way to not forget about this is to file a bug and assign it to yourself.  
> Not add a known-wrong testcase.
FWIW, I do sometimes put in known-bad test cases to document existing issues 
and also to make it easier to find existing test coverage/a good home for the 
test case (often I/we end up adding a new test file, because it's not obvious 
where related existing test coverage is).

If this is the right place for the future test case, and it documents a 
limitation with the existing behavior, I wouldn't be totally against including 
it here, with a FIXME and reference to a filed bug, ideally (though I think 
I've done this without a bug filed too).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added inline comments.
Herald added a project: All.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

ChuanqiXu wrote:
> iains wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > urnathan wrote:
> > > > > I don't think we should be testing for ill-formed code here.  We want 
> > > > > to verify that explicit instantiations, explicit specializations and 
> > > > > implicit instantiations all get the expected linkage -- both external 
> > > > > linkage on exported entities, module linkage on non-exported 
> > > > > module-purview entities.
> > > > I think it could add an `expected-error` once we complete the check in 
> > > > compiler so I added the FIXME here.
> > > would it be possible to find a suitable place in the source for the FIXME?
> > > I would be concerned that it could get forgotten in the test-case.
> > or maybe just file a PR for accepts invalid code?
> I would like to send an issue after the patch to remind me not forgetting it. 
> The issue is needed after the patch since it would crash too. I prefer to 
> remain the FIXME here so that it would look like a pre-commit test case, 
> which should be good.
The way to not forget about this is to file a bug and assign it to yourself.  
Not add a known-wrong testcase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

iains wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > urnathan wrote:
> > > > I don't think we should be testing for ill-formed code here.  We want 
> > > > to verify that explicit instantiations, explicit specializations and 
> > > > implicit instantiations all get the expected linkage -- both external 
> > > > linkage on exported entities, module linkage on non-exported 
> > > > module-purview entities.
> > > I think it could add an `expected-error` once we complete the check in 
> > > compiler so I added the FIXME here.
> > would it be possible to find a suitable place in the source for the FIXME?
> > I would be concerned that it could get forgotten in the test-case.
> or maybe just file a PR for accepts invalid code?
I would like to send an issue after the patch to remind me not forgetting it. 
The issue is needed after the patch since it would crash too. I prefer to 
remain the FIXME here so that it would look like a pre-commit test case, which 
should be good.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Iain Sandoe via Phabricator via cfe-commits]

iains added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

iains wrote:
> ChuanqiXu wrote:
> > urnathan wrote:
> > > I don't think we should be testing for ill-formed code here.  We want to 
> > > verify that explicit instantiations, explicit specializations and 
> > > implicit instantiations all get the expected linkage -- both external 
> > > linkage on exported entities, module linkage on non-exported 
> > > module-purview entities.
> > I think it could add an `expected-error` once we complete the check in 
> > compiler so I added the FIXME here.
> would it be possible to find a suitable place in the source for the FIXME?
> I would be concerned that it could get forgotten in the test-case.
or maybe just file a PR for accepts invalid code?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Iain Sandoe via Phabricator via cfe-commits]

iains added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

ChuanqiXu wrote:
> urnathan wrote:
> > I don't think we should be testing for ill-formed code here.  We want to 
> > verify that explicit instantiations, explicit specializations and implicit 
> > instantiations all get the expected linkage -- both external linkage on 
> > exported entities, module linkage on non-exported module-purview entities.
> I think it could add an `expected-error` once we complete the check in 
> compiler so I added the FIXME here.
would it be possible to find a suitable place in the source for the FIXME?
I would be concerned that it could get forgotten in the test-case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/CodeGenCXX/inconsistent-export-template.cpp:8
+
+// CHECK: void @_Z1fIiEvv
+template <>

The mangled name should contain module name if D118352 is ready.



Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;

urnathan wrote:
> dblaikie wrote:
> > This test doesn't appear to test anything - it verifies that this file 
> > produces no diagnostics, but not that it has any other/particular behavior.
> > 
> > Should this be testing codegen to verify that the correct linkage was used 
> > on the resulting IR functions?
> > 
> > Are there other ways of observing the particular language-level linkage of 
> > the entities to confirm it's as expected?
> yes, this should be a codegen test 
> (clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?
The compiler would crash at the test before the patch landed. So I send the 
patch here to show that the test wouldn't cause the compiler crash.



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

urnathan wrote:
> I don't think we should be testing for ill-formed code here.  We want to 
> verify that explicit instantiations, explicit specializations and implicit 
> instantiations all get the expected linkage -- both external linkage on 
> exported entities, module linkage on non-exported module-purview entities.
I think it could add an `expected-error` once we complete the check in compiler 
so I added the FIXME here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 411996.
ChuanqiXu added a comment.

Add test in CodeGen.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

Files:
  clang/lib/AST/Decl.cpp
  clang/test/CodeGenCXX/inconsistent-export-template.cpp
  clang/test/Modules/inconsist-export-template.cpp


Index: clang/test/Modules/inconsist-export-template.cpp
===
--- /dev/null
+++ clang/test/Modules/inconsist-export-template.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
+export template 
+void f() {
+}
+
+template <>
+void f() {
+}
+
+template 
+void f1() {
+}
+
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+}
+
+export template 
+class C {
+};
+
+template <>
+class C {
+public:
+  void M(){};
+};
+
+void Use(C ) { p.M(); }
Index: clang/test/CodeGenCXX/inconsistent-export-template.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inconsistent-export-template.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple 
-disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+
+// CHECK: void @_Z1fIiEvv
+template <>
+void f() {
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), 
computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;


Index: clang/test/Modules/inconsist-export-template.cpp
===
--- /dev/null
+++ clang/test/Modules/inconsist-export-template.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
+export template 
+void f() {
+}
+
+template <>
+void f() {
+}
+
+template 
+void f1() {
+}
+
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+}
+
+export template 
+class C {
+};
+
+template <>
+class C {
+public:
+  void M(){};
+};
+
+void Use(C ) { p.M(); }
Index: clang/test/CodeGenCXX/inconsistent-export-template.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inconsistent-export-template.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple -disable-llvm-passes -o - | FileCheck %s
+
+export module m;
+export template 
+void f() {
+}
+
+// CHECK: void @_Z1fIiEvv
+template <>
+void f() {
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
-getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
+  getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:26-34
+export template
+class C {
+
+};
+
+template<>
+class C {

I think you'll need a member fn to have something to check the linkage of?  
Might be worth checking instantiating a partial specialization gets things 
right too?  along the lines of ..

export template class C {};
template class C { void M(){}; };

void Use (C ) { p.M(); }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

In D120397#3340256 , @erichkeane 
wrote:

> Linkage stuff is where I get lost quickly, hopefully @urnathan can comment 
> here.  Codewise I think this looks right.

looks right to me too -- take the linkage from the most general template.




Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;

dblaikie wrote:
> This test doesn't appear to test anything - it verifies that this file 
> produces no diagnostics, but not that it has any other/particular behavior.
> 
> Should this be testing codegen to verify that the correct linkage was used on 
> the resulting IR functions?
> 
> Are there other ways of observing the particular language-level linkage of 
> the entities to confirm it's as expected?
yes, this should be a codegen test 
(clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?



Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+

I don't think we should be testing for ill-formed code here.  We want to verify 
that explicit instantiations, explicit specializations and implicit 
instantiations all get the expected linkage -- both external linkage on 
exported entities, module linkage on non-exported module-purview entities.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;

This test doesn't appear to test anything - it verifies that this file produces 
no diagnostics, but not that it has any other/particular behavior.

Should this be testing codegen to verify that the correct linkage was used on 
the resulting IR functions?

Are there other ways of observing the particular language-level linkage of the 
entities to confirm it's as expected?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Linkage stuff is where I get lost quickly, hopefully @urnathan can comment 
here.  Codewise I think this looks right.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397

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

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: urnathan, iains, rsmith, aaron.ballman, erichkeane.
ChuanqiXu added a project: clang.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.

Before the patch, the compiler would crash for the test due to inconsistent 
linkage.

This patch tries to avoid it by make the linkage consistent for template and 
its specialization. After the patch, the behavior of compiler would be 
partially correct for the case.
The correct one is:

  export template
  void f() {}
  
  template<>
  void f() {}

In this case, the linkage for both declaration should be external (the wording 
I get by consulting in WG21 is "the linkage for name `f` should be external").

And for the case:

  template
  void f() {}
  
  export template<>
  void f() {}

Compiler should reject it. This isn't done now. I marked it as FIXME in the 
test. After all, this patch would stop a crash.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120397

Files:
  clang/lib/AST/Decl.cpp
  clang/test/Modules/inconsist-export-template.cpp


Index: clang/test/Modules/inconsist-export-template.cpp
===
--- /dev/null
+++ clang/test/Modules/inconsist-export-template.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
+export template 
+void f() {
+
+}
+
+template <>
+void f() {
+
+}
+
+template 
+void f1() {
+
+}
+
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+
+}
+
+export template
+class C {
+
+};
+
+template<>
+class C {
+
+};
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
 getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;


Index: clang/test/Modules/inconsist-export-template.cpp
===
--- /dev/null
+++ clang/test/Modules/inconsist-export-template.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
+export template 
+void f() {
+
+}
+
+template <>
+void f() {
+
+}
+
+template 
+void f1() {
+
+}
+
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1() {
+
+}
+
+export template
+class C {
+
+};
+
+template<>
+class C {
+
+};
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -391,11 +391,18 @@
   bool considerVisibility =
 shouldConsiderTemplateVisibility(fn, specInfo);
 
-  // Merge information from the template parameters.
   FunctionTemplateDecl *temp = specInfo->getTemplate();
-  LinkageInfo tempLV =
+
+  // Merge information from the template declaration.
+  LinkageInfo tempLV = getLVForDecl(temp, computation);
+  // The linkage of the specialization should be consistent with the
+  // template declaration.
+  LV.setLinkage(tempLV.getLinkage());
+
+  // Merge information from the template parameters.
+  LinkageInfo paramsLV =
 getLVForTemplateParameterList(temp->getTemplateParameters(), computation);
-  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+  LV.mergeMaybeWithVisibility(paramsLV, considerVisibility);
 
   // Merge information from the template arguments.
   const TemplateArgumentList  = *specInfo->TemplateArguments;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits