Okay, got Chrome building locally. The things to sort out:

- The __debug_less business is pickier about const correctness. There's a bunch of CLs of this flavor to upload. Those are worth fixing anyway, whether or not this is technically a bug in libc++.
  https://chromium-review.googlesource.com/c/v8/v8/+/1272184

- I hacked around the std::{lower,upper}_bound __debug_less issues, but I think this should be fixed in libc++.

- There's some symbol visibility issue in the components build I'll likely need libc++ folks to confirm I'm doing it right.

- It doesn't start when iterator debugging is enabled. Perhaps some issue with iterators being used post-fork, since iterator debugging takes locks for some reason. I remember we had similar issues with the glibc version of this feature.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d62627d4c47bb676111242a9e4347ed24a9dab14

commit d62627d4c47bb676111242a9e4347ed24a9dab14
Author: David Benjamin <davidben@chromium.org>
Date: Wed Oct 10 01:04:17 2018

Const-correct CompareCookies.

libc++'s _LIBCPP_DEBUG gets upset when comparators take non-const
references.

Bug: 893810
Change-Id: I0079d410e707a34cfe33a4a5a80a92eaac876ec4
Reviewed-on: https://chromium-review.googlesource.com/c/1272164
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598158}
[modify] https://crrev.com/d62627d4c47bb676111242a9e4347ed24a9dab14/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d4f749cae4ab75a89a210472c9b7156f6a58ab4d

commit d4f749cae4ab75a89a210472c9b7156f6a58ab4d
Author: David Benjamin <davidben@chromium.org>
Date: Wed Oct 10 15:23:38 2018

Const-correct DelayedEntryCompare.

libc++'s _LIBCPP_DEBUG gets upset when comparators take non-const
references.

Bug: chromium:893810
Change-Id: I838ff08bfd53893984f0ce41a9d78d6f1d80a324
Reviewed-on: https://chromium-review.googlesource.com/c/1272184
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56533}
[modify] https://crrev.com/d4f749cae4ab75a89a210472c9b7156f6a58ab4d/src/libplatform/default-foreground-task-runner.h

+glider for libc++ OWNERS, how do I go about reporting a bug or making a change to libc++?

The bug is that it expects the comparator to std::lower_bound and std::upper_bound to satisfy Compare, namely that comp(a, b) == true implies comp(b, a) == false.

But unlike std::sort, std::upper_bound and std::lower_bound don't compare two Ts against each other. The iterator's value type (T1) may be different from the search key (T2). std::lower_bound expects a comp(T1, T2) and std::upper_bound expects a comp(T2, T1). This means that asserting on comp(b, a) may not even compile, since the types have switched.

For example, this call relies on operator<(RegionInfo, StringPiece), which exists. But operator<(StringPiece, RegionInfo) doesn't exist.
https://cs.chromium.org/chromium/src/components/autofill/core/browser/address_rewriter.cc?type=cs&q=address_rewriter.&sq=package:chromium&g=0&l=35
https://cs.chromium.org/chromium/src/components/autofill/core/browser/address_rewriter.h?type=cs&sq=package:chromium&g=0

Here's a slightly goofier example where T1 == T2 = NameToPseudoStruct, but it's actually trying to compare against an AtomicString.
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_selector.cc?rcl=5815972405d41213aa37c6b76185b3129cbc59eb&l=414
I think that one should get fixed so that name is passed into std::lower_bound rather than dummy_key, but then we'd hit the T1 != T2 case and fail to compile with _LIBCPP_DEBUG rather than crash at runtime.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0e6e5e55ddbe204af360c102e9494441a29fbbc

commit b0e6e5e55ddbe204af360c102e9494441a29fbbc
Author: David Benjamin <davidben@chromium.org>
Date: Wed Oct 10 19:35:36 2018

Don't rely on implicit conversion in ReadyFrame binary search.

_LIBCPP_DEBUG gets confused by the conversion. I think the check in
_LIBCPP_DEBUG is in error and should be removed for std::lower_bound, so
this would be moot when/if that's fixed, but it looks like the code
meant to pass the pre-converted ready_frame rather than converting each
comparison.

Bug: 893810
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I60754caf520b7e6f41c119c09f747fe1e22568c4
Reviewed-on: https://chromium-review.googlesource.com/c/1272128
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598453}
[modify] https://crrev.com/b0e6e5e55ddbe204af360c102e9494441a29fbbc/media/filters/video_renderer_algorithm.cc

David, https://bugs.llvm.org/ is the place to report a libc++ bug.

Doesn't asan do the bound checking already?

I remember _GLIBCXX_DEBUG being a bit of a headache and I was glad we essentially got rid of it.

Were the _GLIBCXX_DEBUG issues the threading ones with their iterator checks or something else? libc++ has a lighter debug mode (0 vs 1) that turns off the iterator checks. I'm mostly interested in that one. Iterator checks would be nice but, yeah, the locking does not interact well with fork.

ASan doesn't quite bounds-check as accurately as length-aware checks in the STL would, since it rounds up to allocations. E.g. I believe palmer had to fix several problems when making base::span bounds-check, and the fuzzers would often find stuff.

What I'm actually trying to do is evaluate bounds checks in release mode. Right now we're in this poor situation where containers in //base can have checks, but once they graduate to std, they lose them. Setting _LIBCPP_DEBUG=0 in debug mode seemed a good starting point. (Though it seems to also have this __debug_less thing, so maybe we want something more targeted.)

What we really need is a change in the C++ standard, right? There should be a `#define COWBOY_SPEED_YOLO 1` option for people who want their users to get hurt, and it should be guaranteed to work the same on all platforms.

Extra debugging (which should also exist, but can be implementation-defined/-activated) is a different thing than basic safety in 2019.

Yeah. I was hoping _LIBCPP_DEBUG was effectively that for libc++, but we probably don't want the __debug_less stuff in release builds (or do we?). Still running it on debug seems useful just so we have slightly more confidence that flipping what we actually want on release won't break the world.

Also it means we can evaluate how _LIBCPP_DEBUG=0 (again, this doesn't include the iterator stuff) performs in release mode, if not as something to ship since it's not quiiite what we want, to get a sense of the cost.

I do wonder if these new contract attributes are what we want. I don't know if the expectation is that the STL will include these annotations or if there's a mode where people put "cheap but important" checks where the expectation is folks will run with them in release mode.
https://en.cppreference.com/w/cpp/language/attributes/contract

#10: I'd expect the STL to change to include these checks in future versions. It's beyond the scope of this bug, of course. :)

Interesting. The __debug_less bug isn't quite that it doesn't account for std::lower_bound using different types. Something funny happens here since they'd already attempted to fix it. Still investigating...

https://bugs.llvm.org/show_bug.cgi?id=17147

Ah, it's specifically if one uses the default operator<-based comparison, because __less<T1, T2> in libc++ emits overloads for all of {T1,T2} < {T1,T2}. That breaks the heterogeneous-comparator detection in __debug_less since the reversed method does exist. It just calls something that doesn't exist.

Aha. Okay, the problem is that __less<T1, T2> wants to do this because most functions ask that either type fit in the first or second slot.
https://en.cppreference.com/w/cpp/algorithm/equal_range
https://en.cppreference.com/w/cpp/algorithm/binary_search

However, lower_bound knows which slot is which.
https://en.cppreference.com/w/cpp/algorithm/lower_bound

So they need a separate __less<T1, T2> template that only emits the one comparator used specifically for lower_bound and upper_bound. But I probably should check the spec rather than just use cppreference. 

Does it make sense to also try `-fsanitize-bounds`, either as part of this bug or in a separate bug? E.g.:

+      #  Add bounds checking.
+      cflags += [ "-fsanitize=bounds" ]
+      cflags += [ "-fno-sanitize-recover=undefined" ]
+      ldflags += [ "-lubsan" ]
+      # To get the most compact code:
+      #cflags += [ "-fsanitize-trap=all" ]

Filed https://bugs.llvm.org/show_bug.cgi?id=39458 for the __debug_less issue. Hopefully I filed it right?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bf36a0584562dc9662804a91d03105de1b6977a

commit 1bf36a0584562dc9662804a91d03105de1b6977a
Author: David Benjamin <davidben@chromium.org>
Date: Tue Oct 30 06:39:52 2018

Rewrite NameToPseudoCompare to avoid dummy_key.

Versions of libc++ prior to https://llvm.org/PR39458 trip on this code
in _LIBCPP_DEBUG, as they expect operator< to behave consistently. While
that has since been fixed, the comparator is a little odd. This version
is a little clearer.

Bug: 893810
Change-Id: I92f83b621bf6e057beafa5f6ce8f383715f82474
Reviewed-on: https://chromium-review.googlesource.com/c/1305875
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603807}
[modify] https://crrev.com/1bf36a0584562dc9662804a91d03105de1b6977a/third_party/blink/renderer/core/css/css_selector.cc