[libcxx-commits] [PATCH] D129823: [libc++][ranges] Make range algorithms support proxy iterators

[Jordan Rupprecht via Phabricator via libcxx-commits]

rupprecht added a comment.

In D129823#3680764 <https://reviews.llvm.org/D129823#3680764>, @rupprecht wrote:

> In D129823#3680525 <https://reviews.llvm.org/D129823#3680525>, @ldionne wrote:
>
>> Thanks everyone for chiming in on this issue. I am speaking to @var-const right now who summarized the issues to me. It looks like there is some UB in MySQL right now because their funky iterator type is lying about `iterator_traits<It>::reference`, and that's really the crux of the problem IMO. That needs to be fixed, and it's not something that libc++ can accommodate by making the code work.
>>
>> **However**, it also seems like it's something that we can easily catch at compile-time instead, hence my suggestion <https://reviews.llvm.org/D130538#inline-1256036> to add a `static_assert` in D130538 <https://reviews.llvm.org/D130538>. It will still break Chromium via its MySQL dependency, however the error will clearly point out how it is broken and how to fix it.
>
> Emphatic +1 to this, fixing the UB is better than hacking up libc++, and a static assert would be amazing here. I'd much rather handle a clear build breakage than spend hours/days hunting down the source of a weird runtime error.
>
>> @rupprecht If you are able to get us a reproduction of the failure on top of D130538 <https://reviews.llvm.org/D130538> (without the `static_assert`), we'd be interested in taking a look to understand whether there's something more subtle at play. However, our current understanding is that D130538 <https://reviews.llvm.org/D130538> + `static_assert` will work in all cases (i.e. either it will work as intended, or it will issue a compile-time diagnostic). If you see additional issues after we move forward with D130538 <https://reviews.llvm.org/D130538>, please let us know via the usual means. This has been really useful in finding subtle problems early and we definitely want to address such issues with the highest priority.
>
> I am attempting to do this, although it's a little more challenging now that the failure is moved later -- it was relatively easy to provide the previous repro because the test runner crashed at the moment something went wrong; producing wrong results without crashing requires more specific knowledge of mysql internals.

Sorry about the delay. I have a slightly evil reproducer, modified from my original one, that shows the incorrect sort result if we take the `static_assert` out of D130538 <https://reviews.llvm.org/D130538> so we can keep using a buggy iterator: https://godbolt.org/z/xfhseoaW7 (also attached)

  pre-sort
  	 varlen_element[4] = 2436
  	 varlen_element[5] = 2438
  	 varlen_element[6] = 2439
  	 varlen_element[7] = 2437
  	 varlen_element[8] = 2440
  post-sort
  	 varlen_element[4] = 2436
  	 varlen_element[5] = 2438 <-- should be a 2437 here
  	 varlen_element[6] = 2439
  	 varlen_element[7] = 2439 <-- oops, 2439 is here twice
  	 varlen_element[8] = 2440

I don't know if there's anything interesting you want to look at here -- we should expect nothing less than incorrect results by writing a UB iterator. The iterator is fixed now, and the `static_cast` in D130538 <https://reviews.llvm.org/D130538> catches the buggy version of the iterator.

In fact it looks like the assert has found a few more iterators that are either getting away with UB or have bad test coverage, so we're definitely glad the assert is there now.

F24096317: repro-D129823-2.cpp <https://reviews.llvm.org/F24096317>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823