[c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[bolshakov-a]

Summary

Previously committed as 9e08e51a20d0d2b1c5724bb17e969d036fced4cd, and
reverted because a dependency commit was reverted, then commited again
as 4b574008aef5a7235c1f894ab065fe300d26e786 and reverted again because
"dependency commit" 5a391d38ac6c561ba908334d427f26124ed9132e was
reverted. But it doesn't seem that 5a391d38ac6c was a real dependency
for this.

This commit incorporates 4b574008aef5a7235c1f894ab065fe300d26e786 and
18e093faf726d15f210ab4917142beec51848258 by Richard Smith, with some
minor fixes, most notably:

• VK_PRValue instead of VK_RValue as default kind in lvalue and

member pointer handling branch
in BuildExpressionFromNonTypeTemplateArgumentValue;

• handling of UncommonValue in IsTypeDeclaredInsideVisitor;

• filling in SugaredConverted along with CanonicalConverted

parameter in Sema::CheckTemplateArgument;

• minor cleanup

in TemplateInstantiator::transformNonTypeTemplateParmRef;

• noundef attribute and opaque pointers in template-arguments test;

• analysis for C++17 mode is turned off for templates

in warn-bool-conversion test;
in C++17 and C++20 mode, array reference used as a template argument
of pointer type produces template argument of UncommonValue type, and
BuildExpressionFromNonTypeTemplateArgumentValue makes
OpaqueValueExpr for it, and DiagnoseAlwaysNonNullPointer cannot see
through it; despite of "These cases should not warn" comment, I'm not
sure about correct behavior; I'd expect a suggestion to replace if by
if constexpr;

• temp.arg.nontype/p1.cpp test fixed.

[bolshakov-a]

It works, thanks!

[erichkeane]

clang/include/clang/AST/TemplateBase.h

88

I don't think splitting out the individual cases has all that much value, at least until we NEED it.

As far as a name, what about StructuralValue? P1907 calls the 'type' of that A 'structural type'? It isn't perfect, but it at least seems to be somewhat defensible with standards language.

clang/lib/AST/ItaniumMangle.cpp

4594

This definitely needs to happen. @rjmccall or @eli.friedman ^^ Any idea what the actual mangling should be?

clang/lib/AST/MicrosoftMangle.cpp

1734

We need to end up doing our best to match the microsoft mangling if at all possible, since they own the ABI. I DEFINITELY would want any followup patch to be promised for Clang17 (that is, we don't release Clang17 with this patch and NOT that patch), so I'd expect said patch to be available for review before this gets committed.

As far as whether it needs to happen in THIS patch, we can perhaps decide based on the severity of the break, if you can provide examples (or, if it is split into a separate patch, we can use the tests there).

clang/lib/AST/TemplateBase.cpp

244

Why can this not happen now?

clang/lib/Index/USRGeneration.cpp

1040

I have no idea what is valid here. BUT @akyrtzi and @gribozavr (or @gribozavr2 ?) seem to be the ones that touch these files the most?

clang/lib/Sema/SemaTemplate.cpp

8000

Rather than this, I'd prefer these all down to the llvm_unreachable below. If we find they are reachable, then we'd be expected to implement them at that point.

clang/www/cxx_status.html

640

[akyrtzi]

clang/lib/Index/USRGeneration.cpp

1040

Adding @bnbarham to review the Index changes.

[bnbarham]

clang/lib/Index/USRGeneration.cpp

1040

Just visiting the underlying type seems reasonable, ie. VisitType(Arg.getUncommonValueType());. If it needed to be differentiated between a TemplateArgument::Type you could add a prefix character (eg. U), but that doesn't seem needed to me.

[bolshakov-a]

clang/lib/Index/USRGeneration.cpp

1040

Doesn't the holded value be added so as to distinguish e.g. Tpl<1.5> from Tpl<2.5>?

[bnbarham]

clang/lib/Index/USRGeneration.cpp

1040

Ah I see, yeah, we would. And there's no USR generation for APValue currently, which I assume is why your original question came up.

In general a USR just wants to uniquely identify an entity across compilations and isn't as restricted as the mangled name. For basically everything but LValue it seems like you'd be fine to print the value (for eg. int, float, etc), visit the underlying type (array, vector), or the visit the underlying decl (struct, union, member pointer). That's almost true for LValue as well, just with the extra parts that are also added to the ODR hash.

Alternatively, you could also just print the hash from Profile with the same handling as ODR hash. Worst case we'd accidentally merge specializations, but if that's good enough for the ODR hash it's probably good enough here as well.

[bolshakov-a]

clang/lib/Index/USRGeneration.cpp

1040

it seems like you'd be fine to print the value (for eg. int, float, etc)

I'm in doubt about the dot inside a floating point value representation. Minus sign is allowed, as I can see for TemplateArgument::Integral case.

[bnbarham]

clang/lib/Index/USRGeneration.cpp

1040

As long as there's a prefix for APValue and its kind, the dot is fine (eg. maybe @AP@ and then f for float, i for integer, etc).

[bolshakov-a]

clang/lib/AST/MicrosoftMangle.cpp

1734

I've addressed some issues already present on the main branch in D146386. I could try to fix remaining issues in this PR afrer landing that one.

clang/lib/AST/TemplateBase.cpp

244

Adding const to the TemplateArgument::DA::D type and to the TemplateArgument::getAsDecl() return type would lead to many changes unrelated to this PR.

clang/lib/Index/USRGeneration.cpp

1040

Thank you! I've decided to go the simplest way, i. e. to use ODRHash here. Should I write a test case (or some test cases), or they could become fragile due to possible ODRHash implementation changes? I've checked USR locally a little.

[bolshakov-a]

Rename UncommonValue to StructuralValue and add "TODO" about it.

[bolshakov-a]

Add USR generation and a couple of minor fixes.

[bolshakov-a]

Finalize renaming.

[bolshakov-a]

Finalize renaming one more time.

[erichkeane]

I've got the 1 concern with the mangling that I REALLY want one of our codegen owners to chime in on, otherwise this LGTM.

clang/lib/AST/ItaniumMangle.cpp

4594

This is still an open, and we need @rjmccall @eli.friedman or @asl to help out here.

[bnbarham]

clang/lib/Index/USRGeneration.cpp

1040

You could add a test that checks the ref has the same USR as the def, but yeah, I wouldn't specifically check the USR here.

[bolshakov-a]

Add type to USR so as not to get confused with

template <auto*> struct S {};
struct { int n; } s;

template<>
struct S<(void*)&s.n> {};

template<>
struct S<&s.n> {};

Add test on structural type NTTP USR. Just a couple of cases have been added because most of the relevant code should already be covered by odr_hash.cpp test.

[bolshakov-a]

Rebased.

[bolshakov-a]

Rebased; somehow incorrectly merged ReleaseNotes.rst fixed.

CXX/drs/dr12xx.cpp starts to show that clang accepts now references to bitfields as template arguments in C++20 mode. It is probably not OK.

[bolshakov-a]

Avoid binding references in template arguments to bit-fields. @erichkeane, @hubert.reinterpretcast, please verify.

[bolshakov-a]

Fix MS compatibility mangling algorithm. Tested with MSVC ver. 19.35 (toolset ver. 143).

[bolshakov-a]

Rebased.

[bolshakov-a]

clang/lib/AST/ItaniumMangle.cpp

4594

Ping @efriedma, @rjmccall, @asl.

[efriedma]

clang/lib/AST/ItaniumMangle.cpp

4594

I'm not really familiar with the mangling implications for this particular construct, nor am I actively involved with the Itanium ABI specification, so I'm not sure how I can help you directly.

That said, as a general opinion, I don't think it's worth waiting for updates to the Itanuim ABI document to be merged; such updates are happening slowly at the moment, and having a consistent mangling is clearly an improvement even if it's not specified. My suggested plan of action:

• Make sure you're satisfied the proposed mangling doesn't have any holes you're concerned about (i.e. it produces a unique mangling for all the relevant cases). If you're not sure, I can try to spend some time understanding this, but it doesn't sound like you have any concerns about this.
• Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
• Go ahead with this.

4600

I'm not sure what the point of the if (CE->hasAPValueResult()) is; are you just trying to avoid copying the APValue? (If this is going to be a repeating pattern, maybe we can add some sort of utility class to represent the pattern.)

[bolshakov-a]

clang/lib/AST/ItaniumMangle.cpp

4600

I don't know too, it is Richard's code. Looks really like an optimization.

[bolshakov-a]

clang/lib/AST/ItaniumMangle.cpp

4594

Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.

I'm sorry for noting one more time that Richard already pushed these changes in clang upstream, but they had been just reverted.

Maybe, I should make a PR into Itanium API repository, but I probably need some time to dig into the theory and all the discussions. But yes, even NTTP argument mangling rules are not still merged: https://github.com/itanium-cxx-abi/cxx-abi/pull/140

[bolshakov-a]

clang/lib/AST/ItaniumMangle.cpp

4594

@aaron.ballman, @erichkeane, seems like it is already fixed in the ABI document:

Typically, only references to function template parameters occurring within the dependent signature of the template are mangled this way. In other contexts, template instantiation replaces references to template parameters with the actual template arguments, and mangling should mangle such references exactly as if they were that template argument.

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.template-param

See also the discussion in the issue.

[cor3ntin]

It would be great if we could make progress on this for clang 17, I was told it's forcing some people to use other compilers.
Are the recent changes to the itanium abi enough to complete this work? Thanks!

[aaron.ballman]

I've not spotted any major concerns with this patch, but I did have some minor nits to look into. I'd love to hear from @rsmith and @erichkeane before signing off on this, as the changes are pretty involved and they've both done some in-depth looks at the patch already.

clang/include/clang/AST/TemplateBase.h

168–173

Can you add parameter names for the unnamed ones (we typically only leave a parameter unnamed when it is unused).

382–385

clang/lib/AST/MicrosoftMangle.cpp

1621–1623

Does this work or is the const_cast actually required?

clang/lib/AST/ODRHash.cpp

1281–1284

1287

1298–1299

I think this might be a bit clearer, but I don't insist on the change.

clang/lib/AST/TemplateBase.cpp

163–181

408–415

Why did the order of these calls change?

649

Okay, that's reasonable enough to hold off on changing. Thanks!

clang/lib/Sema/SemaOverload.cpp

5990–5993

I thought we could run into situations where we had a ConstantExpr but it did not yet have its result stored in it. Should we assert that isn't the case here?

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp

60

Sometimes we're lacking coverage for existing features, so when updating code in the area, we'll sometimes ask for extra coverage just to be sure we're not regressing something we think might not have a lot of existing test coverage.

clang/www/cxx_status.html

638

[bolshakov-a]

Rebased.

[bolshakov-a]

Fixes after review.

[bolshakov-a]

Btw, formatting of unrelated lines has leaked into TemplateBase.h. Sorry.

clang/lib/AST/MicrosoftMangle.cpp

1621–1623

No, it doesn't compile, likewise the standard C++ dynamic_cast cannot remove constness.

clang/lib/AST/TemplateBase.cpp

408–415

I don't know, it is from 9e08e51a20d0d2. I've tried to invert the order along with the order for StructuralValue, and all tests have been passed.

clang/lib/Sema/SemaOverload.cpp

5990–5993

If I understand correctly, the sole place where ConstantExpr is constructed which may occur here is BuildExpressionFromNonTypeTemplateArgumentValue function, and a value is set into it there. Should I add the assertion into code?

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp

60

temp_arg_nontype.cpp test already has some enum cases. If a case with type alias should be added, it shoud be added there, not in the temp_arg_nontype_cxx20.cpp, I think.

[bolshakov-a]

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp

60

I've just realized that C++98 didn't had type aliases. But typedefs should probably go as well.

[cor3ntin]

It would be nice to figure out a plan for this PR in the next few weeks, before we get kicked out of Phab!