Page MenuHomePhabricator

[c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects.
ClosedPublic

Authored by rsmith on Oct 22 2020, 6:12 PM.

Details

Summary

The Itanium side of this follows the approach I proposed in
https://github.com/itanium-cxx-abi/cxx-abi/issues/47 on 2020-09-06.

The MSVC side of this was determined empirically by observing MSVC's output. I've asked Jon Caves at Microsoft for clarifications around the FIXMEs.

Diff Detail

Event Timeline

rsmith created this revision.Oct 22 2020, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 6:12 PM
rsmith requested review of this revision.Oct 22 2020, 6:12 PM
rsmith updated this revision to Diff 300524.Sat, Oct 24, 10:24 PM
  • Fix incorrect mangling for zero-but-not-null template arguments.
rsmith updated this revision to Diff 301070.Tue, Oct 27, 11:43 AM
    • Ignore cv-qualifiers in member types for Itanium mangling.
    • Add MSVC-compatible mangling support.
    • Fix bug where template argument value mangling would be used when passing a template parameter object as a pointer or reference template argument.
  1. Updating D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling support

for non-type template parameters of class type and template parameter objects.

rsmith retitled this revision from [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling support for non-type template parameters of class type and template parameter objects. to [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects..Tue, Oct 27, 11:45 AM
rsmith edited the summary of this revision. (Show Details)
rsmith added a reviewer: rnk.
rsmith added inline comments.Tue, Oct 27, 11:53 AM
clang/lib/AST/MicrosoftMangle.cpp
657–660

For what it's worth, I'm fairly convinced the $ is not actually part of the mangling of the member function pointer, and is instead part of the mangling of a non-type template argument (or more generally of a value).

rjmccall added inline comments.Tue, Oct 27, 1:25 PM
clang/lib/AST/APValue.cpp
1047

I'm not sure what ABIs you're thinking about, but the *symbol* linkage/visibility of the type info isn't important, just its formal linkage/visibility.

1052

Objective-C object literals are interesting here; arguably, we could allow you to use one as a template argument, and it wouldn't naturally limit linkage.

clang/lib/AST/ASTContext.cpp
2502

What should we do on targets that allow virtual bases in member pointer conversions?

clang/lib/AST/ItaniumMangle.cpp
4946

Probably worth reemphasizing the general principle that this is not equivalent to asking whether it has a zero bit-pattern.

5154

Surely we have this as a subroutine already, or could.

5158

Can we extract a function to do this mangling that's called from both here and FixedPointLiteral, and then have that assert, rather than leaving a bomb for a corner case of a corner case?

5177

These can use the float-mangling subroutine that you should extract.

The condition here needs to be !isPosZero(), I think.

5203

This could also be extracted and shared with the template-argument mangler. In fact, isn't most of this case redundant with the template-argument mangler?

rnk added inline comments.Wed, Oct 28, 11:18 AM
clang/lib/AST/ASTContext.cpp
2502

It looks like the MS ABI codepaths don't use this routine. If we could put it somewhere Itanium-specific that would be accessible to the mangler and CGCXXABI implementation, that would be ideal, but ASTContext doesn't seem too bad.

clang/lib/AST/MicrosoftMangle.cpp
657–660

That makes sense to me. I'll see if I can simplify in a follow-up.

751–752

IIUC, this means we can mangle __int128 non-type template parms now? Let's add a test for that.

1746

Why is this even an APValue kind... O_o

clang/test/CodeGenCXX/mangle-class-nttp.cpp
28

These FIXMEs represent a lot of future work. :(

Thanks for adding test coverage on both sides, though.

rsmith updated this revision to Diff 303609.Fri, Nov 6, 7:57 PM
rsmith marked 10 inline comments as done.
  • Address review comments.
rsmith marked an inline comment as done.Fri, Nov 6, 7:58 PM
rsmith added inline comments.
clang/lib/AST/APValue.cpp
1052

That makes sense to me; FIXME added.

clang/lib/AST/ASTContext.cpp
2502

While we do support such things in general, we do not support constant-evaluation of a conversion between pointer-to-derived and pointer-to-virtual-base, and there is no APValue representation for such a thing. Because we take an APValue as input, this function should work correctly even under the MS ABI.

clang/lib/AST/ItaniumMangle.cpp
5158

Done. They're somewhat different cases because the other case is actually reachable (via __attribute__((overloadable))) but I think it makes sense to assume that we'll eventually support fixed-point in C++ mode. (I think it's a bit strange we didn't support it from the start.)

5203

Theoretically yes, but while there's a lot of overlap between TemplateArguments and APValues, they're different representations and can represent a somewhat different set of values.

I think we could convert all non-type TemplateArguments to APValues before mangling them, and that might reduce duplication a little, if you'd like?

clang/lib/AST/MicrosoftMangle.cpp
1746

I think the idea is that we want to be able to include these in function-local static dispatch arrays with constant initialization. But yeah, this is by far the weirdest kind of APValue.

clang/test/CodeGenCXX/mangle-class-nttp.cpp
28

I've asked Jon Caves about this, maybe we'll just be told what the rule is =)

rsmith updated this revision to Diff 303611.Fri, Nov 6, 8:20 PM
rsmith marked an inline comment as done.
  • Factor out duplicated mangling of null pointers.
rsmith added inline comments.Fri, Nov 6, 8:21 PM
clang/lib/AST/ItaniumMangle.cpp
5203

I've factored out the duplicated mangling of null pointers. I'm not sure if there's much more that's worth pulling out.

rjmccall accepted this revision.Fri, Nov 6, 11:53 PM

LGTM

clang/lib/AST/ItaniumMangle.cpp
5158

Agreed.

This revision is now accepted and ready to land.Fri, Nov 6, 11:53 PM
This revision was landed with ongoing or failed builds.Mon, Nov 9, 10:29 PM
This revision was automatically updated to reflect the committed changes.