Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by bolshakov-a on Jan 4 2023, 8:33 AM.

Details

Reviewers
aaron.ballman
rsmith
shafik
erichkeane
royjacobson
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

It works, thanks!

erichkeane added inline comments.
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 added inline comments.
clang/lib/Index/USRGeneration.cpp
1040

Adding @bnbarham to review the Index changes.

bnbarham added inline comments.Mar 13 2023, 2:01 PM
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 added inline comments.Mar 13 2023, 2:05 PM
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 added inline comments.Mar 13 2023, 3:09 PM
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 added inline comments.Mar 13 2023, 3:24 PM
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 added inline comments.Mar 13 2023, 3:28 PM
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 added inline comments.Mar 19 2023, 2:11 PM
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.

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

Add USR generation and a couple of minor fixes.

Finalize renaming.

Finalize renaming one more time.

erichkeane added a subscriber: asl.Mar 20 2023, 7:08 AM

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 added inline comments.Mar 21 2023, 2:25 PM
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.

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.

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 added a subscriber: hubert.reinterpretcast.

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

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

bolshakov-a added inline comments.
clang/lib/AST/ItaniumMangle.cpp
4594
efriedma added inline comments.May 14 2023, 12:33 PM
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 added inline comments.May 14 2023, 1:08 PM
clang/lib/AST/ItaniumMangle.cpp
4600

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

bolshakov-a added inline comments.May 14 2023, 2:04 PM
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 added inline comments.Jun 6 2023, 11:40 AM
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.

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!

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

Fixes after review.

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 added inline comments.Aug 6 2023, 4:00 PM
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.

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