Page MenuHomePhabricator

Itanium ABI: Implement mangling for constrained friends
Changes PlannedPublic

Authored by erichkeane on Jun 1 2022, 1:23 PM.

Details

Reviewers
rjmccall
Group Reviewers
Restricted Project
Summary

Thanks to concepts, C++ can now have friends that differ by their
containing function. Because of this, we need to differentiate ones
from different instantiations. This patch implements itanium-cxx-abi
issue #24 (https://github.com/itanium-cxx-abi/cxx-abi/issues/24).

In addition to mangling the containing scope, this ALSO adds the 'F'
differentiator as proposed.

Diff Detail

Event Timeline

erichkeane created this revision.Jun 1 2022, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 1:23 PM
erichkeane requested review of this revision.Jun 1 2022, 1:23 PM

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.

erichkeane planned changes to this revision.Jun 2 2022, 6:08 AM

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.

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.

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.

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.

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.

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?

clang/include/clang/AST/Decl.h
2495
clang/lib/AST/ItaniumMangle.cpp
668–669

Formatting looks off here -- should clang-format the patch.

672–674

I don't know that repeating the comment here is helpful.

1733

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?

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).

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.

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.

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.