This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] Properly construct containers in debug mode tests for map/set
ClosedPublic

Authored by krisb on Apr 7 2021, 3:30 AM.

Details

Summary

The debug mode tests for unordered map/set iterators construct empty containers,
making the code after the first increment meaningless.
It's never executed since the tests exit earlier.

It doesn't seem to be intentional, so the patch makes the tests to construct
containers that include an element.

Diff Detail

Event Timeline

krisb requested review of this revision.Apr 7 2021, 3:30 AM
krisb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2021, 3:30 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
krisb updated this revision to Diff 336067.Apr 8 2021, 5:18 AM

Make it compiled with c++03.

curdeius requested changes to this revision.Apr 8 2021, 6:21 AM
curdeius added a subscriber: curdeius.

Very good catch indeed. Thanks.

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
32–33

In order not to repeat this error, could you add assert(i != c.end(c.bucket(1))); before and assert(i == c.end(c.bucket(1))); after the first increment, please?
And add assert(i != c.end()); for non-local iterators.
Please factor out c.bucket(1) too.

And a nit, 1 can be considered as a (bucket) index if read hastily. Please use some magic value, 42 or whatever, but not 0 nor 1.

This revision now requires changes to proceed.Apr 8 2021, 6:21 AM
Quuxplusone added inline comments.
libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp
30–33

This PR seems like a step forward (especially once @curdeius' comments are addressed), but even after taking curdeius' comments and making the other tests match this one, don't we still have the same problem that if any libc++ assert ever fails, the test succeeds? So again here, if the ++i on line 29 assert-fails (incorrectly), then the rest of the test doesn't run and the test appears to pass (incorrectly).

Scope creep!; but I wonder if these tests should be rewritten to use something like #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : foo(m)) and then define a foo that (1) checks for the exact message m that we expect, and (2) checks a global boolean that we set immediately before the operation that we expect to fail, so that "early failures" don't (incorrectly) count as successes.

curdeius added inline comments.Apr 8 2021, 9:33 AM
libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp
30–33

+1. Good remark.

krisb updated this revision to Diff 336319.Apr 8 2021, 11:19 PM
krisb edited the summary of this revision. (Show Details)

Address @curdeius' comments.

krisb updated this revision to Diff 336327.Apr 9 2021, 12:28 AM

Simplify the changes a bit.

krisb marked an inline comment as done.Apr 9 2021, 12:31 AM

@curdeius thank you for looking at this!

libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp
30–33

@Quuxplusone good point, thank you! I'll take a look.

curdeius added inline comments.Apr 9 2021, 12:38 AM
libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
16

To answer @Quuxplusone's point 2), you can do just something like the suggested edits here... (see another comment for the rest)

Comparing the assertion messages may be a bit difficult though, and brittle.

I.e. if we wanted to do something like:

#include <assert.h>
#include <stdlib.h>
#include <string.h>

void test_assertion(const char* msg) {
    assert(::strcmp(msg, "expected") == 0);
    ::exit(::test_exit_code);
}

#define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : ::test_assertion(m))

we hit the problem that we include some headers for assert, exit and strcmp, but we redefine _LIBCPP_ASSERT afterwards.
Also these headers might get included transitively by later includes (but won't be because of header guards), but they'll have _LIBCPP_ASSERT not redefined.

An orthogonal problem is to have the actual message when assert(strcmp(...)...); fails.

34

... and then here.

krisb added inline comments.Apr 12 2021, 2:23 AM
libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
16

we hit the problem that we include some headers for assert, exit and strcmp, but we redefine _LIBCPP_ASSERT afterwards

It doesn't seem like we need to care about assert.h, stdlib.h and other 'C'-headers here, since _LIBCPP_ASSERT isn't expected to appear in them and any includes.

So for the particular test it may look like:

#include <string.h>
#include <cassert>
int test_exit_code = 1;
#define _LIBCPP_ASSERT(x, m)                                                   \
  ((x) ? (void)0                                                               \
       : ((::strcmp(m, "Attempted to increment non-incrementable unordered "   \
                       "container local_iterator") == 0)                       \
              ? std::exit(::test_exit_code)                                    \
              : assert(x)))

and later in the test

int main(int, char**) {
    ...
    test_exit_code = 0;
    ++i; /* _LIBCPP_ASSERT is expected to fail here */
    ...

I'll test the approach and create a separate review for it.

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
16

@krisb: Yes, that's basically what I'm asking for, except please put it in a helper function so you don't have to string all the side effects into one giant expression. E.g. this: https://godbolt.org/z/nYn1ohEen

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
16

libcxx/test/libcxx/strings/basic.string/string.modifiers/erase_iter_db1.pass.cpp is similarly bogus. @krisb, do you have any interest in including that one in this patch as well? or making a followup patch for all such tests?

krisb added inline comments.Apr 13 2021, 12:42 AM
libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
16

except please put it in a helper function so you don't have to string all the side effects into one giant expression.

Despite the macro looks a bit monstrous, it has one advantage that seems important to me. Doing the checks in such a way makes us possible to report the original failure message from unexpected asserts. This seems valuable. So, honestly, I'd prefer one giant expression with clear error messages than an easy-to-read helper function that is hard to understand why it failed.

do you have any interest in including that one in this patch as well? or making a followup patch for all such tests?

Yes, I'm going to update _LIBCPP_ASSERT in all the debug mode tests and fix the issues it reveals.

LGTM. As I now understand the current situation: D100029 now does not change anything about _LIBCPP_ASSERT nor test/support/.
It's just changing these tests' main functions to make sure they operate on non-empty containers so that they don't prematurely hit an assert and exit "successfully failed."
I think we should land this so we can move on to D100595.
@curdeius?

This revision is now accepted and ready to land.Apr 19 2021, 10:57 PM