[clang][ABI] New c++20 modules mangling scheme

[urnathan]

Summary

The existing module symbol mangling scheme turns out to be undemangleable. It is also desirable to switch to the strong-ownership model as the hoped-for C++17 compatibility turns out to be fragile, and we also now have a better way of controlling that.

The issue is captured on the ABI list at: https://github.com/itanium-cxx-abi/cxx-abi/issues/134

A document describing the issues and new mangling is at: https://drive.google.com/file/d/1quPYyByXqOpCyAPlxYt1JleA70TSFMrp/view?usp=sharing

This patch is the code-generation part. I have a demangler too, but that patch is based on some to-be-landed refactoring of the demangler.

The old mangling is unceremoniously dropped. No backwards compatibility, no deprectated old-mangling flag. It was always labelled experimental. (Old and new manglings cannot be confused.)

FWIW I'm fine with waiting until (a) demangler is ready and (b) clang-14 is out (just in case there's more changes needed)

[ChuanqiXu]

clang/lib/AST/ItaniumMangle.cpp

1052–1058

I looked at the 2.1.1 section at https://drive.google.com/file/d/1qQjqptzOFT_lfXH8L6-iD9nCRi34wjft/view. You mentioned it in the summary of the patch.

1081

Yeah, it is fixed after applying D119550. I would like to give a try to give a reduced test. But I've tried it last week and I am relatively busy. I couldn't promise I could give it finally.

clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp

15

I just feel like we lost a test about function name in implementation unit.

[urnathan]

clang/lib/AST/ItaniumMangle.cpp

1052–1058

2.1.1 Substitutions
A <module-name> is substitutable, using the existing <substitution> encoding:
<substitution> ::= S _
::= S <seq-id> _

1081

that is good to know. totally understand if it's too hard to extract a testcase.

[urnathan]

Added implementation unit test

[urnathan]

clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp

15

ah, it's mmore like there never was one to lose. I've added a codegen test.

[ChuanqiXu]

clang/lib/AST/ItaniumMangle.cpp

1052–1058

Oh, sorry. I referred to 2.4.1:

<module-subst> ::= W _ <digit> # substitutions 0-9
::= W W <seq-id> _ # (n+10)th substitution

I mean the LHS of the comment. Its name should be <substitution> instead of <module-subst>, right?

[urnathan]

clang/lib/AST/ItaniumMangle.cpp

1052–1058

Section 2.4 discusses alternatives that were considered and rejected (it is very useful to have this kind of information so that future me knows we already thought about it).

by LHS of comment do you mean the BNF? This alternative is not a <substitution> element and would have a distinct name.

[urnathan]

Update after excise of IgnoreLinkageSpecDecls

[urnathan]

clang/lib/AST/ItaniumMangle.cpp

1052–1058

oh I see it now.

[ChuanqiXu]

clang/lib/AST/ItaniumMangle.cpp

1071–1078

For the partition part, I think we shouldn't mangle partition name. We should mangle about the name of the primary module (module-name) only. Since module linkage refers to that the entity could be referred by name for other units in the same module. So I think we could ignore partition part of the module simply.

[urnathan]

Add module initializer mangling entry point and fix partition mangling elswhere.

[urnathan]

clang/lib/AST/ItaniumMangle.cpp

1071–1078

d'oh! right, there is one case where we do need to mangle the partition name (the initializer function), But mostly we should not.

[urnathan]

Looks like the partition stuff will need tweaking if/when D118586 lands.

[ChuanqiXu]

Looks like the partition stuff will need tweaking if/when D118586 lands.

I think it is worth to add D118586 as a related revision.

clang/lib/AST/ItaniumMangle.cpp

1071–1078

So why do we need the 'P' marker. I think we should treat names uniformly in partition unit and primary module interface unit.

1413

Unused variable?

6068–6070

It looks it is guarded at the line 6071.

6088–6089

What's the rationale to ban the case? I think it worths a comment . It looks like we don't want to mangle std name not in GMF. It looks like we would ban the case:

export module m;
namespace std {
template <class T>
class xxx { ... };
}

Do we need to ban this? Or if someone is trying to implement STL in named module (I know it wouldn't happen in recent future), the pattern looks not right.

[urnathan]

In D118352#3328101, @ChuanqiXu wrote:

Looks like the partition stuff will need tweaking if/when D118586 lands.

I think it is worth to add D118586 as a related revision.

I don't see the need. While both deal with modules, neither is dependent on the other and we're not going to forget to address the partition fixmes here.

clang/lib/AST/ItaniumMangle.cpp

1071–1078

The global initializer function. Partitions end up needing to emit them and they must be both externally reachable and distinct. Fortunately only the primary module interface unit needs to know about them.

1413

No, it is used later in code I have not changed.

6068–6070

No. that other check is checking the ownership of the template, this is checking the ownership of one of its template parameters. I.e. in

std::basic_string<char, std::type_traits<char>, std::allocator<char>>

the other check is for std::basic_string, this check is for std::type_traits and std::allocator.

6088–6089

We're not banning that, it would mangle as _ZStW1m3xxx. the special manglings for types in the std namespace are for when they are attached to the global module.

[urnathan]

Reminder to add to clang/docs/ReleaseNotes.rst, once this and the demangler patches are in.

[urnathan]

rebase on top of D116773

[ChuanqiXu]

Thanks for explanation. Now it looks good to me. Let's accept it formally after the series of partition landed and so that we could add test about partitions.

[urnathan]

In D118352#3331396, @ChuanqiXu wrote:

Thanks for explanation. Now it looks good to me. Let's accept it formally after the series of partition landed and so that we could add test about partitions.

thanks,

We're going to have to adjust a bit when partitions land, and the global initializer is a separate set of patches only tangentially impacting this. I don't think the project benefits from waiting.

[ChuanqiXu]

@urnathan FWIW, D114714 is landed~

[urnathan]

Updated to new partitions API

[ChuanqiXu]

If I don't misread the codes, it looks like mangleModuleInitializer is not called.

---

Now we could add test for partitions.

[urnathan]

In D118352#3358864, @ChuanqiXu wrote:

If I don't misread the codes, it looks like mangleModuleInitializer is not called.

Now we could add test for partitions.

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

[ChuanqiXu]

In D118352#3359626, @urnathan wrote:

In D118352#3358864, @ChuanqiXu wrote:

If I don't misread the codes, it looks like mangleModuleInitializer is not called.

Now we could add test for partitions.

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

It's odd to see unused functions. I just worry about that other people might delete these functions as cleaning up (maybe there wouldn't be).

---

Although the general declarations in partitions wouldn't be mangled specially, I think it would be better to add test case to show that.

[urnathan]

added partition tests

[urnathan]

In D118352#3362694, @ChuanqiXu wrote:

In D118352#3359626, @urnathan wrote:

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

It's odd to see unused functions. I just worry about that other people might delete these functions as cleaning up (maybe there wouldn't be).

That's a risk I'm ok with.

Although the general declarations in partitions wouldn't be mangled specially, I think it would be better to add test case to show that.

good idea, added.

[iains]

In D118352#3363909, @urnathan wrote:

In D118352#3362694, @ChuanqiXu wrote:

In D118352#3359626, @urnathan wrote:

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

It's odd to see unused functions. I just worry about that other people might delete these functions as cleaning up (maybe there wouldn't be).

That's a risk I'm ok with.

I agree - I think we already a fair amount of infrastructure in the compiler to handle module initialisers, ahead of a full implementation - so I do not see a reason for waiting longer to apply this because we cannot yet exercise it full.

Although the general declarations in partitions wouldn't be mangled specially, I think it would be better to add test case to show that.

good idea, added.

[ChuanqiXu]

LGTM, then. BTW, I see there is already no dependency with D119833 in code. But it shows dependency still in phab.

[thakis]

Looks like this breaks tests on macOS: http://45.33.8.238/macm1/29650/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[thakis]

(also, please don't land unused code -- land those functions when you land their callers)

[phosek]

We're also seeing this issue on our Mac bots, is it possible to revert it?

[ChuanqiXu]

In D118352#3368919, @phosek wrote:

We're also seeing this issue on our Mac bots, is it possible to revert it?

I've reverted it. Since this is the new feature, I think it wouldn't so hurry to land it. BTW, I think it would be helpful to provide more information to fix it.

[steven_wu]

In D118352#3368922, @ChuanqiXu wrote:

In D118352#3368919, @phosek wrote:

We're also seeing this issue on our Mac bots, is it possible to revert it?

I've reverted it. Since this is the new feature, I think it wouldn't so hurry to land it. BTW, I think it would be helpful to provide more information to fix it.

It breaks the macOS bot for LLVM, you can click the test case for its output: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/27745/testReport/

You can see the error output. The problem might be that dso_local does not get codegen for macho targets.

[h-vetinari]

Something must have gone wrong... communication-wise... as @urnathan seems to have abandoned (resp. resigned from) all modules PRs. Hope any misunderstandings or grievances can be worked out!

[ChuanqiXu]

In D118352#3371425, @h-vetinari wrote:

Something must have gone wrong... communication-wise... as @urnathan seems to have abandoned (resp. resigned from) all modules PRs. Hope any misunderstandings or grievances can be worked out!

Oh, yeah... it is surprising...