[c2h] Make catch macros work on device#8928
Conversation
c5fc788 to
7da5c27
Compare
| #define CUDAX_REQUIRE(condition) REQUIRE(condition) | ||
|
|
||
| #define CUDAX_CHECK(condition) \ | ||
| NV_IF_ELSE_TARGET(NV_IS_DEVICE, \ | ||
| (cudax_require_impl(condition, #condition, __FILE__, __LINE__, __PRETTY_FUNCTION__);), \ | ||
| (CHECK(condition);)) | ||
| #define CUDAX_CHECK(condition) CHECK(condition) | ||
|
|
||
| #define CUDAX_FAIL(message) /* */ \ | ||
| NV_IF_ELSE_TARGET(NV_IS_DEVICE, /* */ \ | ||
| (cudax_require_impl(false, message, __FILE__, __LINE__, __PRETTY_FUNCTION__);), \ | ||
| (FAIL(message);)) | ||
| #define CUDAX_FAIL(message) FAIL(message) | ||
|
|
||
| #define CUDAX_CHECK_FALSE(condition) CUDAX_CHECK(!(condition)) | ||
| #define CUDAX_CHECK_FALSE(condition) CHECK_FALSE(condition) |
There was a problem hiding this comment.
I will remove these in a separate PR
| #define CUDART(call) REQUIRE((call) == cudaSuccess) | ||
|
|
||
| // There is a problem with clang-cuda and nv/target, but we don't need the device side macros yet, | ||
| // disable them for now | ||
| #if _CCCL_CUDA_COMPILER(CLANG) | ||
| # define CCCLRT_REQUIRE(condition) REQUIRE(condition) | ||
| # define CCCLRT_CHECK(condition) CHECK(condition) | ||
| # define CCCLRT_FAIL(message) FAIL(message) | ||
| # define CCCLRT_CHECK_FALSE(condition) CCCLRT_CHECK(!(condition)) | ||
|
|
||
| #else // _CCCL_CUDA_COMPILER(CLANG) | ||
| # define CCCLRT_REQUIRE(condition) \ | ||
| NV_IF_ELSE_TARGET(NV_IS_DEVICE, \ | ||
| (ccclrt_require_impl(condition, #condition, __FILE__, __LINE__, __PRETTY_FUNCTION__);), \ | ||
| (REQUIRE(condition);)) | ||
|
|
||
| # define CCCLRT_CHECK(condition) \ | ||
| NV_IF_ELSE_TARGET(NV_IS_DEVICE, \ | ||
| (ccclrt_require_impl(condition, #condition, __FILE__, __LINE__, __PRETTY_FUNCTION__);), \ | ||
| (CHECK(condition);)) | ||
|
|
||
| # define CCCLRT_FAIL(message) /* */ \ | ||
| NV_IF_ELSE_TARGET(NV_IS_DEVICE, /* */ \ | ||
| (ccclrt_require_impl(false, message, __FILE__, __LINE__, __PRETTY_FUNCTION__);), \ | ||
| (FAIL(message);)) | ||
|
|
||
| # define CCCLRT_CHECK_FALSE(condition) CCCLRT_CHECK(!(condition)) | ||
| #endif // _CCCL_CUDA_COMPILER(CLANG) | ||
| #define CCCLRT_REQUIRE(condition) REQUIRE(condition) | ||
|
|
||
| #define CCCLRT_CHECK(condition) CHECK(condition) | ||
|
|
||
| #define CCCLRT_FAIL(message) FAIL(message) | ||
|
|
||
| #define CCCLRT_CHECK_FALSE(condition) CHECK_FALSE(condition) | ||
|
|
||
| // Explicit device side require macros for clang-cuda | ||
| #define CCCLRT_REQUIRE_DEVICE(condition) \ | ||
| ccclrt_require_impl(condition, #condition, __FILE__, __LINE__, __PRETTY_FUNCTION__); | ||
| #define CCCLRT_CHECK_DEVICE(condition) \ | ||
| ccclrt_require_impl(condition, #condition, __FILE__, __LINE__, __PRETTY_FUNCTION__); | ||
| #define CCCLRT_FAIL_DEVICE(message) ccclrt_require_impl(false, message, __FILE__, __LINE__, __PRETTY_FUNCTION__); | ||
| #define CCCLRT_CHECK_FALSE_DEVICE(condition) CCCLRT_CHECK_DEVICE(!(condition)) | ||
| #define CCCLRT_REQUIRE_DEVICE(condition) REQUIRE_DEVICE(condition) | ||
| #define CCCLRT_CHECK_DEVICE(condition) CHECK(condition) | ||
| #define CCCLRT_FAIL_DEVICE(message) FAIL(message) | ||
| #define CCCLRT_CHECK_FALSE_DEVICE(condition) CHECK_FALSE(condition) |
| // example-end custom-type | ||
|
|
||
| static __host__ std::ostream& operator<<(std::ostream& os, const custom_t& self) | ||
| __host__ std::ostream& operator<<(std::ostream& os, const custom_t& self) |
There was a problem hiding this comment.
I got warnings about these functions being unused 🤷
There was a problem hiding this comment.
IMO then we should remove them if they really are unused.
There was a problem hiding this comment.
They are used, just not during device pass and that makes cicc complain
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughSummary by CodeRabbit
important: WalkthroughAdds a host/device-compatible Catch2 macro header that implements device-side assertion behavior and aliases, exposes REQUIRE/CHECK/FAIL/TEMPLATE/BDD/matcher macros for host and device, and wires the build and helper macros to use prefixed Catch2 forms. important: ChangesCatch2 host+device testing infrastructure
Suggested labels
Suggested reviewers
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
c2h/include/c2h/catch2_test_macros.h (3)
27-37: 💤 Low valuesuggestion: The internal macro
C2H_INTERNAL_DEVICE_TEST_PRINTshould use a reserved identifier pattern (e.g.,_C2H_INTERNAL_DEVICE_TEST_PRINT) per libcudacxx coding style, which requires non-public symbols to use reserved identifiers (_prefix for macros).
64-69: ⚡ Quick winsuggestion: Line 67 concatenates
#EXPR#TYPEwithout spacing, producing output like "buf.at(4)std::out_of_range". Insert a space: `#EXPR " " `#TYPEor#EXPR ", "#TYPE``.
97-101: 💤 Low valuesuggestion: Similar to
CHECK_THROWS, line 100 concatenates#EXPR#TYPEwithout spacing. Use `#EXPR " " `#TYPEor#EXPR ", "#TYPE`` for readability.libcudacxx/test/libcudacxx/cuda/ccclrt/common/testing.cuh (1)
37-40: ⚡ Quick winsuggestion:
CCCLRT_CHECK_DEVICEandCCCLRT_FAIL_DEVICEcurrently forward toCHECK/FAILinstead of explicit device variants. This makes*_DEVICEbehavior depend on non-device alias semantics under different toolchain/config combinations. Consider forwarding toCHECK_DEVICEandFAIL_DEVICEfor consistency withCCCLRT_REQUIRE_DEVICE.As per coding guidelines, this path should prioritize host/device availability and portability across supported CUDA toolchains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6cfc2c30-30b1-4c59-8b4f-b0653a9160a2
📒 Files selected for processing (11)
c2h/CMakeLists.txtc2h/include/c2h/catch2_test_helper.hc2h/include/c2h/catch2_test_macros.hcub/test/catch2_test_device_radix_sort_custom.cucub/test/catch2_test_device_topk_api.cucub/test/insert_nested_NVTX_range_guard.hcudax/test/common/testing.cuhcudax/test/cufile/driver_register.cucudax/test/utility/optionally_static.culibcudacxx/test/libcudacxx/cuda/ccclrt/common/testing.cuhlibcudacxx/test/libcudacxx/cuda/containers/buffer/access.cu
This comment has been minimized.
This comment has been minimized.
| // <catch2/matchers/catch_matchers.hpp> | ||
|
|
||
| #define REQUIRE_THROWS_WITH(...) CATCH_REQUIRE_THROWS_WITH(__VA_ARGS__) | ||
| #define REQUIRE_THROWS_MATCHES(...) CATCH_REQUIRE_THROWS_MATCHES(__VA_ARGS__) |
There was a problem hiding this comment.
In my humble (and correct!) opinion, REQUIRE_THROWS_MATCHES() is the only proper way to check exceptions thrown as it checks both the type and expected message so there is no ambiguity on which exception is being captured.
Perhaps we can take this time to fix up some defaults and just delete the other macros.
There was a problem hiding this comment.
We definitely can! Let's move this discussion to slack/new issue. This PR really only aims to provide host + device working alternatives
There was a problem hiding this comment.
I would strongly advise to stay with the exact semantics of Catch2. If REQUIRE_THROWS_MATCHES is indeed better, we should just use an agent to rewrite the tests not using it.
If we change the meaning of macros between C2H and Catch2, it will create a lot of confusion for maintainers.
| })) | ||
| #define REQUIRE_NOTHROW(...) NV_IF_ELSE_TARGET(NV_IS_HOST, (CATCH_REQUIRE_NOTHROW(__VA_ARGS__);), (__VA_ARGS__;)) | ||
|
|
||
| #define CHECK(...) \ |
There was a problem hiding this comment.
Similarly, we should consider removing CHECK() and replacing all instances with REQUIRE(). I am struggling to think of a good scenario where you would want test execution to continue after initial failure.
There was a problem hiding this comment.
I think there could be value in having more information in the log from some external testing like CI or QA, so you might not have to reproduce the issue yourself, while REQUIRE would not give you enough information to figure out the failure. I wouldn't say its common and I can see using CHECK too often lead to a more messy log with multiple lines of repeated same failure for example. So its very case by case, but I think there is space for both of these macros
There was a problem hiding this comment.
Agreed! But maybe someone else would have a valid use case for that
There was a problem hiding this comment.
o you might not have to reproduce the issue yourself, while REQUIRE would not give you enough information to figure out the failure.
Ehhh, overall I think this ends up being rare in practice.
I see it similarly to C++ compiler error messages. Yes, once in a blue moon seeing the 30 different overloads of operator<< that GCC failed to match (and Very Helpful(TM) description for each on why they didn't match) might help you debug the failure, but usually all you want to see is that you accidentally typed bool is_less = foo << bar when you meant bool is_less = foo < bar, and that's the first error message in that whole stack.
More often than not, CHECK() leads to red herring errors, e.g. when you have
CHECK(container.size() >= n);
// a lot of other unrelated code...
CHECK(container[n] == foo); // oopsAnd now you are left trying parse the crash log instead.
There was a problem hiding this comment.
I use CHECK() a lot where I want to validate a bunch of values after I ran a test. For example, I do a SortPairs and afterwards validate if the keys and values correspond to two reference arrays. I want to see both failures if they mismatch. Please keep the CHECK() macro.
| // example-end custom-type | ||
|
|
||
| static __host__ std::ostream& operator<<(std::ostream& os, const custom_t& self) | ||
| __host__ std::ostream& operator<<(std::ostream& os, const custom_t& self) |
There was a problem hiding this comment.
IMO then we should remove them if they really are unused.
| // <catch2/catch2_test_macros.hpp> | ||
|
|
||
| #define REQUIRE(...) \ | ||
| NV_IF_ELSE_TARGET(NV_IS_HOST, (CATCH_REQUIRE(__VA_ARGS__);), ({ \ |
There was a problem hiding this comment.
Don't you need to handle clang-coda here?
There was a problem hiding this comment.
It seems to be working correctly in most cases, for other problematic cases I added REQUIRE_DEVICE
bd9a3ea to
896439c
Compare
bernhardmgruber
left a comment
There was a problem hiding this comment.
I am fine with this, but I want to point out that this may create additional maintenance work when upgrading Catch2.
| })) | ||
| #define REQUIRE_NOTHROW(...) NV_IF_ELSE_TARGET(NV_IS_HOST, (CATCH_REQUIRE_NOTHROW(__VA_ARGS__);), (__VA_ARGS__;)) | ||
|
|
||
| #define CHECK(...) \ |
There was a problem hiding this comment.
I use CHECK() a lot where I want to validate a bunch of values after I ran a test. For example, I do a SortPairs and afterwards validate if the keys and values correspond to two reference arrays. I want to see both failures if they mismatch. Please keep the CHECK() macro.
| // <catch2/matchers/catch_matchers.hpp> | ||
|
|
||
| #define REQUIRE_THROWS_WITH(...) CATCH_REQUIRE_THROWS_WITH(__VA_ARGS__) | ||
| #define REQUIRE_THROWS_MATCHES(...) CATCH_REQUIRE_THROWS_MATCHES(__VA_ARGS__) |
There was a problem hiding this comment.
I would strongly advise to stay with the exact semantics of Catch2. If REQUIRE_THROWS_MATCHES is indeed better, we should just use an agent to rewrite the tests not using it.
If we change the meaning of macros between C2H and Catch2, it will create a lot of confusion for maintainers.
896439c to
694ebaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c5fe465-04e8-41f4-9cc6-a7dccf465957
📒 Files selected for processing (1)
c2h/include/c2h/catch2_test_macros.h
🥳 CI Workflow Results🟩 Finished in 2h 32m: Pass: 100%/358 | Total: 8d 09h | Max: 2h 31m | Hits: 68%/684690See results here. |
Catch 2 test macros are unsupported in device code, so we have other alternatives like
CUDAX_REQUIREorCCCLRT_REQUIREthat should work in both host and device code.I don't like this, so I found a way to allow us using the original macros directly. If
CATCH_CONFIG_PREFIX_ALLmacro is defined, catch 2 prepends these macros withCATCH_and the non-prefixed versions are not defined. We can then define those manually and define our own behaviour in device code.The output for failed tests looks something like:
I've also implemented some extensions, such as: