Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::enable_if mangling different in clang >= 18 compared to clang <= 17 or gcc #85656

Closed
Romain-Geissler-1A opened this issue Mar 18, 2024 · 10 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" invalid Resolved as invalid, i.e. not a bug

Comments

@Romain-Geissler-1A
Copy link
Member

Romain-Geissler-1A commented Mar 18, 2024

Hi,

While migrating to clang 18, we hit a name mangling difference compared to clang 17 (or gcc).

The following snippet:

#include <type_traits>

template <typename T> class SomeCondition
{
    public:
        static constexpr bool value = true;
};

template <typename T, typename std::enable_if<SomeCondition<T>::value, int>::type = 0> void SomeFunction(const T&);

void f()
{
    SomeFunction(0);
}

eventually leads to clang 18 mangle the call to SomeFunction as _Z12SomeFunctionIiTnNSt9enable_ifIXsr13SomeConditionIT_EE5valueEiE4typeELi0EEvRKS1_ (see Compiler explorer: https://godbolt.org/z/Khv93qxvf) while clang 17 does mangle it as _Z12SomeFunctionIiLi0EEvRKT_ (Compiler explorer: https://godbolt.org/z/WKeanb15P), and gcc as well (Compiler explorer: https://godbolt.org/z/1eqGKdnzY).

Is this change with clang 18 expected ?

@github-actions github-actions bot added the clang Clang issues not falling into any other category label Mar 18, 2024
@DimitryAndric
Copy link
Collaborator

@philnik777 did a bunch of refactoring touching enable_if:

76a2472 [libc++] Refactor more __enable_ifs to the canonical style (#81457)
640274f [libc++][NFC] Refactor __enable_ifs in to be defaulted template arguments
48c805b [libcxx] replaces SFINAE with requires-expressions in bind_front and bind_back (#68249)
9f3e3ef [libc++][NFC] Refactor __enable_if return types to defaulted template parameters
6256ccf [libc++][NFC] Update the remaining enable_ifs
4da76ea [libc++][NFC] Refactor enable_ifs in defaulted arguments to defaulted template arguments
475bd19 [libc++][NFC] Refactor return type enable_ifs to defaulted template arguments
f3589d2 [libc++][NFC] Refactor the enable_ifs in the math headers

Not sure if these were supposed to change any mangling, though.

@Romain-Geissler-1A
Copy link
Member Author

Romain-Geissler-1A commented Mar 18, 2024

Mmmm I should have written it in my first message, the STL used is libstdc++, not libc++. More precisely in my case, the one of gcc 13 (taken from a rather fresh commit from the releases/gcc-13 branch). Compiler explorer also uses libstdc++ by default.

@philnik777
Copy link
Contributor

This is caused by a fix in the Itanium mangling. You can get the old mangling with -fclang-abi-compat=17: https://godbolt.org/z/zWhKcq9f9

@philnik777 philnik777 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2024
@philnik777 philnik777 added the invalid Resolved as invalid, i.e. not a bug label Mar 18, 2024
@Romain-Geissler-1A
Copy link
Member Author

Ok thanks. So I opened this bug on gcc side to adopt the same mangling change if indeed the historical one was invalid: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114383

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed clang Clang issues not falling into any other category labels Mar 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2024

@llvm/issue-subscribers-clang-frontend

Author: Romain Geissler @ Amadeus (Romain-Geissler-1A)

Hi,

While migrating to clang 18, we hit a name mangling difference compared to clang 17 (or gcc).

The following snippet:

#include &lt;type_traits&gt;

template &lt;typename T&gt; class SomeCondition
{
    public:
        static constexpr bool value = true;
};

template &lt;typename T, typename std::enable_if&lt;SomeCondition&lt;T&gt;::value, int&gt;::type = 0&gt; void SomeFunction(const T&amp;);

void f()
{
    SomeFunction(0);
}

eventually leads to clang 18 mangle the call to SomeFunction as _Z12SomeFunctionIiTnNSt9enable_ifIXsr13SomeConditionIT_EE5valueEiE4typeELi0EEvRKS1_ (see Compiler explorer: https://godbolt.org/z/Khv93qxvf) while clang 17 does mangle it as _Z12SomeFunctionIiLi0EEvRKT_ (Compiler explorer: https://godbolt.org/z/WKeanb15P), and gcc as well (Compiler explorer: https://godbolt.org/z/1eqGKdnzY).

Is this change with clang 18 expected ?

@pinskia
Copy link

pinskia commented Mar 18, 2024

It would be useful to point to the change that was done so the GCC folks can make sure it is done correctly there.
Note this seems like a huge ABI for many uses of enable_if so I am shocked it was decided it would be the default ...

@philnik777
Copy link
Contributor

4b163e3 is the patch that changed it. I've read the commit message again, and it may not actually be intended given that the patch claims that only new constructs are affected, which I don't think is the case here. CC @zygoloid @AaronBallman @erichkeane

@philnik777 philnik777 reopened this Mar 18, 2024
@zygoloid
Copy link
Collaborator

It's intentional that the mangling here changed, and necessary for conformance. Note that this is valid:

template <typename T, typename std::enable_if<SomeCondition<T>::value, int>::type = 0> void SomeFunction(const T&) {}
template <typename T, typename std::enable_if<OtherSomeCondition<T>::value, int>::type = 0> void SomeFunction(const T&) {}

... and declares two different function templates. Without this change, they would mangle the same. It's also valid for one of those templates to be declared in one translation unit and the other to be declared in a different translation unit, resulting in both templates actually being used with the same T in the same program.

It would be useful to point to the change that was done so the GCC folks can make sure it is done correctly there.

See:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100825
itanium-cxx-abi/cxx-abi#24
itanium-cxx-abi/cxx-abi#31
itanium-cxx-abi/cxx-abi#47

@philnik777 philnik777 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2024
@zygoloid
Copy link
Collaborator

Also, apologies for the incomplete commit message. There wasn't much I could do about that once I noticed, but I updated the release notes here: aaa79a5

@Romain-Geissler-1A
Copy link
Member Author

Indirectly related to this, should the demangler show the actual condition to better spot which symbol we are talking about ? Because right now llvm-cxxfilt _Z12SomeFunctionIiTnNSt9enable_ifIXsr13SomeConditionIT_EE5valueEiE4typeELi0EEvRKS1_ reads void SomeFunction<int, 0>(int const&). In the past, this was really the same symbol no matter what the condition was, but if now we can have different symbol, should the symbolizer show the distinct names in the beautified version as well ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" invalid Resolved as invalid, i.e. not a bug
Projects
None yet
Development

No branches or pull requests

7 participants