[libcxx-dev] How do _LIBCPP_DEBUG=1 "debug iterators" ever work, when linking against libc++.a?

[Arthur O'Dwyer via libcxx-dev]

Hi folks,

I recently added a buildkite CI step that runs libc++'s test suite with
-D_LIBCPP_DEBUG=1. This turns on "debug iterators," which is a thing in the
library that maintains a collection of all known container instances and
iterator instances, and updates the collection on every
container-construction, container-destruction, container-modification,
iterator-construction, iterator-destruction, or iterator-modification. This
enables it to print an error message on any iterator-modification,
iterator-comparison, or iterator-dereference that violates the
iterator-invalidation guarantees of the Standard.
https://reviews.llvm.org/D100866 was the review that added that CI step.
I marked a bunch of tests as "LIBCXX-DEBUG-FIXME", with the intent of
fixing them all quickly. The patches for all-but-one of those FIXMEs are:
https://reviews.llvm.org/D101674
https://reviews.llvm.org/D101675
https://reviews.llvm.org/D101676
https://reviews.llvm.org/D101677
https://reviews.llvm.org/D101678
https://reviews.llvm.org/D101679

I've just tracked down the very final LIBCXX-DEBUG-FIXME. Unfortunately,
it's a doozy.
The symptom is a segfault (null pointer dereference) in
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.proximate/proximate.pass.cpp

It's a segfault instead of a _LIBCPP_ASSERT-failure, because the
compiled-to-binary filesystem library was not built with _LIBCPP_ASSERT
enabled. Okay, no problem, I should enable debug assertions in the library
for this particular CI builder. I believe this is the patch for that:

```
--- a/libcxx/cmake/caches/Generic-debug-iterators.cmake
+++ b/libcxx/cmake/caches/Generic-debug-iterators.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_TEST_PARAMS "debug_level=1"
"additional_features=LIBCXX-DEBUG-FIXME" CACHE STRING "")
 set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
+set(LIBCXX_ENABLE_ASSERTIONS ON CACHE BOOL "")
```

Now the symptom is a _LIBCPP_ASSERT-failure. The problem is that we're
move-constructing a string, and the source string was never registered with
the debug-iterators library.

Reduced test case:
```
cat > test.cpp <<EOF
#include <string>
void test_debug_function(std::__libcpp_debug_info const&) {
    std::abort();
}
int main(int, char**) {
    std::__libcpp_set_debug_function(&test_debug_function);
    std::string x = "1234567890123456789012345678901234";
    std::string y = x;  // calls into the compiled library
    std::string z = std::move(y);
}
EOF
bin/clang++ test.cpp -D_LIBCPP_DEBUG=1 lib/libc++.a lib/libc++abi.a
./a.out
Abort trap: 6
```

The problem seems to be that
(1) libc++.a needs to copy strings sometimes, so it contains codegen for
the copy constructor
(2) libc++.a is compiled with -D_LIBCPP_DEBUG=0, i.e., assertions but no
debug iterators
(3) Therefore any strings created by the copy constructor inside libc++.a,
don't get registered with the debug-iterators library
(4) Therefore pretty much everything is affected by unpredictable
assert-fails??
I'm amazed that only one test out of our entire test suite seems to be
affected by this issue. I would think that pretty much everything in the
debug-iterators CI should be failing because of this; I'm not sure how it's
working right now.

The real question is, how was this ever *supposed* to work? It seems like
the person-who-compiled-libc++.a and the
person-compiling-their-user-code-against-libc++-headers need to agree on
the setting of _LIBCPP_DEBUG:— If libc++.a has _LIBCPP_DEBUG=1 and the user
code doesn't, everything explodes; and also vice versa. But this doesn't
seem realistic.

None of the offending basic_string constructors are currently marked with
_LIBCPP_INLINE_VISIBILITY. I suspect that adding _LIBCPP_INLINE_VISIBILITY
could help reduce the surface area of the problem even further... but based
on what I said above, I don't see how the problem *ever* gets to zero,
unless we can somehow prevent libc++.a from ever constructing a string (or
at least never letting strings-it-constructs leak out into the user's code
where _LIBCPP_DEBUG=1 might be in effect).

https://reviews.llvm.org/D48616 may be related — it added some non-inline
constructors — but the review comments don't talk about inline or
visibility at all.

What am I missing here? Thoughts?

–Arthur
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-dev/attachments/20210501/5731ddda/attachment.html>