Skip to content

[libc++] ABI compat breakage using reference type for std::set key_compare #118559

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

Closed
hubert-reinterpretcast opened this issue Dec 3, 2024 · 4 comments · Fixed by #118685
Closed
Labels
ABI Application Binary Interface libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@hubert-reinterpretcast
Copy link
Collaborator

The following compiled fine with libc++ version 19:

#include <set>

struct Cmp {
  __int128 x;
  bool operator()(__int128, __int128) const;
};

struct MySet {
  unsigned char b;
  std::set<__int128, const Cmp &> s;
};

extern char q[sizeof(MySet)];
extern char q[40];

It no longer compiles with libc++ trunk (https://godbolt.org/z/cTbEovcer). It seems the size of MySet has changed because of a change in std::set.
The draft release notes (https://libcxx.llvm.org/ReleaseNotes.html#abi-affecting-changes) mention issues with overaligned empty types, but nothing about reference types.

@hubert-reinterpretcast hubert-reinterpretcast added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 3, 2024
@rnk
Copy link
Collaborator

rnk commented Dec 4, 2024

I am personally surprised by how alignof works on references, but it is consistent with sizeof. alignof(__int128&) is 16, not 8 or pointer-aligned. That seems to be the root cause here.

__compressed_pair_padding has some accommodation to avoid adding extra padding around references, but there isn't analogous logic to fix up the alignment attribute.

@jwakely
Copy link
Contributor

jwakely commented Dec 4, 2024

std::set<__int128, const Cmp &> is somewhere in the intersection of underspecified, undefined, and just a very bad idea.

@AaronBallman AaronBallman added the ABI Application Binary Interface label Dec 4, 2024
@hubert-reinterpretcast
Copy link
Collaborator Author

std::set<__int128, const Cmp &> is somewhere in the intersection of underspecified, undefined, and just a very bad idea.

I tend to agree; however, from that point of view, I think a deliberate compile-time error is better than the ABI break (documented or not).

ldionne added a commit to ldionne/llvm-project that referenced this issue Dec 4, 2024
…rence comparators

While reference comparators are a terrible idea and it's not entirely
clear whether they are supported, fixing the unintended ABI break is
straightforward so we should do it as a first step.

Fixes llvm#118559
@ldionne
Copy link
Member

ldionne commented Dec 4, 2024

I do agree that reference comparators are a terrible idea. But this was an unintended ABI break and (unless I missed something) this is easy to address, at least until we officially decide to make such comparators ill-formed: #118685

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Application Binary Interface libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
5 participants