Page MenuHomePhabricator

Add a buildkite for _LIBCPP_DEBUG=1.
ClosedPublic

Authored by Quuxplusone on Tue, Apr 20, 10:01 AM.

Details

Summary

Add a debug-mode CI step.
My general roadmap is intended to be:

  • Land the changes to Generic-debug-iterators.cmake, run-buildbot, and buildkite-pipeline.yml, plus marking all offending tests as UNSUPPORTED: LIBCXX-DEBUG-FIXME.
  • Land the fix to support N3644 "Null Forward Iterators" in debug mode (D100881). (This is done.)
  • Land a fix to <span> (D101003) and unmark those tests.
  • Fix other failures, unmarking tests that pass, and changing others from UNSUPPORTED: LIBCXX-DEBUG-FIXME to UNSUPPORTED: debug_level_0, debug_level_1 as appropriate. (This is done, locally. I haven't submitted PRs for them yet though.)
  • Don't leave any LIBCXX-DEBUG-FIXME tests in master at the end of this whole process.
  • Remove the LIBCXX-DEBUG-FIXME feature from Generic-debug-iterators.cmake.

To run llvm-lit manually from the command line:

    
./bin/llvm-lit -sv --param std=c++2b --param cxx_under_test=`pwd`/bin/clang \
    --param debug_level=1 ../libcxx/test/

Diff Detail

Event Timeline

Quuxplusone created this revision.Tue, Apr 20, 10:01 AM
Quuxplusone requested review of this revision.Tue, Apr 20, 10:01 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Apr 20, 10:01 AM

Nice to see another improvement to our build pipeline, thanks for working on it! The general approach looks good to me.

libcxx/src/debug.cpp
441 ↗(On Diff #338909)

This seems trivial, but I think it should land in a separate commit. (I don't mind to do that after accepting this patch.)
I assume this is what you mean with "Land the fix to support N3644 "Null Forward Iterators" in debug mode." correct?

Just curious how did you discover this?

libcxx/utils/ci/buildkite-pipeline.yml
127–140

I'm not to fond of "Debug mode", it sounds too generic for me. How do you feel about "Debug iterators"?

Quuxplusone marked an inline comment as done.

s/Debug Mode/Debug iterators/
Fix quoting in CMake file (I hope).
Remove N3644 fix to see how many tests it affects.

[libc++] [test] Stop using invalid iterators to insert into sets/maps. (Applies 4c80bfbd53caf consistently across associative and unord tests.)
[libc++] std::advance shouldn't use ADL >= on the _Distance type.

[libc++] [test] Our __debug_less breaks some complexity guarantees.
[libc++] [test] Iterating a string::iterator "off the end" is UB.
[libc++] Paper over a bug in string::assign.
[libc++] <span>, like <string_view>, has no use for debug iterators.
[libc++] Constexpr char_traits::copy mustn't compare unrelated pointers.

Quuxplusone added inline comments.Tue, Apr 20, 7:37 PM
libcxx/src/debug.cpp
441 ↗(On Diff #338909)

It all started with D100029. :) That led to looking at _LIBCPP_DEBUG == 2 assertions, which led to looking at all code under #if _LIBCPP_DEBUG == 2 (and incidentally D100737 too), which led to D100730, which revealed that <span> was completely broken in debug mode, we just didn't have any tests specifically for span-in-debug-mode. So, since the apparent intent is that everything should just work in debug mode... let's test it!

Running tests locally with -D_LIBCPP_DEBUG=1 turned up some failures due to N3644 immediately, so it just happened that I fixed this line before finishing the buildkite-related parts of the patch. Then I ran tests locally and marked the remaining offenders with XFAIL; but, having already fixed N3644, I didn't actually know how many tests are affected by it — just "at least one."

But now I know, and have filed D100881! :)

libcxx/utils/libcxx/test/config.py
484 ↗(On Diff #339077)

I still haven't figured out why https://buildkite.com/llvm-project/libcxx-ci/builds/2676#7b1ae675-f9cd-4f9d-8b4a-6db9ec8f8e53 is so unhappy with this script now...

curdeius added inline comments.Wed, Apr 21, 6:59 AM
libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_hint_const_lvalue.pass.cpp
38 ↗(On Diff #339077)

What's the reason for this and similar changes?

libcxx/utils/libcxx/test/config.py
484 ↗(On Diff #339077)

@Quuxplusone, I have a similar failure when cmake chooses gcc 9 (without -std=c++2b flag), but it doesn't happen with a newer compiler nor with std=c++20.

Quuxplusone marked an inline comment as done.

Try changing Generic-debug.cmake as

 set(LIBCXX_TEST_PARAMS "std=c++2b" "debug_level=1" "additional_features=LIBCXX-DEBUG-FIXME" CACHE STRING "")
-set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
+set(LIBCXXABI_TEST_PARAMS "std=c++2b" CACHE STRING "")

to see if that fixes the cryptic failure.

Keep trying to solve this "flag None" issue in the Python.

Figured out how to run this locally, and I think I've diagnosed the problem: If you set the lit parameter std=c++2b (not via llvm-lit --param but via the weird cmake technique used in the buildbot scripts), and the system compiler doesn't support -std=c++2b, then you get this error:

AssertionError: Trying to enable compile flag None, which is not supported

Changing to std=c++20 fixes it locally for me. I'm hoping that changing the compiler to clang-tot (copied from the generic-cxx2b section of the script) will fix it just as well on buildkite, without needing to abandon C++2b. If this still fails on buildkite, my backup plan is to change it to std=c++20 for now.

libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_hint_const_lvalue.pass.cpp
38 ↗(On Diff #339077)

[libc++] [test] Stop using invalid iterators to insert into sets/maps. (Applies 4c80bfbd53caf consistently across associative and unord tests.)

[libc++] [test] Stop using invalid iterators to insert into sets/maps.

This simply applies Howard's commit 4c80bfbd53caf consistently
across all the associative and unordered container tests.

"unord.set/insert_hint_const_lvalue.pass.cpp" failed with `-D_LIBCPP_DEBUG=1`
before this patch; it was the only one that incorrectly reused
invalid iterator `e`. The others already used valid iterators
(generally `c.end()`); I'm just making them all match the same pattern
of usage: "e, then r, then c.end() for the rest."
libcxx/test/std/containers/unord/unord.set/insert_hint_const_lvalue.pass.cpp
38

(@curdeius : Here's the one where the test was actually using an invalidated iterator e.)

Eliminate all my fixes from this review, now that buildkite is happy. Now this review consists only of what will be committed as a single commit, following my original "roadmap" idea. Tests that fail at the moment are marked XFAIL; I'll submit PRs to fix them individually.

Quuxplusone retitled this revision from [WIP] Add a buildkite for _LIBCPP_DEBUG=1. to Add a buildkite for _LIBCPP_DEBUG=1..Wed, Apr 21, 10:01 AM
Quuxplusone edited the summary of this revision. (Show Details)

Eliminate all my fixes from this review, now that buildkite is happy. Now this review consists only of what will be committed as a single commit, following my original "roadmap" idea. Tests that fail at the moment are marked XFAIL; I'll submit PRs to fix them individually.

Seems you forgot to restore the removed builds from libcxx/utils/ci/buildkite-pipeline.yml
Can you update this file to give the patch a full CI run.

libcxx/src/debug.cpp
441 ↗(On Diff #338909)

Thanks for the information and the new patch.

ldionne requested changes to this revision.Wed, Apr 21, 2:59 PM

This is indeed quite nice! Some comments, but I love that we're adding this.

libcxx/cmake/caches/Generic-debug.cmake
1 ↗(On Diff #339288)

This shouldn't set std=c++2b explicitly, I believe we should be using the default here.

libcxx/utils/ci/run-buildbot
521

Should be clang and clang++, like the other jobs?

Also, please move close to the generic-no-debug) step.

libcxx/utils/libcxx/test/config.py
484 ↗(On Diff #339288)
This revision now requires changes to proceed.Wed, Apr 21, 2:59 PM
Quuxplusone marked an inline comment as done.Wed, Apr 21, 3:19 PM
Quuxplusone added inline comments.
libcxx/cmake/caches/Generic-debug.cmake
1 ↗(On Diff #339288)

I want to make sure that we're testing new features as soon as they're pull-requested; testing only C++14 mode wouldn't have caught the issues with <span>. Of course we could add debug-mode testing for multiple versions, like std=c++03 AND std=c++2b and maybe something in between; I thought that was overkill, but I'd do it if you want it.

I don't think we want to be testing only C++14, though; I think we want at least C++20, and I see only upsides to testing C++2b (given that the goal here is to catch things at pull-request time, and people tend to pull-request mostly features that are enabled in '20 and '2b, not '14 anymore).

How about if I provide just generic-cxx03-debug and generic-cxx2b-debug, with those names? (This also helps to indicate that they're not the opposite of generic-no-debug, as brought up below.)

libcxx/utils/ci/run-buildbot
521

(A) clang-tot turned out to be the magic incantation that makes the std=c++2b parameter work. Compare to the generic-cxx2b) case. (If we stopped trying to test 2b pull requests and went to 20 or earlier, then this could indeed be just clang.)

(B) Will do. But notice that generic-no-debug is actually talking about a build configuration that doesn't build debug.cpp into the library; it's not the "opposite" of generic-debug. All of generic-no-{debug,filesystem,localization,random_device} have the same structure of "normal compilation, just don't link in some .cpp files to the library."

libcxx/utils/libcxx/test/config.py
484 ↗(On Diff #339288)

I'll do so. But actually I'm pretty sure that all the trouble I had was completely due to writing clang/std=c++2b instead of (either clang-tot/std=c++2b or clang/std=c++20) above.

Jobs for generic-cxx03-debug and generic-cxx2b-debug.
Rebase.

Quuxplusone edited the summary of this revision. (Show Details)Wed, Apr 21, 3:45 PM

Rebase, and change XFAIL to UNSUPPORTED.
(Many of these tests pass in C++03 mode but fail in C++11 mode, so XFAIL is wrong.)

ldionne requested changes to this revision.Mon, Apr 26, 9:55 AM

@Quuxplusone

I want to make sure that we're testing new features as soon as they're pull-requested; testing only C++14 mode wouldn't have caught the issues with <span>. Of course we could add debug-mode testing for multiple versions, like std=c++03 AND std=c++2b and maybe something in between; I thought that was overkill, but I'd do it if you want it.

We agree that testing only C++14 is bad by default, but that's not what we do. By default, the Lit configuration will test the latest supported standard anyway. I think we should have a single job with the debug mode enabled, and that job should *not* mention what standard version it's using (it should use the default one).

This revision now requires changes to proceed.Mon, Apr 26, 9:55 AM
Quuxplusone edited the summary of this revision. (Show Details)

Remove generic-cxx03-debug.
Rename generic-cxx2b-debug to generic-debug-iterators.
Use "the highest supported version" instead of hard-coding std=c++2b ... But leave this build using clang-tot instead of plain old clang, so that our "highest supported version" actually is c++2b, instead of c++20.

Poke buildkite.
@ldionne ping! (assuming buildkite is still happy with this)

ldionne accepted this revision.Fri, Apr 30, 2:19 PM

LGTM and I don't want to be unproductive so I'll let it slip, but it still seems to me that it would be more consistent to use

export CC=clang
export CXX=clang++

And if we really want to be using ToT Clang here, then perhaps we should consider using ToT everywhere instead, and adding a single job that tests on an "older" compiler, as a means of testing that we still support those older compilers. Actually, after writing that, I think it would make sense to do it as part of the patch that will explain our compiler support policy, which I still need to write.

This revision is now accepted and ready to land.Fri, Apr 30, 2:19 PM
This revision was automatically updated to reflect the committed changes.