[libcxx-commits] [PATCH] D101003: [libc++] <span>, like <string_view>, has no use for debug iterators.

Wed Apr 28 14:47:14 PDT 2021

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D101003#2723658 <https://reviews.llvm.org/D101003#2723658>, @Quuxplusone wrote:

> Okay. But remember, through all of this, we //know// that //nobody// is using `span::iterator` **//in _LIBCPP_DEBUG mode//**, because that doesn't compile. The ABI breakage we're worrying about is ABI breakage specifically in the regular non-debug mode.

Yes, I know, that's what I'm talking about too.

>> 1. If someone has `span::iterator` as part of a function signature, it'll be a linker error since the mangling will change (from `__wrap_iter` to `T*`).
>
> Agreed. Would you prefer to make `span` use `__wrap_iter` conditionally? I.e.,

Yes, going down that route is definitely a possibility.

>> 2. This changes the triviality of `span::iterator`, which is good for performance but also changes how it is handled by the calling convention.
>
> No, `__wrap_iter<T*>` is already trivially copyable today (outside of debug mode).
> So I don't think the calling convention would change at all. Example: https://godbolt.org/z/do8o4Ez6z

Oh, I missed the `#ifdef` around the copy constructor. This is actually great news.

> @ldionne, I still think we should just break ABI — stylistically nobody //should// ever be passing `span::iterator` across an ABI boundary //period//,

You are, very obviously, not a vendor :-). Libc++ has historically had a policy of zero tolerance for ABI breaks. This is serious. We can't break our users "because they should not have been writing code in some way", especially since that judgement is mostly subjective. Concretely, people write lots of code, and what you think might never happen actually does happen, and sometimes even for good reasons.

I've actually been trying to relax that policy one little step at a time, for example with `std::pointer_safety`. You'll say it's trivial and I agree, however it's still a step in the right direction. Now, the issue at hand with `span::iterator` is a lot more serious IMO, and we can't take it lightly (apologies if I'm mis-perceiving how you take the situation).

> Also, better to break early (before claiming C++20 support) than late;

Where do we *not* claim C++20 support? This is an entirely theoretical argument. We support `std::span` the day we ship it in our headers without some sort of explicit `unavailable` attribute (or something along those lines). That very day, which was about 3 years ago, we decided that we'd support `std::span` in a stable way pretty much forever.

> if we don't break ABI now I think we're just kicking the tech-debt can down the road to where it will be even //harder// to break ABI in the future. However, I do think the solution I put in response to #1 would be OK if you really wanted to kick that can.

I think @Mordante's solution is the better solution here. I suggest this:

1. We conditionally make `std::span::iterator` be `T*` when (1) debug mode is enabled, or (2) an ABI macro is defined.
2. That ABI macro is DISABLED by default, so that we're using `__wrap_iter` all the time by default (i.e. no ABI break).
3. We then switch to enabling the ABI macro by default along with a release note that explains the ABI break. We add a note to contact the libcxx list if they ever turn that macro on, because otherwise that option will be removed at [some release].

I want to do (3) in a separate step so that we can test it on its own (e.g. build all of Apple/Google/others with the change). It will also make it easier to revert if needed, and to isolate the change in case it causes an issue. Then, if nobody says anything and there is no breakage in the wild, we can remove the macro altogether in a few releases, assuming nobody ever actually needed to define that macro to bring back the old behavior.

It's more complicated than just making the change, but it's the only reasonable way to proceed IMO. That way, we fix the issue in the short term, we have no tech debt in the long term, and we don't break users leaving them helpless.

Also, the only reason while I'm OK with making the change at all is because it'll be a linker error. If it were a runtime error (e.g. if the triviality had changed), I don't think we could ever go to step (3), and that would always stay a feature enabled only in the unstable ABI.

Are you OK with that direction? If so, please implement step (1) and (2) here, and we can have (3) be a separate review.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101003/new/

https://reviews.llvm.org/D101003