This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Generalize defines for skipping allocation checks
ClosedPublic

Authored by mstorsjo on Aug 9 2021, 4:28 AM.

Details

Summary

This allows waiving the right amount of asserts on Windows and zOS.
This should supersede D107124 and D105910 (but I haven't had tested
it on zOS).

Some of the new.delete tests end up with lots of conditional asserts
and very little actual testing left intact, but this at least shows
off what needs to be disabled; for those tests I'm personally ok
with adding UNSUPPORTED lines instead.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Aug 9 2021, 4:28 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2021, 4:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: ldionne.Aug 9 2021, 6:00 AM

This LGTM, please wait for CI to finish.

ldionne accepted this revision.Aug 9 2021, 6:00 AM
This revision is now accepted and ready to land.Aug 9 2021, 6:00 AM
Quuxplusone requested changes to this revision.Aug 9 2021, 7:00 AM
Quuxplusone added inline comments.
libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp
153–155

I would find this easier to reason about if it were like

RequireAllocationGuard g(0);  // require "at least zero" allocations by default
#if TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS
if (DisableAllocations) {
    g.requireExactly(0);
}
#endif // TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS
LHS += RHS;

I'm not thrilled by the macro name WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS for two reasons: (1) no ASSERT or TEST prefix; (2) the associated assert macro is ASSERT_WITH, not ASSERT_WITHOUT.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
65–68

You shouldn't need to alter these lines.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
65–68

You shouldn't need to alter these lines.

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp
149–150

Could we change the "no-op" version of the macro to

#define ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(...) (void)(__VA_ARGS__)

so that line 149 won't be necessary?

libcxx/test/support/test_macros.h
417

This PR currently doesn't demonstrate any uses of ASSERT_WITH_OPERATOR_NEW_FALLBACKS.

I think you should go ahead and merge D107124 and D105910's changes into this PR, and then ask @NancyWang2222 and @DanielMcIntosh-IBM to download your patch and test it locally.

This revision now requires changes to proceed.Aug 9 2021, 7:00 AM
mstorsjo added inline comments.Aug 9 2021, 7:48 AM
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
65–68

One has to; as the overridden operator new above wasn’t called, Buff is still null, and new_called is zero.

libcxx/test/support/test_macros.h
417

Oh, oops. The patch does cover the tests for both of them, but I accidentally used the wrong macro for the new/delete cases. I’ll revise the patch in 4-5 hours.

Let me know if this is ready, I will need to test it on z/OS.

mstorsjo added inline comments.Aug 9 2021, 9:09 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp
153–155

Thanks, that does indeed make it less obfuscated!

Unfortunately, it requires moving the DisableAllocations variable into an ifdef, but I think the end result still is better.

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp
149–150

Nice, yes, that does reduce the clutter.

mstorsjo updated this revision to Diff 365200.Aug 9 2021, 9:11 AM

Applied the suggestions; removed the WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS define, using ASSERT_WITH_OPERATOR_NEW_FALLBACKS in the new/delete tests.

Let me know if this is ready, I will need to test it on z/OS.

I think this should be fine to try on z/OS now.

LGTM if buildkite is happy. I think the DisableAllocations renaming should be done, but if you object, then I don't care to block this over it.

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp
205

As long as we're messing with this, is this a good time to change the name DisableAllocations to ExpectNoAllocations? Here we're not disabling anything from happening; we're just setting up what we expect to see happen.

Could even

bool ExpectNoAllocations = std::is_same<CharT, char>::value && std::is_same<path::value_type, char>::value;

to eliminate that #if _WIN32, right?

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
65–68

Oh, I see, we're calling new through the fallback but we're calling this delete[] directly from line 78. OK then.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
65–68

As above: never mind.

mstorsjo added inline comments.Aug 9 2021, 10:41 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp
205

Oh, good idea about the renaming - yes, that’s much clearer. Regarding merging the windows ifdef that way, I guess that’d work. Semantically there’s a very minor difference, maybe, but it’s close enough to make it worthwhile. I’ll give it a shot.

mstorsjo updated this revision to Diff 365259.Aug 9 2021, 12:24 PM

Renamed DisableAllocations into ExpectNoAllocations, simplified the definition of the variable in path.append.pass.cpp to avoid an ifdef.

NancyWang2222 added a comment.EditedAug 9 2021, 2:16 PM

The changes look ok to me although I havent tried it on z/OS as long as build pass. I will fix in separate patch if there is an issue on z/OS. just want to confirm those assert wont be run on z/OS and assert macros are empty. I will need some time to download patch and test it out.

NancyWang2222 added inline comments.Aug 9 2021, 2:28 PM
libcxx/test/support/test_macros.h
418

those VA_ARGS set 0 on WIN32 only but still set on MVS.

NancyWang2222 added inline comments.Aug 9 2021, 2:33 PM
libcxx/test/support/test_macros.h
418

nvm. I guess it is just checking condition and doesn't trigger assert

mstorsjo added inline comments.Aug 9 2021, 11:59 PM
libcxx/test/support/test_macros.h
418

Yes, it was suggested to do it this way, to avoid warnings about unused variables when the assert isn't used. So now it evaluates the expression, but doesn't assert on it being true.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 10 2021, 1:07 AM
This revision was automatically updated to reflect the committed changes.

hi There is the failing test case libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp , this test case is not in https://reviews.llvm.org/D107124 and https://reviews.llvm.org/D105910 , the failure seems not related to this patch. it fails same without this patch. we will investigate separately. rest of tests in https://reviews.llvm.org/D107124 and https://reviews.llvm.org/D105910 passed. Thanks for the fixes.
Assertion failed: Result == E, file: /plex/wangn/llvm/dev/llvm-project/libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp, line: 250
CEE5207E The signal SIGABRT was received.

hi do you know Why this patch was closed?

hi do you know Why this patch was closed?

I applied it already as it was approved, as I thought you said you can fix any remaining z/OS related details in separate patches later?

hi do you know Why this patch was closed?

I applied it already as it was approved, as I thought you said you can fix any remaining z/OS related details in separate patches later?

Yes. changes are good. I just want to make sure this changes will be landed.