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

[ChuanqiXu]

Summary

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<class T>
void f() {}

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

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<class T>
void f() {}

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

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.

[erichkeane]

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

[dblaikie]

clang/test/Modules/inconsist-export-template.cpp

1–2 ↗

(On Diff #410743)

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?

[urnathan]

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.

clang/test/Modules/inconsist-export-template.cpp

1–2 ↗

(On Diff #410743)

yes, this should be a codegen test (clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?

19–23 ↗

(On Diff #410743)

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.

[urnathan]

clang/test/Modules/inconsist-export-template.cpp

26–34 ↗

(On Diff #410743)

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<typename> class C {};
template<typename T> class C<T *> { void M(){}; };

void Use (C<int *> &p) { p.M(); }

[ChuanqiXu]

Add test in CodeGen.

[ChuanqiXu]

clang/test/CodeGenCXX/inconsistent-export-template.cpp

8 ↗

(On Diff #411996)

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

clang/test/Modules/inconsist-export-template.cpp

1–2 ↗

(On Diff #410743)

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.

19–23 ↗

(On Diff #410743)

I think it could add an expected-error once we complete the check in compiler so I added the FIXME here.

[iains]

clang/test/Modules/inconsist-export-template.cpp

19–23 ↗

(On Diff #410743)

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.

[iains]

clang/test/Modules/inconsist-export-template.cpp

19–23 ↗

(On Diff #410743)

or maybe just file a PR for accepts invalid code?

[ChuanqiXu]

clang/test/Modules/inconsist-export-template.cpp

19–23 ↗

(On Diff #410743)

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.

[urnathan]

clang/test/Modules/inconsist-export-template.cpp

19–23 ↗

(On Diff #410743)

The way to not forget about this is to file a bug and assign it to yourself. Not add a known-wrong testcase.

[dblaikie]

clang/test/Modules/inconsist-export-template.cpp

19–23 ↗

(On Diff #410743)

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

[ChuanqiXu]

Address comments:

• File an issue and add a reference to it.

[ChuanqiXu]

clang/test/Modules/inconsist-export-template.cpp

19–23 ↗

(On Diff #410743)

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.

[ChuanqiXu]

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

[dblaikie]

clang/test/Modules/inconsist-export-template.cpp

1–2 ↗

(On Diff #410743)

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)

[aaron.ballman]

clang/test/Modules/inconsist-export-template.cpp

1–2 ↗

(On Diff #410743)

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?

[aaron.ballman]

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

[ChuanqiXu]

Use Unittest instead of expected-no-diagnostic tests.

[ChuanqiXu]

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.

[aaron.ballman]

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?

[dblaikie]

clang/test/CodeGenCXX/inconsistent-export-template.cpp

1–11 ↗

(On Diff #417175)

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.

clang/unittests/AST/DeclTest.cpp

194–195

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)

[ChuanqiXu]

Address comments.

[ChuanqiXu]

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.

[iains]

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

[aaron.ballman]

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!

clang/lib/AST/Decl.cpp

 /// \param[out] LV the computation to use for the parent
 void LinkageComputer::mergeTemplateLV(
     LinkageInfo &LV, const FunctionDecl *fn,
     const FunctionTemplateSpecializationInfo *specInfo,
     LVComputationKind computation) {
   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 &templateArgs = *specInfo->TemplateArguments;
   LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs, computation);
   LV.mergeMaybeWithVisibility(argsLV, considerVisibility);
 }
 
 /// Does the given declaration have a direct visibility attribute

clang/unittests/AST/DeclTest.cpp

TEST(Decl, IsInExportDeclContext) {
   auto AST =
       tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
   ASTContext &Ctx = AST->getASTContext();
 
   const auto *f =
       selectFirst<FunctionDecl>("f", match(functionDecl().bind("f"), Ctx));
   EXPECT_TRUE(f->isInExportDeclContext());
 }
+
+TEST(Decl, InConsistLinkageForTemplates) {
+  llvm::Annotations Code(R"(
+    export module m;
+    export template <class T>
+    void f() {}
+
+    template <>
+    void f<int>() {})");
+
+  auto AST =
+      tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext &Ctx = AST->getASTContext();
+
+  llvm::SmallVector<ast_matchers::BoundNodes, 2> Funcs =
+      match(functionDecl().bind("f"), Ctx);
+
+  EXPECT_EQ(Funcs.size(), 2);
+  const FunctionDecl *TemplateF = Funcs[0].getNodeAs<FunctionDecl>("f");
+  const FunctionDecl *SpecializedF = Funcs[1].getNodeAs<FunctionDecl>("f");
+  EXPECT_EQ(TemplateF->getLinkageInternal(),
+            SpecializedF->getLinkageInternal());
+}