Confirmed, this is not just weird, it's an (unintended) ABI break.

The problem is that p.first can't be laid out at offset 0, because that would put two objects of __non_trivially_copyable_base at that offset, which C++'s layout rules don't permit.

The easiest solution would be to template __non_trivially_copyable_base on the same template arguments as the pair.

Thanks Richard for the suggested fix.

This ABI break has been shipping in FreeBSD for a while now. While I'm happy to re-commit a fix, we should ask the FreeBSD folks when they're going to be able to take a real ABI break on pair so we can make it trivial.

@​emaste and @​dim Could you weigh in on behalf of FreeBSD?

Adding _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR accidentally broke the ABI as described in this bug. Fixing in "breaks" the ABI again. Wh

Fix up for review as https://reviews.llvm.org/D56357

This is has always been a bit of a

@​emaste and @​dim Could you weigh in on behalf of FreeBSD?

Adding _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR accidentally
broke the ABI as described in this bug. Fixing in "breaks" the ABI again. Wh

The "Wh" was maybe part of a sentence that you were either going to finish, or decided not to add? :)

In any case, this std::pair trivial copy constructor has always been a little bit of an ugly wart for FreeBSD, since we were the first to actually ship with a libc++.so using this bad ABI.

I don't really know what our plans are breaking ABI, we'll probably have to go for libc++.so.2 at some point (and with /usr/include/c++/v2 too?), but for backwards compat reasons, we will likely always have to ship a libc++.so.1 too. (Or it will go into a compat-for-freebsd-12-and-earlier package.)

Ideally you would solve such an issue with a linker script and symbol versioning, but this is probably hard with libc++.

That said, I don't think anybody will actually want to depend on having the extra padding that this bug report is about, so I think it is likely best to apply the D56357 fix at some point, and maybe even backport it to older FreeBSD branches.

Ed, what's your opinion on this?

The "Wh" was maybe part of a sentence that you were either going to finish,
or decided not to add? :)

Yeah, it was just a mistake. Ignore it.

In any case, this std::pair trivial copy constructor has always been a
little bit of an ugly wart for FreeBSD, since we were the first to actually
ship with a libc++.so using this bad ABI.

I don't think std::pair ever crosses the shared library boundary. If it doesn't then we can omit any discussion about shipping and versioning libc++.so. Then the only concern becomes programs and libraries that mix libc++ versions and pass std::pair between them. I'm assuming mixing libc++ header versions is a case FreeBSD supports?

That said, I don't think anybody will actually want to depend on having
the extra padding that this bug report is about, so I think it is likely
best to apply the D56357 fix at some point, and maybe even backport it to
older FreeBSD branches.

OK. Note that by "depending" on the extra padding really just means "I mixed libc++ headers of different versions", because accessing p.first depends on the if there is padding. So we will be breaking all programs that pass std::pair<std::pair<T, U>, V> between differently compiled TU's.

Roughly, my point is that if we're going to break some ABI's using std::pair, We should consider changing triviality at the same time.

However, note that D56357 only affects cases of nested pairs or a pair<T, U> where T has a pair at offset zero. But changing the triviality will break all usages of std::pair. Again, these cases only matter if FreeBSD supports programs and libraries which are compiled or linked against more than one version of the libc++ headers (not including libc++.so).

Ed, what's your opinion on this?

I'm going to go ahead and commit the fix since the release branch is tomorrow.

@​Ed or @​Dimitry, if you object to this fix, please speak up ASAP.

Committed in r351290.

Looks like this has been fixed, so I'm going to close this to clean up the bug list.