[libcxx][test] Do not assume array::iterator is a pointer by strega-nil · Pull Request #100603 · llvm/llvm-project

strega-nil · 2024-07-25T17:20:28Z

In the tests I added for ranges::find_last{_if{_not}}, I accidentally introduced an assumption that same_as<array<T, 0>::iterator, T*>; this is a faulty assumption on MSVC-STL.

Fixes #100498.

cc @StephanTLavavej, can you test this PR with the MSVC-STL?

In the tests I added for `ranges::find_last{_if{_not}}`, I accidentally introduced an assumption that `same_as<array<T, 0>::iterator, T*>`; this is a faulty assumption on MSVC-STL.

llvmbot · 2024-07-25T17:21:00Z

@llvm/pr-subscribers-libcxx

Author: nicole mazzuca (strega-nil)

Changes

In the tests I added for ranges::find_last{_if{_not}}, I accidentally introduced an assumption that same_as<array<T, 0>::iterator, T*>; this is a faulty assumption on MSVC-STL.

Full diff: https://github.com/llvm/llvm-project/pull/100603.diff

3 Files Affected:

(modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp (+2-1)
(modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp (+2-1)
(modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp (+2-1)

diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp
index 2a2b12fb2c288..036631b19f48a 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last.pass.cpp
@@ -61,7 +61,8 @@ template <class It, class Sent = It>
 constexpr void test_iterators() {
   using ValueT    = std::iter_value_t<It>;
   auto make_range = [](auto& a) {
-    return std::ranges::subrange(It(std::ranges::begin(a)), Sent(It(std::ranges::end(a))));
+    return std::ranges::subrange(
+        It(std::to_address(std::ranges::begin(a))), Sent(It(std::to_address(std::ranges::end(a)))));
   };
   { // simple test
     {
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp
index a15f81bd4e481..427ef3539947f 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp
@@ -59,7 +59,8 @@ static_assert(!HasFindLastIfR<ForwardRangeNotSentinelEqualityComparableWith>);
 
 template <class It, class Sent>
 constexpr auto make_range(auto& a) {
-  return std::ranges::subrange(It(std::ranges::begin(a)), Sent(It(std::ranges::end(a))));
+  return std::ranges::subrange(
+      It(std::to_address(std::ranges::begin(a))), Sent(It(std::to_address(std::ranges::end(a)))));
 }
 
 template <template <class> class IteratorT, template <class> class SentinelT>
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp
index bb0e411acf0fa..f8357f9eecb93 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if_not.pass.cpp
@@ -59,7 +59,8 @@ static_assert(!HasFindLastIfR<ForwardRangeNotSentinelEqualityComparableWith>);
 
 template <class It, class Sent>
 constexpr auto make_range(auto& a) {
-  return std::ranges::subrange(It(std::ranges::begin(a)), Sent(It(std::ranges::end(a))));
+  return std::ranges::subrange(
+      It(std::to_address(std::ranges::begin(a))), Sent(It(std::to_address(std::ranges::end(a)))));
 }
 
 template <template <class> class IteratorT, template <class> class SentinelT>

ldionne

This LGTM once CI is green, but I am surprised that this wasn't caught by our unstable-ABI job. That job should define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY and _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW, which use __wrap_iter in array and string_view and should catch those issues.

StephanTLavavej · 2024-07-25T22:56:05Z

@strega-nil There are still errors with MSVC's STL. Full logs: clang_errors.txt

Here's my analysis of the first error, which is:

D:\GitHub\STL\llvm-project\libcxx\test\std\algorithms\alg.nonmodifying\alg.find.last\ranges.find_last_if.pass.cpp(104,21): error: cannot cast from type 'iterator' (aka '_Array_iterator<int, 0>') to pointer type 'it' (aka 'const std::_Array_iterator<int, 0> *')
  104 |       assert(ret == it(a.begin()));
      |                     ^~~~~~~~~~~~
C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt\assert.h(40,17): note: expanded from macro 'assert'
   40 |             (!!(expression)) ||                                                              \
      |                 ^~~~~~~~~~
D:\GitHub\STL\llvm-project\libcxx\test\std\algorithms\alg.nonmodifying\alg.find.last\ranges.find_last_if.pass.cpp(193,3): note: in instantiation of function template specialization 'test_iterator_classes<add_const_to_ptr_t, std::type_identity_t>' requested here
  193 |   test_iterator_classes<add_const_to_ptr_t, std::type_identity_t>();
      |   ^

https://github.com/strega-nil/llvm-project/blob/04a67b5617d11a94627c5ee89f24e2fff9ce8eda/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp#L104

https://github.com/strega-nil/llvm-project/blob/04a67b5617d11a94627c5ee89f24e2fff9ce8eda/libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp#L188-L193

It appears that add_const_to_ptr_t contains the assumption, since it's using remove_pointer_t on an array iterator.

strega-nil · 2024-07-26T07:43:27Z

This LGTM once CI is green, but I am surprised that this wasn't caught by our unstable-ABI job. That job should define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY and _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW, which use __wrap_iter in array and string_view and should catch those issues.

It looks like defining _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY is not enough to get array<T, 0>::iterator to be int*, because array<T, 0> is its own template specialization.

additionally, actually fix the rest of the tests now that I can actually test them.

strega-nil · 2024-07-26T09:33:13Z

@ldionne would you mind taking a look again? I've now actually changed product code (in array), since it failed to wrap the iterator for array<T, 0>.

StephanTLavavej · 2024-07-27T00:39:41Z

The tests are now passing for MSVC's STL (completely with Clang; with MSVC modulo a compiler bug in constexpr evaluation). Thanks!

mordante

In general it looks good, but I'd like to understand the constexpr changes for the contiguous_iterator

libcxx/include/array

libcxx/test/support/test_iterators.h

libcxx/include/array

github-actions · 2024-07-27T19:13:33Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Zingam · 2024-07-28T04:10:59Z

@strega-nil Off-topic question: are the changes to array and contigious_iterator supposed to fix this type of an error:

error: no matching conversion for functional-style cast from 'std::__wrap_iter<int *>' to 'contiguous_iterator<int *>'

strega-nil · 2024-07-28T10:09:32Z

@strega-nil Off-topic question: are the changes to array and contigious_iterator supposed to fix this type of an error:

error: no matching conversion for functional-style cast from 'std::__wrap_iter<int *>' to 'contiguous_iterator<int *>'

No, these would fix contiguous_iterator<std::__wrap_iter<int*>> to work at all.

`array<T, 0>::iterator` was always a pointer even when `_LIBCXX_ABI_USE_WRAP_ITER_IN_STD_ARRAY` was defined. This patch fixes that minor bug. Discovered as part of [llvm#100603][]. [llvm#100603]: llvm#100603

cjdb

LGTM with one request!

cjdb · 2024-07-29T20:05:28Z

libcxx/test/std/algorithms/alg.nonmodifying/alg.find.last/ranges.find_last_if.pass.cpp

  friend constexpr bool operator==(NonConstComparable&, const NonConstComparable&) { return true; }
 };

+// note: this should really use `std::const_iterator`


Can you open an issue and change this to a TODO instead please? AIUI we're only not doing that because const_iterator isn't around yet.

Sure, I'll open an issue after merging.

Oh, I meant using the issue in the todo as TODO(<#issue-number>): <reason>.

Oh, I meant using the issue in the todo as TODO(<#issue-number>): <reason>.

This was a part of my conditional LGTM. I will make it clearer in future reviews.

strega-nil · 2024-07-30T09:28:39Z

Merging as the only failure was agent lost, and the previous commit (before a comment-only change) did pass.

`array<T, 0>::iterator` was always a pointer even when `_LIBCXX_ABI_USE_WRAP_ITER_IN_STD_ARRAY` was defined. This patch fixes that minor bug. Discovered as part of [#100603][]. Drive-by: switch from `typedef` to `using` in `<array>` [#100603]: #100603

`array<T, 0>::iterator` was always a pointer even when `_LIBCXX_ABI_USE_WRAP_ITER_IN_STD_ARRAY` was defined. This patch fixes that minor bug. Discovered as part of [#100603][]. Drive-by: switch from `typedef` to `using` in `<array>` [#100603]: llvm/llvm-project#100603 NOKEYCHECK=True GitOrigin-RevId: d067062a42b0ce591f03c15cb76fe0fb27d1d9c1

[libcxx] Do not assume array::iterator is a pointer

d08c22c

In the tests I added for `ranges::find_last{_if{_not}}`, I accidentally introduced an assumption that `same_as<array<T, 0>::iterator, T*>`; this is a faulty assumption on MSVC-STL.

strega-nil requested a review from a team as a code owner July 25, 2024 17:20

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 25, 2024

strega-nil changed the title ~~[libcxx] Do not assume array::iterator is a pointer~~ [libcxx][test] Do not assume array::iterator is a pointer Jul 25, 2024

make sure to include <memory>

04a67b5

ldionne approved these changes Jul 25, 2024

View reviewed changes

fix array<T, 0>::iterator under ABI_USE_WRAP_ITER

92fda36

additionally, actually fix the rest of the tests now that I can actually test them.

strega-nil requested a review from ldionne July 27, 2024 07:29

mordante reviewed Jul 27, 2024

View reviewed changes

libcxx/include/array Outdated Show resolved Hide resolved

libcxx/test/support/test_iterators.h Outdated Show resolved Hide resolved

libcxx/test/support/test_iterators.h Outdated Show resolved Hide resolved

philnik777 reviewed Jul 27, 2024

View reviewed changes

libcxx/include/array Outdated Show resolved Hide resolved

CRs - remove array changes

135531e

strega-nil force-pushed the array-pointer-iterators-testing-bug branch from 8857422 to 135531e Compare July 27, 2024 19:14

format

ed1bc6d

cjdb approved these changes Jul 29, 2024

View reviewed changes

switch a note to a TODO

19d4819

strega-nil merged commit c6b192a into llvm:main Jul 30, 2024

strega-nil deleted the array-pointer-iterators-testing-bug branch July 30, 2024 09:29

strega-nil mentioned this pull request Jul 30, 2024

[libc++] fix array<T, 0> under USE_WRAP_ITER #101156

Merged

StephanTLavavej mentioned this pull request Aug 24, 2024

Update llvm-project microsoft/STL#4910

Merged

Conversation

strega-nil commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 25, 2024

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Jul 25, 2024

Uh oh!

strega-nil commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil commented Jul 26, 2024

Uh oh!

StephanTLavavej commented Jul 27, 2024

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zingam commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil commented Jul 28, 2024

Uh oh!

cjdb left a comment

Choose a reason for hiding this comment

Uh oh!

cjdb Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

strega-nil Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjdb Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

cjdb Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

strega-nil commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

strega-nil commented Jul 25, 2024 •

edited

Loading

strega-nil commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 27, 2024 •

edited

Loading

Zingam commented Jul 28, 2024 •

edited

Loading

strega-nil Jul 29, 2024 •

edited

Loading