All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 'info'.

In D126907#3617713, @ChuanqiXu wrote:

In D126907#3615807, @erichkeane wrote:

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of uses for concept). And all the tests passed now. So it looks like it wouldn't cause regression failure.

The implementation basically looks good to me. (This is really large and I can't be sure I don't miss something). Only some minor issues remained.

Is there any chance you can validate this against https://reviews.llvm.org/D126907 as well? We touch similar code, and I'm intending to get that committed in the near future. Otherwise, after a quick look, I don't see anything particualrly bad, other than a lack of release notes and update of www-status.

Rebased!

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

So I don't think this is the right fix for this... But I DID check and see that the test case seems to be fixed here: https://reviews.llvm.org/D126907 , which does a lot of rework for this sort of thing.

I'm generally OK with this. BUT Aaron/JF/Tom should review it.

In D127812#3601476, @danielkiss wrote:

In D127812#3599530, @aaron.ballman wrote:

In D127812#3587223, @ilinpv wrote:

In D127812#3585249, @erichkeane wrote:

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts.

Is it *necessary* that it diverges like this? (Is there some published standards document you're trying to conform to, is there an implementation difficulty with not diverging, something else?)

In ACLE there are a few rules/behaviour documented around what should be the behaviour of the "default" for example. Making for example "default" optional hopefully makes the adation of multi versioning seamless as possible. Compilers won't support from day one these attributes therefore the goal was to make the whole addition of a multi versioned function as less distributive as possible while the code is still compiled with older compilers too.

// this is the original code
// mandating __attribute__ ((target ("default"))) would not work with the today's compilers
void foo(){}

// new backward compatible code
#ifdef __ARM_FEATURE_FUNCTION_MULTI_VERSIONING
__attribute__ ((target_version("fancy_feature")))
void foo(){}
#endif

if the "default" is not mandated here, it felt not right to mandate it for the target_clones either.

"target" attribute has been already used and supported on AArch64 in a different sense, like target("arm"), target("dotprod"), target("branch-protection=bti"). The intention of creating new "target_version" attribute is not to overlap with that. It also has different format, mangling and semantic, e.g. treating function without attribute as "default", and option to disable attribute droping function multi versions. Do these explanations dispel your concern?

Do I understand correctly that AArch64 was using an attribute named target which does not behave like the attribute with the same name in GCC and Clang on other architectures, and now you'd like to introduce a new attribute which does behave like target but under a different name? If I have that correct, I don't think that's reasonable -- there's far too much possibility for confusion with that situation already, and adding a new attribute only increases the confusion. I'm not certain where the technical debt came from, but we shouldn't increase the burden on users here; I think target and target_clones should be made to work consistently across architectures if at all possible.

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

Ah, i see! Thanks for the explanation!

In D126907#3600876, @ChuanqiXu wrote:

Great progress!

In D126907#3599835, @erichkeane wrote:

Note that the failure comes down to:

template<typename T> concept C = T::f();
template<template<C> class P> struct S1{};
template<C> struct X{};
S1<X> s11;

and requires the -frelaxed-template-template-args flag:

[ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp -frelaxed-template-template-args
temp.cpp:5:4: error: template template argument 'X' is more constrained than template template parameter 'P'
S1<X> s11;
   ^
temp.cpp:3:29: note: 'P' declared here
template <template<C> class P> struct S1{};
                            ^
temp.cpp:4:20: note: 'X' declared here
template<C> struct X{};
                   ^
1 error generated.

As far as I could tell, we could omit the diagnostic by deleting https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/include/clang/Sema/SemaConcept.h#L45-L53

This change is obsolutely wrong but it shows the bug comes from ParameterMapping so we could locate https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/lib/Sema/SemaConcept.cpp#L723 further.

Note that the failure comes down to:

As promised, got it down to the 1 failure, and added tests for @ChuanqiXu and the std::vector example. That all seems to work, just a problem with the CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp test left.

Made good progress today, but got trapped down the CheckDeducedArgs path for quite a bit longer with partial specializations. I have all of the crashes I know of 'fixed', but have a few tests that still fail for unknown reasons. Because of that, i don't have anything I can really upload, so I'll have to do so mid-next-week.

In D126907#3591195, @ChuanqiXu wrote:

In D126907#3588708, @erichkeane wrote:

In D126907#3588417, @ChuanqiXu wrote:

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.

---

Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

I don't know of the 'another idea'? We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation. And the problem is that the RawCompletionToken uses a concept to refer to 'itself'. However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'. However, this IS happening during instantiation?

My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't? I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.

Yeah, we have to do instantiation before checking. My point is that it looks like we're doing another instantiation when we check the concepts. And my point is that if it we should avoid the second instantiation.

In D127593#3590072, @Javier-varez wrote:

I did try this locally and the built test binaries seem to crash with a segmentation fault. I run one of them in lldb to check the error and I got this backtrace:

llvm-project/build on  fix-trivially-copyable-check [?⇕] via △ v3.22.3 took 3s ❯ lldb /Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp
(lldb) target create "/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp"
Current executable set to '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64).
(lldb) r
Process 61339 launched: '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64)
Process 61339 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
   198
   199 	  /* dynamically updated stack window for uber threads to avoid get_specific
   200 	     call */
-> 201 	  if (!TCR_4(other_threads[i]->th.th_info.ds.ds_stackgrow)) {
   202 	    KMP_FATAL(StackOverflow, i);
   203 	  }
   204
Target 0: (omp_single_copyprivate.c.tmp) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
    frame #1: 0x00000001003b8c60 libomp.dylib`void __kmp_release_template<kmp_flag_64<false, true> >(flag=0x000000016fdff0b8) at kmp_wait_release.h:790:39
    frame #2: 0x00000001003b8c20 libomp.dylib`::__kmp_release_64(flag=0x000000016fdff0b8) at kmp_wait_release.cpp:25:46
    frame #3: 0x00000001003798d0 libomp.dylib`__kmp_reap_thread(thread=0x0000000101010a40, is_root=0) at kmp_runtime.cpp:6146:9
    frame #4: 0x000000010037497c libomp.dylib`__kmp_internal_end() at kmp_runtime.cpp:6334:7
    frame #5: 0x00000001003745ec libomp.dylib`::__kmp_internal_end_library(gtid_req=-1) at kmp_runtime.cpp:6498:3
    frame #6: 0x0000000100374258 libomp.dylib`::__kmp_internal_end_atexit() at kmp_runtime.cpp:6115:3
    frame #7: 0x0000000100374218 libomp.dylib`::__kmp_internal_end_dtor() at kmp_runtime.cpp:6083:3
    frame #8: 0x0000000189359dc4 libsystem_c.dylib`__cxa_finalize_ranges + 464
    frame #9: 0x0000000189359b68 libsystem_c.dylib`exit + 44
    frame #10: 0x000000018947aec4 libdyld.dylib`dyld4::LibSystemHelpers::exit(int) const + 20
    frame #11: 0x00000001000150d4 dyld`start + 592
(lldb)

Not sure how to proceed from here though... I'm really not familiar with OpenMP

I probably need to spend more time on this at one point, but first look seemed fine to me. I think the approach is about right, and the solution is there.

In D126907#3588417, @ChuanqiXu wrote:

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.

---

Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

In D126907#3585626, @erichkeane wrote:

See here: https://github.com/llvm/llvm-project/issues/55673

This might be the same issue, the crash at least looks the same.

See here: https://github.com/llvm/llvm-project/issues/55673

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

In D126907#3584555, @ChuanqiXu wrote:

I finally found some time to look at the crash. Although I haven't get an idea, I found it crash at the following one too:

template <typename T>
concept Constraint = true;

template <typename... Signatures>
class completion_handler_async_result {
public:
    template <typename Initiation, Constraint RawCompletionToken>
    static void initiate(Initiation&& initiation, RawCompletionToken&& token);
};

template <typename... Signatures>
concept Constraint2 =
    requires() {
        completion_handler_async_result<Signatures...>::initiate(0, 0);
    };

template <typename T>
void use(int F)
    requires Constraint2<int>
{}

The crash log is:

clang/lib/AST/ExprConstant.cpp:15083: bool clang::Expr::EvaluateAsConstantExpr(clang::Expr::EvalResult&, const clang::ASTContext&, clang::Expr::ConstantExprKind) const: Assertion `!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.

I am not sure if they are the same problem.

In D127593#3583125, @royjacobson wrote:

Thank you! LGTM. I see the CI still has those OpenMP failures - could you try to rebase on main to get it green?

@erichkeane is this good to go? Should I ping some ABI people about this?

Did another quick read through and made a handful of 'nit' level suggestions. Since much of it is my words, I'd love it if someone else could take a read through as a double-check.

In D127593#3579518, @royjacobson wrote:

I don't think the OpenMP tests are failing because of this PR, they are a bit flaky sometimes.

@erichkeane - Since we haven't branched clang15 yet, I think it's Ok not to add the Ver15 value to the ABI options?

The organization and content of the document are quite good, I learned quite a lot, so thank you! I found the prose itself a bit difficult to read, so did my best to reword in each section as I could. Feel free to take/modify/review whatever you'd like of it.

The command line on one of the tests is a little strange to me, and I don't see the need for the -std=gnu11 on either, but the test content itself looks fine enough to me.

In D127593#3576538, @royjacobson wrote:

Hi Javier, thank you for submitting this patch!

As far as I could tell, this patch implements the CWG2171 defect report from 2016: https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2171. That means that you'll have to add a test inside clang/test/CXX/drs/dr21xx.cpp to make sure it shows on the DRs status page.

This change is also a large ABI break, which means it has to be considered carefully. I think the best approach here would be to revert to the previous behavior when the -fclang-abi-compat flag is used for clang <= 14, but I'm not sure about this. I would like the approval of someone with more ABI experience here.

The code itself and the tests look good! If you'll need help with the changes I mentioned please let me know.

So from what I can tell, the problem is evaluating Constraint on initiate, and trying to find the 'right' argument from the MultiLevelTemplateArgumentList for RawCompletionToken. The problem is that the RawCompletionToken, as a template parameter, gets 'filled in to' the Concept relative to initiate instead of relative to the "root".

In D127487#3573667, @ilya-biryukov wrote:

I've checked that the change works fine on top of D126907. The bug is still there after D126907 and gets fixed by this.
Also, the merge conflict is actually minimal, no code changes intersect.

I'm not quite understanding this yet, so I'll have to take another look early next week. However, I AM intending to get https://reviews.llvm.org/D126907 committed in the next week or so. Could you perhaps see how it interacts with that? Its a sizable, multi-month project that I'd like to make sure doesn't get stuck in a rebase-loop again.

I was able to reduce it today. BUT didn't get a chance to debug:

In D126907#3568943, @ChuanqiXu wrote:

In D126907#3566633, @erichkeane wrote:

All of the comments from @ChuanqiXu done, thank you so much!

As far as the crash you're seeing: That is typically a 'depth mismatch' kinda thing. They sadly only show up when the types of the template arguments are different (that is, pack vs integral vs type), but I thought I got all of them with examples.

I'll keep hunting for more during the day today, but any amount of reproducer you can provide (even if not minimized) would be incredibly helpful!

Oh, if a not minimized reproducer is desired, you could find it at: https://github.com/alibaba/async_simple. It should crash when we run 'make' in build directory. (The version of libstdc++ is 10.2 in my environment)

I would try to make a reduced example in the next week if we don't go on smoothly.

LGTM! Thanks for this!

All of the comments from @ChuanqiXu done, thank you so much!

In D126907#3565491, @ChuanqiXu wrote:

Oh, the diff part is small really. The change looks good to me and I've tested it for libcxx.

But it crashed at "clang/lib/Sema/SemaTemplateInstantiate.cpp:1852: clang::QualType {anonymous}::TemplateInstantiator::TransformTemplateTypeParmType(clang::TypeLocBuilder&, clang::TemplateTypeParmTypeLoc): Assertion `Arg.getKind() == TemplateArgument::Type && "Template argument kind mismatch"' failed" in an internal library. I am trying to get a reduced example. I would suggest to do more testing with other workloads (with libstdc++ >= 10)

In D126818#3562355, @tahonermann wrote:

Note we might be confused, the parens there aren't completely clear as to what your intent is.

Well, I know that I'm confused and not clear what my intent is :)

I asked the question because there appears to be an asymmetry between (temp.friend)p9 sentence 1 and (temp.friend)p9 sentence 2. Sentence 1 applies to all constrained non-template friend declarations regardless of any template argument dependence while sentence 2 applies to just a subset of constrained friend function templates; those that have some amount of template dependence. The difference impacts when mangling differences are required.

I spent some time analyzing how gcc, clang, and MSVC handle these different cases. See https://godbolt.org/z/85E5eMh3x. Search for FIXME? to find cases where I think one or more of the compilers is misbehaving or where it is unclear to me whether or how [temp.friend]p9 applies. Some highlights:

• Some compilers diagnose some ill-formed cases when parsing the class template, others don't until the class template is instantiated. Not surprising.
• All of the compilers reject non-template friend function definitions with non-dependent constraints due to duplicate definitions, presumably in violation of [temp.friend]p9; see the ff2() example.
• All of the compilers reject non-template friend function definitions with dependent constraints due to duplicate definitions, presumably in violation of [temp.friend]p9; see the ff6() example.
• Name mangling is currently insufficient to differentiate (otherwise) non-dependent friend function templates with dependent constraints; see the fft5() and fft6() examples.
• None of the compilers reject some cases of non-definitions that should be rejected by [temp.friend]p9; see the fft5() and fft7() examples.

@ChuanqiXu : i was hoping you could take a look at this, since you did such a great job reviewing the rest of this (note this is mostly the same patch as the last one, just with the 'friends' stuff dealt with).

This mostly/all seems reasonable to me, and the diagnostics, while not perfect, are IMO a vast improvement.

In D126818#3555843, @aaron.ballman wrote:

I'm a bit uneasy that this is implementing something that's not yet been accepted into the Itanium ABI document. That runs the risk of requiring an ABI break if the Itanium document changes directions before finalizing. Also, what should we be doing for the Microsoft mangling, or do we already handle that properly?

In D126818#3554617, @tahonermann wrote:

I wonder if I'm reading (temp.friend)p9` sentence 2 correctly. Which of these should it be parsed as?

1. A (friend function template with a constraint) that depends on a template parameter from an enclosing template shall be a definition.
2. A friend function template (with a constraint that depends on a template parameter from an enclosing template) shall be a definition.

I'm guessing that the first interpretation is the intent.

In D125402#3553855, @erichkeane wrote:

In D125402#3553841, @aaron.ballman wrote:

In D125402#3553825, @erichkeane wrote:

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

I don't think that is possible? I think 'defaultedMoveConstructor' includes the user typing StructName(StructName&&) = delete;.

Note that out of line deleted implementations are prohibited: https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.

In D125402#3553841, @aaron.ballman wrote:

In D125402#3553825, @erichkeane wrote:

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

In D126818#3551504, @erichkeane wrote:

Note this was mentioned as a 'must also do' in my discussion on deferred constraints with @rsmith, however as this already fails, I don't see this as a prerequisite. However, I think this is easy enough to just do.

THAT SAID: The proposal on the itanium-abi github wasn't particularly clear as to which of the following three options were the 'right' answer. I'm hoping @rjmccall can decide whether this is acceptable:

Option 1: Change the mangling of ALL friend functions. I determined this is likely an ABI break on otherwise 'settled' code, and figured we wouldn't want to do this.

Option 2: Change the mangling for ALL constrained friend functions. This seemed easy enough to do, AND changes the mangling for things only covered by 'experimental' concepts support. So I think changing the ABI for current things is OK here.

Option 3: Change the mangling for constrained friend functions that fall under temp.friend p9. The only difference between this and Option2 is that this would need to check the 'template friend function depends on containing scope'. I don't believe this is necessary so long as we are consistent with it. Presumably this could end up with multiple symbols for functions that are the 'same declaration' by rule in separate translation units, such as:

struct Base{};
template<int N>
struct S {
  template<typename U>
  friend void func(Base&) requires(U::value) {}
};
void uses() {
  S<1> s;
  S<2> s2; // This is an error during Sema, since 'func' is a duplicate here.
  func(s); // all S<N>s SHOULD end up with the same declaration, but in different TUs they might end up with different copies here?
}

In typing this comment up, I believe I got the 'option' wrong here. Should I have gone with #3 above? I'm going to think about it overnight (and hope you reviewer folks can chime in!) and get to it in the morning.

In D125802#3553015, @aaron.ballman wrote:

In D125802#3552047, @browneee wrote:

It looks like the leak is rooted at the allocation here:
https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L3857

The VarTemplateSpecializationDecl is allocated using placement new which uses the AST structure for ownership: https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/lib/AST/DeclBase.cpp#L99

The problem is the TemplateArgumentListInfo inside https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/DeclTemplate.h#L2721
This object contains a vector which does not use placement new: https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/TemplateBase.h#L564

Apparently ASTTemplateArgumentListInfo should be used instead https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/TemplateBase.h#L575

Wow, thank you for the fantastic sleuthing! It seems that declaration is nine years old, so I'm surprised the leak is only being discovered now and as part of this particular test case.

I don't have a particularly easy way to test this locally at the moment -- do you know if switching VarTemplateSpecializationDecl::TemplateArgsInfo to be a ASTTemplateArgumentListInfo solves the issue for you?

Note this was mentioned as a 'must also do' in my discussion on deferred constraints with @rsmith, however as this already fails, I don't see this as a prerequisite. However, I think this is easy enough to just do.

I think I'm Ok with this as a 1st step for the cleanup. I think we should probably evaluate what amount of work is necessary to extract the ExtInfo out into trailing storage, depending on whether it saves us room in the FunctionProtoType and FunctionNoProtoType types.

In D126642#3547905, @sdesmalen wrote:

In D126642#3547526, @erichkeane wrote:

Right, yeah. One thing to consider: ExtInfo was 'first', and so it got to keep the 'in bitfield bits'. However, much of what is in there is, IMO, not something that is used often enough to be worth having in there. Of the bits being used there, I think 'NoReturn' is the only one used with any regularity (other than perhaps 'this call' in Windows-32-bit machines). I wonder if we should consider moving as much of that as possible over.

That sounds sensible. Some of the fields there are also valid in FunctionNoProtoType though, and I don't believe I saw an equivalent to ExtProtoInfo for FunctionNoProtoType, is that because it's uncommon enough that it only requires changing in a handful of places? I'm not too familiar with Clang code, so I didn't look to deep into this.

I agree with the above, we probably need an RFC on discourse for this.

In D126642#3547496, @sdesmalen wrote:

In D126642#3547284, @erichkeane wrote:

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here.

The reason I repurposed the bit was because I previously tried adding a bit to unsigned ExtInfo : 13; but that fails

static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase), "changing bitfields changed sizeof(Type)!");

This seems innocuous enough, and seems to better reflect the intent of the standard, so LGTM.

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here. Also, the 'hasBitfields' seems like the answer should just be 'no' in the case when its 'no'...

I'd prefer we limit the scope of this to JUST a 'do-while' loop. I think we can clarify the "if an inner statement uses braces, the outer one should too", AND special-case the do-while loop as needing braces.

I have a change to the release note that I'd like to see improved in SOME way, but I trust you to correct/reword as you wish. I'm still not particularly happy with the mechanism of the test, but I cannot come up with a way to cause the Semantic Analyzer to cause this.

How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?

Also needs some release notes.

In D119544#3526810, @ChuanqiXu wrote:

In D119544#3494281, @erichkeane wrote:

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I agree this should be a better direction.

@rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is supposed to fix the friend function issues:

Took me a bit to get the prose, but I think I got it now. PERHAPS there is use to 'finish the thought' on the outcome there? Either way, LGTM.

This likely also needs a Release Note.

I think I agree with the justification here, though am a touch confused in the test. I'm also a touch concerned that we have getSubobjectSizeInBits returning a 'rounded up to power of 2' bit happening here. The bitfield case returns non-powers-of-two, but the _BitInt case does not.

In D125711#3517065, @royjacobson wrote:

Code and added/modified tests LGTM!

Do you think we should add a release note, given that it could break existing code? Its seems a bit unlikely, but the amount of broken tests have made me a bit worried.
Also, maybe mention the github issue (https://github.com/llvm/llvm-project/issues/51173) in the commit message.

In D124726#3516896, @ken-matsui wrote:

@nathanchance

Oops, thank you for your comment!
I would like to work on a hotfix patch for this issue.

@aaron.ballman

Should we entirely opt-out of this suggestion on .S files? I think there are other approaches here, such as decreasing the max edit distance to avoid suggesting this case in .S files, but this approach is avoiding just this edge case so that it would possibly bring a new issue like this. Conversely, opting out of this suggestion on the whole .S files will not be able to catch any typoes. Considering possible edge cases (# directives are also treated as comments), I think opting out would be the only way, unfortunately. What do you think?

For now, I will create a patch opting out of this suggestion on .S files.