Page MenuHomePhabricator

[libcxx][test] Attempt to make debug mode tests more bulletproof
ClosedPublic

Authored by krisb on Apr 15 2021, 12:39 PM.

Details

Summary

The problem with debug mode tests is that it isn't known which particular
_LIBCPP_ASSERT causes the test to exit, and as shown by
https://reviews.llvm.org/D100029 and 2908eb20ba7 it might be not the
expected one.

This patch implements the appriach suggested by @Quuxplusone in D100029
that allows us to cautch only those _LIBCPP_ASSERTs that are going to be
tested; all others will produce an error.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Quuxplusone added inline comments.Apr 15 2021, 2:36 PM
libcxx/test/libcxx/strings/basic.string/string.iterators/db_iterators_4.pass.cpp
27–28

You forgot to remove the assert here. (Grep XFAIL(assert for more instances.)

libcxx/test/support/debug_macros.h
13

Even better: static bool.
And it'd be good to name it something that connotes testiness and the fact that it's related to TEST_LIBCPP_ASSERT.
Finally, I suggest that this should actually be a const char * that we set to the expected message; that way, we don't need to redefine _LIBCPP_ASSERT differently in each different test file.

// copyright header
// header guard for "replace_libcpp_assert.h"

// This file must be included before any other files, even "test_macros.h",
// since it affects the contents of <__debug>.

static const char *test_expect_libcpp_assert = 0;
#define _LIBCPP_ASSERT(x, m) do { \
    if (!test_expect_libcpp_assert) { \
      _LIBCPP_ASSERT_IMPL(x, m); \
    } else if (x) { \
    } else if (std::strcmp(m, test_expect_libcpp_assert) == 0) { \
      std::exit(0); \
    } else { \
      _LIBCPP_ASSERT_IMPL(false, m); \
    } \
  } while (0)

Used like this:

// copyright header
#include "replace_libcpp_assert.h"
#include "test_macros.h"
~~~

int main(~~~) {
  C::iterator i = c.begin();
  ++i;
  assert(i == c.end());
  test_expect_libcpp_assert = "Attempted to increment non-incrementable iterator";
  ++i;
  assert(false);
}
17–20

I'm concerned that any variation on XFAIL is too easy to confuse with the notion of an XFAILed test case. Personally I would rather just see these three lines written out in each test case. I show above my suggestion on how to implement that.

Quuxplusone added inline comments.Apr 15 2021, 2:41 PM
libcxx/test/libcxx/containers/sequences/list/list.ops/db_splice_pos_list_iter_iter.pass.cpp
16

IMHO yes. It seems to be a very straightforwardly missing assertion, circa line 2099 of <list>.

krisb added a subscriber: EricWF.Apr 17 2021, 1:59 PM

Thank you all for your comments! But before proceeding, I want to ask a question.
It seems we have another approach for debug mode tests that was added in D59166, which I didn't notice before but which looks much better than we have now (except the facts that it is more complicated, has more requirements, and may suffer from the same problem because most tests don't check for the exact assertion just the fact that an assertion happened). So, the question is why the approach from D59166 didn't replace other (I mean the tests which this patch about) debug mode tests? Was the reason in the requirements? Should it finally become the only approach for debug mode tests or it is supposed to coexist with others?

Add @EricWF to help with this.

krisb updated this revision to Diff 338386.Apr 18 2021, 10:29 AM
krisb marked 6 inline comments as done.

Address comments, switch to setting debug function instead of redefining _LIBCPP_ASSERT.

libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
19 ↗(On Diff #337874)

As @curdeius correctly mention below these tests are not expected to fail, so basically we do not need to redefine _LIBCPP_ASSERT for them at all. I'll clean this up (forgot about this).

libcxx/test/support/debug_macros.h
13

If to keep these tests in at least as a short-term solution, it may make sense to reuse the logic we already have in __debug/debug.cpp. I didn't notice it early, but it seems to be the best way to handle things. I'll attempt to implement an approach in the next update.

krisb updated this revision to Diff 338535.Apr 19 2021, 8:47 AM

Rebase on main + D100029 + D100592, remove unused typedefs, update after D100728.

Rebase on main + D100029 + D100592, remove unused typedefs, update after D100728

My impression was that D100029 was obsolete and should be abandoned, with any remaining bits rolled into D100595.
D100592 looks ready to land whenever you feel like it; I hope it's not blocked on D100029 (and if it is, I suggest you figure out how to extricate it from that dependency).

ldionne added inline comments.Apr 19 2021, 11:21 AM
libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
21 ↗(On Diff #337874)

Now this makes more sense to me - we just don't want to overwrite the _LIBCPP_ASSERT macro.

However, shouldn't we now rename this test since it's not related to the libc++ debug mode anymore? And while we're at it, maybe we don't need this test anymore if it's already tested in the tests for std::list?

krisb added a comment.Apr 19 2021, 3:39 PM

Rebase on main + D100029 + D100592, remove unused typedefs, update after D100728

My impression was that D100029 was obsolete and should be abandoned, with any remaining bits rolled into D100595.
D100592 looks ready to land whenever you feel like it; I hope it's not blocked on D100029 (and if it is, I suggest you figure out how to extricate it from that dependency).

That's not quite right. D100029 is still actual, as it ensures that the tested containers have at least one element, which seems to be assumed when the tests were written. Otherwise, we would need to adjust the tests to check against empty containers.
D100592 isn't blocked by anything, so I'll land it once the pre-commit tests pass.

libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
21 ↗(On Diff #337874)

It doesn't seem like we have some normal mode tests that check that iterators keep valid after moving. But I think it can still be valuable in debug mode (I mean with -D_LIBCPP_DEBUG=1 defined) as it also checks that debug mode correctly handles cases like this and doesn't trigger any assertions (which would be triggered, for example, if there were two different containers). Does it make sense?

curdeius added inline comments.Apr 20 2021, 4:16 AM
libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
21 ↗(On Diff #337874)

I think it's important to keep these tests with -D_LIBCPP_DEBUG=1/2 as @krisb indicated because they should pass in debug mode. However, as their purpose isn't clear, it would be wise to add a comment.

krisb updated this revision to Diff 340431.Apr 25 2021, 9:50 PM
krisb marked 2 inline comments as done.

Remove two more unused typedefs.

krisb added inline comments.Apr 25 2021, 10:04 PM
libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
21 ↗(On Diff #337874)

Well, from the perspective of D100866 it looks okay to switch these tests from debug to normal mode, so that 'debug mode tests' are assumed to be only negative tests. But if this is acceptable, I'd prefer to do this in a follow-up patch.

Quuxplusone added inline comments.Apr 26 2021, 10:50 AM
libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
21 ↗(On Diff #337874)

I agree: once D100866 lands, these tests won't really have anything to do with a "debug mode." They simply test a property of the move-constructor of std::list (namely iterator non-invalidation), which is mandated by the Standard to work. And then the new buildkite job in D100866 will verify that this property holds both in regular mode and in debug mode.

This test should be removed, and we should add a legitimate test for iterator (and pointer) non-invalidation in-or-alongside these files:

  • libcxx/test/std/containers/sequences/list/list.cons/move.pass.cpp
  • libcxx/test/std/containers/sequences/list/list.special/swap.pass.cpp

I'm fine with saying that we can do that in a separate PR. However, honestly, I think it'd be good to submit that PR now, get it merged now, and then after that we can update this PR to simply delete this file.
If you really really want to defer that for later, then OK but please add a FIXME comment in this file explaining the situation and pointing at move.pass.cpp and swap.pass.cpp, so that we physically cannot forget about this.

krisb updated this revision to Diff 341325.Apr 28 2021, 2:57 PM

Add FIXMEs to a few db_move.pass.cpp tests to not forget to convert them to regular mode.

krisb added inline comments.Apr 28 2021, 2:59 PM
libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
21 ↗(On Diff #337874)

Since we have a couple more similar tests that should be checked and converted to regular mode, I'd prefer to do this after D100866 gets landed, so we will be able to run new tests in both regular and debug modes with ease.
Having this, I'd not block this patch by D100866 and focus on issues in debug mode tests first. So, if you okay with this, I'd prefer to add FIXMEs and proceed with other changes from this patch.

ldionne accepted this revision.Apr 30 2021, 2:32 PM

It seems we have another approach for debug mode tests that was added in D59166, which I didn't notice before but which looks much better than we have now (except the facts that it is more complicated, has more requirements, and may suffer from the same problem because most tests don't check for the exact assertion just the fact that an assertion happened). So, the question is why the approach from D59166 didn't replace other (I mean the tests which this patch about) debug mode tests? Was the reason in the requirements? Should it finally become the only approach for debug mode tests or it is supposed to coexist with others?

I think it's just that the work was not finished. I'm fine with this patch as-is, I think it's an improvement. If you want to convert everything to D59166 style, feel free to do it and you'll have my support too.

@Quuxplusone Please approve once you're ok with that, and the author can ship-it then.

LGTM % comments, but I'd like to
(A) wait until either-Kris-or-I have split off the tests-that-really-aren't-debug-mode-tests into a separate PR, and also
(B) see it updated to search-and-replace EXPECTED_FAIL into ASSERT_FAILS_WITH_MESSAGE (or something nicer, if we can find something nicer) and pass buildkite again

libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
13–15 ↗(On Diff #341325)

@krisb: D100866 has landed. Either you or I should make a separate PR for adding these tests as regular-mode tests. (I'm happy to do it. I should probably just do it.) I'd like to do that before landing this one, so that this one can be smaller.

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_9.pass.cpp
35

I notice several of these messages are inconsistently missing the word "a" — "...increment a non-incrementable...". Should we fix them? I say yes.

Orthogonally, I notice there are no matching tests for --i "Attempted to decrement..." Do we care?

libcxx/test/libcxx/containers/unord/unord.set/db_insert_hint_const_lvalue.pass.cpp
29–32

Nit: I would prefer not to break the string literal, so that it's easier to grep for. Break the line as

EXPECTED_FAIL(c.insert(e, v),
    "unordered_set::insert(const_iterator, const value_type&) called with an iterator not referring to this unordered_set");

and ignore clang-format's griping.

libcxx/test/support/debug_macros.h
26–32

I think the only remaining controversial point is what to name this macro. I don't really like EXPECTED_FAIL (in that it doesn't start with either ASSERT_ or TEST_, and it still reminds me of XFAIL, and technically it's a reserved identifier because <errno.h>).
I suggest ASSERT_FAILS_WITH_MESSAGE(expr, m) — what do you think of that name, @krisb?

krisb added a comment.May 3 2021, 11:58 AM

It seems we have another approach for debug mode tests that was added in D59166, which I didn't notice before but which looks much better than we have now (except the facts that it is more complicated, has more requirements, and may suffer from the same problem because most tests don't check for the exact assertion just the fact that an assertion happened). So, the question is why the approach from D59166 didn't replace other (I mean the tests which this patch about) debug mode tests? Was the reason in the requirements? Should it finally become the only approach for debug mode tests or it is supposed to coexist with others?

I think it's just that the work was not finished. I'm fine with this patch as-is, I think it's an improvement. If you want to convert everything to D59166 style, feel free to do it and you'll have my support too.

I'm still looking at D59166 but despite it's a great way to avoid implementing and maintaining dozens (maybe finally we need hundreds) debug tests, it doesn't look like a convenient way to test debug mode on different platforms, especially exotic ones. I'm thinking about something like a test generator that would create individual test files by a given template; it allows keeping the tests small and easy to understand. But I need more time to investigate options.

libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp
13–15 ↗(On Diff #341325)

Great! Do you want me to take care of these then or prefer to do it by yourself?

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_9.pass.cpp
35

Good points, thank you!

I notice several of these messages are inconsistently missing the word "a" — "...increment a non-incrementable...". Should we fix them? I say yes.

Yes, and it is definitely better to do before this patch. Are you going to take care of this or want me to fix them?

Orthogonally, I notice there are no matching tests for --i "Attempted to decrement..." Do we care?

I think yes, it would be good to have such tests. Once this patch is landed I'll prepare a follow-up.

libcxx/test/support/debug_macros.h
26–32

ASSERT_FAILS_WITH_MESSAGE(expr, m) is okay to me, but I'd still prefer to start it with EXPECT/EXPECTED. Maybe we can find something better? Here are some options that came to my mind:

EXPECT_ASSERT_FAIL
EXPECT_LIBCPP_ASSERT_FAIL
EXPECT_ASSERT_FAIL_WITH_MESSAGE

EXPECT_DEATH_WITH_MESSAGE
EXPECT_EXIT_WITH_MESSAGE

EXPECTED_ERROR

LIBCPP_ASSERT_FAIL
LIBCPP_ASSERT_FAIL_WITH_MESSAGE

If nothing looks good to you, I'll stay with your variant.

Quuxplusone added inline comments.May 3 2021, 12:03 PM
libcxx/test/support/debug_macros.h
26–32

I specifically want something that starts either with ASSERT_ or TEST_, for consistency with the other testing macros.
I do see how it's awkward that we want to express that we're "asserting" that "_LIBCPP_ASSERT" "assert-fails" — lots of potential repetition in there, that we're both trying to avoid somehow.

krisb added a comment.May 6 2021, 3:33 AM

@Quuxplusone , just want to ask, are you going to do smth related to this patch, or prefer me to take care of all the dependencies (like missed articles and db_move tests issue)? Just want to avoid doing the same things twice.

libcxx/test/support/debug_macros.h
26–32

What about something like

TEST_EXIT_WITH_MESSAGE
TEST_ASSERT_FAILURE
TEST_LIBCPP_ASSERT_FAILURE

then?

just want to ask, are you going to do smth related to this patch, or prefer me to take care of all the dependencies

Yes, sorry, it's on my to-do list. If we're lucky I'll get to it today. :)

libcxx/test/support/debug_macros.h
26–32

TEST_LIBCPP_ASSERT_FAILURE would be OK by me.
(The competitor is ASSERT_FAILS_WITH_MESSAGE. Either of these two is fine with me.)

krisb updated this revision to Diff 345079.May 13 2021, 3:12 AM

Rebased on the top of main.
Extended the approach on all the debug mode tests.
Renamed EXPECTED_FAIL -> TEST_LIBCPP_ASSERT_FAILURE.

krisb updated this revision to Diff 345258.May 13 2021, 12:45 PM

Restart tests

These tests sure are tedious. :) I may have more comments later, but I figure I should post these for now.

libcxx/test/libcxx/containers/sequences/list/list.modifiers/emplace_db1.pass.cpp
22–28

Could we replace this whole bit with

struct A {
    explicit A(int i, double d) {}
};
libcxx/test/libcxx/containers/sequences/list/list.modifiers/erase_iter_iter_db1.pass.cpp
27–28

std::next (and ditto throughout)

libcxx/test/libcxx/containers/sequences/list/list.modifiers/insert_iter_iter_iter_db1.pass.cpp
28–31

Let's make this simply

TEST_LIBCPP_ASSERT_FAILURE(v.insert(v2.cbegin(), a, a+5),
    "list::insert(iterator, range) called with an iterator not referring to this list");

(and remove test_iterators.h)
I see no reason for std::next, N, or cpp17_input_iterator to be involved here.

libcxx/test/libcxx/containers/sequences/list/list.modifiers/insert_iter_size_value_db1.pass.cpp
26–28

Check me on this, but I'm pretty sure this is testing the wrong thing. It looks like it should be more like

c1.insert(c2.cbegin(), 5, 1)

with a message about an iterator not referring to this list.

libcxx/test/libcxx/containers/sequences/list/list.modifiers/pop_back_db1.pass.cpp
33

Now that we run the regular tests in debug mode (so that non-failing pop_backs are already well tested), we can simplify this test to just

int main(int, char**)
{
    std::list<int> c;
    TEST_LIBCPP_ASSERT_FAILURE(c.pop_back(), "list::pop_back() called on an empty list");

    return 0;
}
libcxx/test/libcxx/containers/sequences/list/list.ops/db_splice_pos_list_iter.pass.cpp
29

FWIW, the error message here is still missing an article: "referring to the list argument"?
Ditto in "db_splice_pos_list_iter_iter.pass.cpp".

libcxx/test/libcxx/containers/sequences/vector/db_back.pass.cpp
24–29

This can be just

int main(int, char**)
{
    std::vector<int> c;
    TEST_LIBCPP_ASSERT_FAILURE(c.back(), "back() called on an empty vector");

    return 0;
}

Please make the substitution throughout this directory, anywhere you see typedef int T; being used trivially.

libcxx/test/libcxx/containers/sequences/vector/db_back_2.pass.cpp
24–30

This test seems the same as the previous...? Ah, I see, this one uses vector<int, min_allocator<int>> instead of vector<int>. However, IMHO we can just kill this test; it's not significantly different from the previous one. I recommend

git rm db_back_2.pass.cpp
git rm db_cback_2.pass.cpp
git rm db_front_2.pass.cpp
git rm db_cfront_2.pass.cpp
git rm db_index_2.pass.cpp
git rm db_cindex_2.pass.cpp
libcxx/test/libcxx/containers/sequences/vector/db_front.pass.cpp
23–29

As above:

int main(int, char**)
{
    std::vector<int> c;
    TEST_LIBCPP_ASSERT_FAILURE(c.front(), "front() called on an empty vector");

    return 0;
}
libcxx/test/libcxx/containers/sequences/vector/db_index.pass.cpp
28

On these operator[] tests, I'd prefer a non-empty vector: more like

int main(int, char**)
{
    std::vector<int> c(5);
    TEST_LIBCPP_ASSERT_FAILURE(c[5], "vector[] index out of bounds");

    return 0;
}
libcxx/test/libcxx/containers/unord/unord.multimap/db_swap_1.pass.cpp
36–39

There's a couple of things wrong here IMHO:

  • c2.erase(i1) invalidates iterator i1, so even trying to copy its value into j seems sketchy.
  • j is unused.
  • c1.erase(i1) uses an invalid iterator; I think it should use a valid iterator.

These issues could all be addressed by doing this:

int main(int, char**)
{
    typedef std::pair<int, int> P;
    P a1[] = {P(1, 1), P(3, 3), P(7, 7), P(9, 9), P(10, 10)};
    P a2[] = {P(0, 0), P(2, 2), P(4, 4), P(5, 5), P(6, 6), P(8, 8), P(11, 11)};
    std::unordered_multimap<int, int> c1(a1, a1+5);
    std::unordered_multimap<int, int> c2(a2, a2+7);
    std::unordered_multimap<int, int>::iterator i1 = c1.begin();
    std::unordered_multimap<int, int>::iterator i2 = c2.begin();
    swap(c1, c2);
    c1.erase(i2);
    TEST_LIBCPP_ASSERT_FAILURE(c1.erase(i1),
        "unordered container erase(iterator) called with an iterator not referring to this container");

    return 0;
}

Ditto on the other unordered containers' db_swap_1 tests.

libcxx/test/libcxx/utilities/optional/optional.object/optional.object.observe/dereference_const_rvalue.pass.cpp
23–24

const std::optional<int> opt = std::nullopt;
Judging from the filename, the const should be there.

krisb updated this revision to Diff 345808.May 17 2021, 3:11 AM
krisb marked 8 inline comments as done.

Addressed review comments.

krisb added inline comments.May 17 2021, 3:14 AM
libcxx/test/libcxx/containers/sequences/list/list.modifiers/insert_iter_size_value_db1.pass.cpp
26–28

Agreed

libcxx/test/libcxx/containers/sequences/list/list.modifiers/pop_back_db1.pass.cpp
33

I'd leave this out of the scope of the patch. This patch is already large enough, so I'm not sure this is a good idea to include more NFCs here, especially those that are not closely related to the initial intent of the patch.
Except for the changes for TEST_LIBCPP_ASSERT_FAILURE macro I'd:

  • fix correctness issues,
  • remove redundant includes (since most became redundant because of this patch),
  • simplify whatever within TEST_LIBCPP_ASSERT_FAILURE.

All other improvements/simplifications are good for follow-ups. What do you think?

libcxx/test/libcxx/containers/sequences/vector/db_back_2.pass.cpp
24–30

I'd leave this out of the scope of this patch. We have similar tests for all the containers, and they tested the specific functionality at least at the moment they were added (I haven't checked how things are going now, though).

Quuxplusone accepted this revision.May 17 2021, 8:21 AM
Quuxplusone added inline comments.
libcxx/test/libcxx/containers/sequences/vector/db_back_2.pass.cpp
24–30

OK. I also realize now that min_allocator uses fancy pointers, so that is probably good to have coverage on. :)

libcxx/test/libcxx/utilities/optional/optional.object/optional.object.observe/op_arrow_const.pass.cpp
24

Nit: remove constexpr here

This revision is now accepted and ready to land.May 17 2021, 8:21 AM
krisb updated this revision to Diff 346060.May 18 2021, 12:05 AM
krisb marked 10 inline comments as done.

Rebase & rerun tests.