[libc++] Fix __wrap_iter copy-assignment in constexpr contexts

[philnik]

Summary

Fixes https://github.com/llvm/llvm-project/issues/52902

In debug mode during constant evaluation the iterator was never assigend. There seem to be no other instances of this bug.

[• Quuxplusone]

LGTM!

I recommend that you use a git commit message that includes either Fixes https://github.com/llvm/llvm-project/issues/52902 or simply Fixes #52902, on a line/paragraph by itself.
For an example of the former, see 960712ccc7103ded3d95e10c61516126836a1eba; for an example of the latter, see 6e28b86cc629c351b1f5d238146af1b6f7ceb785.
(I myself am going to start doing the latter.)
This will tell GitHub to associate that commit with that GitHub issue, and in fact I believe it will auto-close that issue, which is nice.

[Mordante]

Since this fixes a bug I'd like to see a regression test. The fix itself seems correct.

[philnik]

In D116346#3212830, @Mordante wrote:

Since this fixes a bug I'd like to see a regression test. The fix itself seems correct.

Where should I put that test?

[• Quuxplusone]

In D116346#3213932, @philnik wrote:

In D116346#3212830, @Mordante wrote:

Since this fixes a bug I'd like to see a regression test. The fix itself seems correct.

Where should I put that test?

+1, a test is a good idea. But IIUC, this is code which is not yet "active," in the sense that string::begin is not constexpr yet and so it's not really possible to get a std::string::iterator at constexpr time except by default-constructing one.
@philnik, can you go ahead and make libcxx/test/std/strings/basic.string/string.iterators/iterators.pass.cpp constexpr-friendly (presumably by extracting its diff from your larger constexpr-string patch)? And then expand it a little bit so that it also tests operator= in that same function. That seems like the right approach to me.

[philnik]

• Added test

[• Quuxplusone]

libcxx/test/std/strings/basic.string/string.iterators/iterators.pass.cpp

58 ↗

(On Diff #396863)

Before line 57, let's assert ( i1 != i2 );
Notice that this test won't work until std::string becomes constexpr, which is why it's #if'ed out on line 82 below.
...Waitaminute. Since we're still waiting for CI to come back, anyway, could you please investigate why this godbolt (using span instead of string to get its __wrap_iters) does not reproduce the bug? I think it should fail, but I must be missing something.
https://godbolt.org/z/4aGTK79b5

[philnik]

• Added assert

libcxx/test/std/strings/basic.string/string.iterators/iterators.pass.cpp

58 ↗

(On Diff #396863)

You forgot to add -D_LIBCPP_DEBUG=1 to your command line args.

[• Quuxplusone]

libcxx/test/std/strings/basic.string/string.iterators/iterators.pass.cpp

58 ↗

(On Diff #396863)

-D_LIBCPP_DEBUG=1 would make https://godbolt.org/z/4aGTK79b5 fail to compile because std::span<int>::iterator would be just int*, not __wrap_iter<int*>. ...Ah, I get it. Right now __wrap_iter<T>::operator= is broken only at constexpr time and only when _LIBCPP_DEBUG_LEVEL == 2.
At that debug level, span<int>::iterator is not __wrap_iter, so we can't use span to reproduce the bug either.
Okay, I'm convinced that it's impossible to trigger this bug right now, and so the #if'ed-out test you have here is the best we can do, and so IMO once buildkite is back and green, this is good to go.

[Mordante]

LGTM with one minor suggestion, but please wait for the CI to complete.

libcxx/test/std/strings/basic.string/string.iterators/iterators.pass.cpp

82 ↗

(On Diff #396868)

Can we make this test dependent on the implementation status of the constexpr std::string in a similar fashion as testing for char8_t?

[philnik]

• Use feature test macro

[ldionne]

LGTM but please rebase onto main to poke CI.

[philnik]

Rebased to poke CI

libcxx/include/__iterator/wrap_iter.h

__wrap_iter(const __wrap_iter& __x)
         : __i(__x.base())
     {
       if (!__libcpp_is_constant_evaluated())
         __get_db()->__iterator_copy(this, _VSTD::addressof(__x));
     }
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
     __wrap_iter& operator=(const __wrap_iter& __x)
     {
-        if (this != _VSTD::addressof(__x) && !__libcpp_is_constant_evaluated())
+        if (this != _VSTD::addressof(__x))
         {
+            if (!__libcpp_is_constant_evaluated())
                 __get_db()->__iterator_copy(this, _VSTD::addressof(__x));
             __i = __x.__i;
         }
         return *this;
     }
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
     ~__wrap_iter()
     {
       if (!__libcpp_is_constant_evaluated())