Differential D126818
Itanium ABI: Implement mangling for constrained friends Authored by erichkeane on Jun 1 2022, 1:23 PM.
Details
Thanks to concepts, C++ can now have friends that differ by their In addition to mangling the containing scope, this ALSO adds the 'F'
Diff Detail Event TimelineComment Actions 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. Comment Actions After thinking about it, I'm about 99% sure I made the wrong choice here. I should have gone with #3. That likely requires the infrastructure from the deferred concepts patch, so I'll get that ready, then rebase this to be ready. Comment Actions I wonder if I'm reading (temp.friend)p9` sentence 2 correctly. Which of these should it be parsed as?
I'm guessing that the first interpretation is the intent. Comment Actions I believe it is the 2nd one actually. A friend function with a constraint, where said constraint depends on a template param from an enclosing template, shall be a definition. And only THOSE are protected against being a 'duplicate' of another in a different scope. I essentially implemented the former in this (as it is a superset of the cases) for the purposes of mangling, but I think that was the wrong idea. The result would be that when the constraint does NOT depend on a enclosing template, we end up emitting different names for the same function in different TUs (potentially). Initially I thought that was fine, but it ends up with extra duplication. Note we might be confused, the parens there aren't completely clear as to what your intent is. Comment Actions 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?
Comment Actions I'm a touch uneasy as well, but there at least seems to be agreement in this solution (at least according to Richard). I'm hopeful that @rjmccall can comment as to whether this is the 'final' version here, or what we can do to finalize it. The Microsoft mangling IS a problem as well, the Microsoft compiler has the same issue (https://godbolt.org/z/cnf6nM1aj), but I don't want to fix it there until MSVC does so that we don't diverge on their implementation. In the meantime, these would just not be callable in that situation (as they are now). Comment Actions
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:
Comment Actions Yes, I understand that Clang doesn't currently properly implement [temp.friend]p9, and it is my belief too that none of the other compilers get it right at the moment either. Clang approximates it by just directly comparing partially instantiated constraints, so it gets it MOSTLY right. I believe I have all of your test cases in my implementation of [temp.friend]p9 as a part of the deferred concepts patch here: https://reviews.llvm.org/D126907 Aaron, my, and at least 1 other core expert's reading was: -A non template friend declaration with a requires-clause shall be a definition. >> That is, ALL non-template friends, no matter what the requires-clause depends on. -A friend function template with a constraint that depends on the template parameter from an enclosing template shall be a definition. >> That is, when the declaration is a friend function template, it must be a definition ONLY IF it depends on a template param from an enclosing template. THOSE two sentences aren't particularly relevant, except for setting up the two groups (that is, ALL constrained non-template friends and constrained friend function templates where the constraint depends on an enclosing template). The third sentence is the valuable one to both of these: -Such a constrained friend function or function template declaration does not declare the same function or function template as a declaration in any other scope. >> That is, any friend in either of those two groups from sentence 1 and sentence 2 do not conflict with the same lexical declaration in a different instantiation. THIS patch attempts to implement the mangling required in that last sentence, however it doesn't differentiate the "ONLY if it depends on a template param from an enclosing template", which I intend to fix in a future revision, once the infrastructure from my deferred concepts patch (see link above) has made it into the repo. Comment Actions SO I believe my 'future revision' part in the above is actually no longer necessary. We prohibit that with a Sema 'redefinition error': https://godbolt.org/z/1r549vfK4 So the question here is whether we want 'baz' in that godbolt link to use the 'F' mangling or not. @rjmccall? The 'easiest' thing to do I think (based on this patch) is to ALWAYS use the 'F' mangling for a friend with a constraint. WDYT? Comment Actions Updated to work via option-3 above, I believe this is the appropriate solution based on the itanium proposal and the C++ standard. @rjmccall, what concerns do we still have about Itanium being willing to add this? Do we think any progress is EVER going to be made? Comment Actions I'm too often slow to actually apply edits to the ABI document. There's been plenty of time for feedback on this one; go ahead and act like it's accepted. Comment Actions Ah! I see! I thought last time you had held this patch up on waiting for someone to do the work in the ABI group. Thank you for the update! So this should be ready for review now. It is much smaller than the previous version, since we started setting the bit for 'FriendConstraintRefersToEnclosingTemplate' during declaration creation. Comment Actions CWG 2596 was discussed at Kona and, afaict, CWG is opting for a path of least effort, with a different result than what is implemented this patch and previously discussed in the ABI issue:
Comment Actions Can you clarify the difference here? What did we choose different? The example from that CWG issue is exactly in the test for this (see the top of useS) so I'm not sure what difference we're missing? Can you clarify what I'm not matching here? Comment Actions I wrote the CWG issue and the example therein indeed flags exactly what is being addressed by this patch, but I'm referring particularly to the comment added from CWGs Kona meeting 2022-11-10 (see now emphasized quote above). I may be mistaken, but I read it as proposing to make useS ill-formed, emphasizing that you need to include the class somewhere in the hidden friend's signature, whereas overloading as shown in the useS example would not be supported. For more details, see the Kona minutes at EDG-wiki. Comment Actions Hmm... I'm still a bit confused and hoping someone else can show up and let me know how to fix this. There is quite a bit of work to do temp.friend p9 that I implemented to mean a constrained-non-template friend or a constrained-template-friend-that-refers-to-enclosing-scope don't conflict, based on the wording. The minutes aren't particularly clear to me unfortunately as to what they really want, particularly since the suggested wording says the opposite of what I THOUGHT the discussion was doing at the end? The wording/example are still consistent with my interpretation/implementation, so I have no idea what is going on. We might find ourselves wanting to hold off until CWG comes up with actual wording? The "Refers to an enclosing template" part seems sensible enough (and I have work in place to detect and store that), so the question is whether we want non-template friends to conflict ever. IMO, the current rule (implemented in clang and here, where constrained non-template friends are somehow different) is a little odd-ball, but at least easy enough to reason about. I think perhaps we need to wait on CWG to clarify what they mean, at least by including a wording consistent with that top thing. The unfortunate part here is that Clang implements 1/2 of this at the moment: we implement the SEMA changes, but not the mangling changes for the current wording. Comment Actions Same reflection, and agreed.
Should consider asking on the CWG reflector to make sure they are aware of Clang's quite far-going implementation experience with fixing this defect (in the way originally proposed), and of Itanium C++'s/@rjmccall's view that the proposed ABI updates looks reasonable? Comment Actions To be clear, my opinion is just that the requested change is implementable, as I understand it. I'm not trying to advocate for any specific language change. Comment Actions I agree that each version of what we THINK the requested change might be is implementable, but at the moment, what the requested change IS is clear as mud :) Comment Actions That's totally fair. I just want to be clear that I'm not advocating for any particular language change right now and would need to read up on the concrete alternatives to render a more specific opinion. | ||||||||||||||||||||||
Formatting looks off here -- should clang-format the patch.