[libc++] Remove symbols for a std::allocator_arg & friends from the dylib

[ldionne]

Summary

This patch removes the symbols defined in the library for std::allocator_arg,
std::defer_lock, std::try_to_lock, std::adopt_lock, and std::piecewise_construct.
Those were defined in the library because we provided them in C++03 as an
extension, and in C++03 it was impossible to define them as constexpr
variables, like in the spec.

This is technically an ABI break since we are removing symbols from the
library. However, in practice, only programs compiled in C++03 mode who
take the address of those objects (or pass them as a reference) will have
an undefined ref to those symbols. In practice, this is expected to be
rare. First, those are C++11 features that we happen to provide in C++03,
and only the C++03 definition can potentially lead to code referencing
the dylib definition. So any code that is using these objects but compiling
in C++11 mode (as they should) is not at risk. Second, all uses of these
types in the library is done by passing those types by value to a function
that can get inlined. Since they are empty types, the compiler won't
generate an undefined reference if passed by value, since there's nothing
to pass anyway.

Long story short, the risk for code actually containing an undefined
reference to one of these types is rather small (but non-zero). I also
couldn't find any app on the App Store that referenced these symbols,
which supports my impression that this won't be an issue in practice.

[philnik]

LGTM from my side. We should probably notify vendors?

[ldionne]

In D145587#4178207, @philnik wrote:

LGTM from my side. We should probably notify vendors?

Yes, definitely.

[ldionne]

For reviewers -- note that we could technically keep defining those symbols in the dylib. I could define a .cpp file with something like

struct defer_lock_t { explicit defer_lock_t() = default; };
struct try_to_lock_t { explicit try_to_lock_t() = default; };
struct adopt_lock_t { explicit adopt_lock_t() = default; };

const defer_lock_t  defer_lock{};
const try_to_lock_t try_to_lock{};
const adopt_lock_t  adopt_lock{};

// etc... for others

That way, we would prevent the list of exported symbols from changing and we would still support the (assumed to be) rare previously-built programs that rely on those symbols. However, that would also put all the other conforming programs in the situation of a (probably benign) ODR violation, since there would now be both a symbol provided in the dylib and an inline constexpr symbol defined via the headers. I know this is flagged by at least one implementation -- the linker on Windows will actually error out if we do that, detecting that there's a duplicate symbol. This is actually how I realized this issue and got to this patch series in the first place.

So I think we have to pick our poison. We either break things in a presumably small way today and then we get to live the rest of our lives in a clean world, or we decide to live with those symbols in the dylib forever and with (presumably benign) ODR violations in valid programs forever. Given that I expect the scope of breakage to be small, I am biased towards the first option and that is what this patch realizes, but I am open to discussion.

For completeness: the status quo is also not viable since we currently have both benign ODR violations *and* a non-conforming implementation, which is really the worst of both worlds.

[Mordante]

In D145587#4178293, @ldionne wrote:

For reviewers -- note that we could technically keep defining those symbols in the dylib. I could define a .cpp file with something like

struct defer_lock_t { explicit defer_lock_t() = default; };
struct try_to_lock_t { explicit try_to_lock_t() = default; };
struct adopt_lock_t { explicit adopt_lock_t() = default; };

const defer_lock_t  defer_lock{};
const try_to_lock_t try_to_lock{};
const adopt_lock_t  adopt_lock{};

// etc... for others

It would still be possible if we make this an extra library, which uses can link to manually, or automatically when invoking clang with std=c++98 -stdlib=libc++. (In a similar fashion we do for the experimental library.)

Note that is something we can do, I'm not convinced we should. However when we want to go that route we can look at more C++03 extensions that can be done in the same fashion. However whether or not that is wanted/needed is up to the vendors.

The code changes itself LGTM!

libcxx/lib/abi/CHANGELOG.TXT

25

28

[ldionne]

In D145587#4178814, @Mordante wrote:

It would still be possible if we make this an extra library, which uses can link to manually, or automatically when invoking clang with std=c++98 -stdlib=libc++. (In a similar fashion we do for the experimental library.)

Note that is something we can do, I'm not convinced we should.

Yeah, I agree. For the time being, since I think there won't be actual impact to this change, I would be tempted to go ahead. In the past we've had some success at making these sorts of benign ABI breaks when we did our due diligence to check that nobody would be broken. Here I feel like it's within the acceptable margin of error -- I would be genuinely surprised if someone was actually relying on that due to the explanations in the patch, so I am tempted to keep things simple and just go with it. Also, we are pretty early in the LLVM 17 release cycle so if this breaks a project really badly, we should have plenty of time to make changes.

[ldionne]

Rebase onto main after applying comments.

[Mordante]

In D145587#4204717, @ldionne wrote:

In D145587#4178814, @Mordante wrote:

It would still be possible if we make this an extra library, which uses can link to manually, or automatically when invoking clang with std=c++98 -stdlib=libc++. (In a similar fashion we do for the experimental library.)

Note that is something we can do, I'm not convinced we should.

Yeah, I agree. For the time being, since I think there won't be actual impact to this change, I would be tempted to go ahead. In the past we've had some success at making these sorts of benign ABI breaks when we did our due diligence to check that nobody would be broken. Here I feel like it's within the acceptable margin of error -- I would be genuinely surprised if someone was actually relying on that due to the explanations in the patch, so I am tempted to keep things simple and just go with it. Also, we are pretty early in the LLVM 17 release cycle so if this breaks a project really badly, we should have plenty of time to make changes.

Since this is an ABI break, let's inform the vendors about this change. If this is an issue for other vendors we can consider a C++03 library. For now I'm fine to go ahead with the breakage.