Page MenuHomePhabricator

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

Authored by urnathan on Jan 27 2022, 4:58 AM.

Details

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)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ChuanqiXu added inline comments.Feb 13 2022, 6:34 PM
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 marked an inline comment as done.Feb 14 2022, 4:59 AM
urnathan added inline comments.
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 updated this revision to Diff 408402.Feb 14 2022, 6:49 AM
urnathan marked an inline comment as done.

Added implementation unit test

urnathan marked an inline comment as done.Feb 14 2022, 6:50 AM
urnathan added inline comments.
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 added inline comments.Feb 14 2022, 5:59 PM
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 marked an inline comment as done.Feb 15 2022, 3:43 AM
urnathan added inline comments.
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 updated this revision to Diff 408823.Feb 15 2022, 5:03 AM

Update after excise of IgnoreLinkageSpecDecls

urnathan updated this revision to Diff 408895.Feb 15 2022, 8:44 AM
urnathan marked an inline comment as done.
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1052–1058

oh I see it now.

ChuanqiXu added inline comments.Feb 15 2022, 10:44 PM
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 updated this revision to Diff 409230.Feb 16 2022, 6:30 AM

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

urnathan marked an inline comment as done.Feb 16 2022, 6:30 AM
urnathan added inline comments.
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 marked an inline comment as done.Feb 16 2022, 6:48 AM

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

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 marked 3 inline comments as done.Feb 17 2022, 4:36 AM

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 edited the summary of this revision. (Show Details)Feb 17 2022, 5:54 AM
urnathan marked 3 inline comments as done.Feb 17 2022, 7:36 AM

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

urnathan updated this revision to Diff 409761.Feb 17 2022, 1:02 PM
urnathan added a subscriber: cfe-commits.

rebase on top of D116773

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

urnathan updated this revision to Diff 412669.Mar 3 2022, 4:11 AM

Updated to new partitions API

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 4:11 AM

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


Now we could add test for partitions.

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.

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 updated this revision to Diff 413472.Mar 7 2022, 7:31 AM

added partition tests

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 added a comment.Mar 7 2022, 8:12 AM

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 accepted this revision.Mar 8 2022, 4:12 AM

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

This revision is now accepted and ready to land.Mar 8 2022, 4:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 6:22 AM
thakis added a subscriber: thakis.Mar 8 2022, 7:01 AM

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 added a comment.Mar 8 2022, 7:02 AM

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

phosek added a subscriber: phosek.Mar 8 2022, 6:05 PM

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

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.

ChuanqiXu reopened this revision.Mar 8 2022, 6:18 PM
This revision is now accepted and ready to land.Mar 8 2022, 6:18 PM

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.

urnathan abandoned this revision.Mar 9 2022, 10:52 AM

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!

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