This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Diagnose bad iterators in the classic algorithms
AbandonedPublic

Authored by philnik on Jun 8 2023, 2:23 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

philnik created this revision.Jun 8 2023, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 2:23 PM
Herald added a subscriber: wenlei. · View Herald Transcript
philnik requested review of this revision.Jun 8 2023, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 2:23 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I mainly glossed over the patch, but I don't see any tests. Are there existing test for these requirements?

I mainly glossed over the patch, but I don't see any tests. Are there existing test for these requirements?

No, there aren't any. I first wanted to make sure this will happen, before putting the work into adding tests for this.

ldionne added subscribers: rupprecht, EricWF, Restricted Project, ldionne.Jun 13 2023, 3:59 PM

I think we definitely want to do this because it could catch genuine bugs and also hardens us against Hyrum's law. The big challenge here is rolling this out without it being too painful. First things first. I'll run this on a significant code base to get an idea of how many failures we get.

@rupprecht or @EricWF Could you also take this for a spin at Google once the CI is green? @philnik Please ping us here once this is ready to be taken for a spin.

Adding vendors for awareness.

libcxx/include/__algorithm/transform.h
1

We're missing a release note.

eaeltsin added a subscriber: eaeltsin.EditedJun 14 2023, 1:01 AM

Hi Louis,

We'll run it on Google codebase as soon as it is declared ready.

But please note this can take a couple of days :)

Hi Louis,

We'll run it on Google codebase as soon as it is declared ready.

But please note this can take a couple of days :)

Thanks! It takes a few days for us as well. We're not in any rush, I just wanted to make sure we got your attention!

@philnik will ping once this is ready to go.

philnik updated this revision to Diff 531852.Jun 15 2023, 11:37 AM

Try to fix CI

@eaeltsin @ldionne This should be ready for a testing round.

ldionne added a comment.EditedJun 15 2023, 2:46 PM

@eaeltsin @ldionne This should be ready for a testing round.

Ref for my purposes: 110860823&110860826 111042279&111042282

Saw a number of failures on Google's codebase sample, running more tests to understand the scale better.

Okay, we have a significant amount of failures from this, will likely require 2-3 weeks of cleanups.

What is also interesting, I see failures from compiling bits of the LLVM project itself, for example:

TOOLCHAIN/include/c++/v1/__algorithm/for_each.h:26:3: error: static assertion failed
   26 |   _LIBCPP_REQUIRE_CPP17_INPUT_ITERATOR(_InputIterator);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
TOOLCHAIN/include/c++/v1/__iterator/cpp17_iterator_concepts.h:159:72: note: expanded from macro '_LIBCPP_REQUIRE_CPP17_INPUT_ITERATOR'
  159 | #    define _LIBCPP_REQUIRE_CPP17_INPUT_ITERATOR(iter_t) static_assert(::std::__cpp17_input_iterator<iter_t>);
      |                                                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SRC/clang/lib/Basic/Sarif.cpp:89:8: note: in instantiation of function template specialization 'std::for_each<llvm::sys::path::const_iterator, (lambda at SRC/clang/lib/Basic/Sarif.cpp:89:30)>' requested here
   89 |   std::for_each(++Iter, End, [&Ret](StringRef Component) {
      |        ^
TOOLCHAIN/include/c++/v1/__algorithm/for_each.h:26:3: note: because 'llvm::sys::path::const_iterator' does not satisfy '__cpp17_input_iterator'
   26 |   _LIBCPP_REQUIRE_CPP17_INPUT_ITERATOR(_InputIterator);
      |   ^
TOOLCHAIN/include/c++/v1/__iterator/cpp17_iterator_concepts.h:159:72: note: expanded from macro '_LIBCPP_REQUIRE_CPP17_INPUT_ITERATOR'
  159 | #    define _LIBCPP_REQUIRE_CPP17_INPUT_ITERATOR(iter_t) static_assert(::std::__cpp17_input_iterator<iter_t>);
      |                                                                        ^
TOOLCHAIN/include/c++/v1/__iterator/cpp17_iterator_concepts.h:87:20: note: because '(void)__lhs++' would be invalid: cannot increment value of type 'llvm::sys::path::const_iterator'
   87 |       { (void)__lhs++ };
      |                    ^
1 error generated.

Okay, we have a significant amount of failures from this, will likely require 2-3 weeks of cleanups.

Interesting, thanks for the feedback. My builds are still running, I had to restart them after some stupid mistake I made.

Do you feel like it would be helpful to introduce this behind a macro? We could make it opt-in for one release, and then make it opt-out for one release, and then make it mandatory (or something along those lines). I feel like anything that takes 2-3 weeks of cleanups is significant enough that it should allow being done gradually.

Here is another interesting bit - from all the failures that I'm seeing, ~70% are not in our internal code but in the open source code we are using (including the LLVM project itself). Fixing those problems in corresponding upstream repositories looks like a significant effort (here is the reason for my rough estimation of 3 weeks).

Yes, I think we need to be able to opt-out for quite some time.

I do see a number of issues as well. Definitely something we can deal with, but it's not going to be trivial. Given the impact, I think we should introduce this behind an opt-in flag for LLVM 17, then move it to opt-out for LLVM 18, and then in LLVM 19 make it mandatory. That will give plenty of time for folks to migrate.

philnik abandoned this revision.Sep 21 2023, 1:56 AM
libcxx/test/libcxx/algorithms/bad_iterator_traits.verify.cpp