util: Error out at compile time if KJ_NO_EXCEPTIONS is set - #364
util: Error out at compile time if KJ_NO_EXCEPTIONS is set#364ryanofsky wants to merge 1 commit into
Conversation
libmultiprocess requires capnp/kj to be built with exception support: mp::serverInvoke (proxy-types.h) relies on capnp's RPC layer catching exceptions thrown by server methods and converting them into remote-exception replies for the client, which capnp only does when KJ_NO_EXCEPTIONS is unset. If it's set, that conversion code (kj::getCaughtExceptionAsKj) is compiled out of capnp/kj, and an exception thrown by a server method escapes uncaught instead, crashing the process. This was hit in practice on NetBSD (bitcoin/bitcoin#36058): capnp's kj/common.h exception-support autodetection is fooled there, because NetBSD's <sys/cdefs.h> defines a dummy __has_feature(x)=0 stub for compilers (GCC<14) that don't natively support __has_feature. capnp reads that as "no exceptions" even though the compiler has exceptions enabled, so NetBSD's prebuilt capnproto package ends up linked without the exception-catching code libmultiprocess needs. Fixed upstream in capnproto/capnproto#2756, but not yet in any capnproto release or in NetBSD's pkgsrc package. Fail the build immediately with an explanatory #error instead of letting this surface later as a runtime crash on the affected platform. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste |
| // KJ exception support is required, or exceptions thrown in serverInvoke below go uncaught | ||
| // and crash the process instead of being returned to the client as errors. | ||
| #if KJ_NO_EXCEPTIONS | ||
| #error "KJ_NO_EXCEPTIONS=1 is set but libmultiprocess requires support for exceptions. Please check build settings. On NetBSD you may also need to set -DKJ_NO_EXCEPTIONS=0 -DKJ_NO_RTTI=0 explicitly (https://github.com/capnproto/capnproto/pull/2756)" |
There was a problem hiding this comment.
While the comment about -DKJ_NO_RTTI=0 is technically correct, setting the flag isn't necessary in practice.
There was a problem hiding this comment.
While the comment about
-DKJ_NO_RTTI=0is technically correct, setting the flag isn't necessary in practice.
Thanks, I only saw the failing CI run in bitcoin/bitcoin#36058 and wasn't sure what was needed to make CI pass. I believe KJ_NO_RTTI is good to set though to improve exception error messages.
Why not check earlier, at configure time? Related: bitcoin/bitcoin#36058. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
re: #364 (comment)
Why not check earlier, at configure time?
This seems less reliable because the header is installed and can be compiled with different flags after being installed. I wouldn't object to another change adding a cmake check though if it would be helpful.
| // KJ exception support is required, or exceptions thrown in serverInvoke below go uncaught | ||
| // and crash the process instead of being returned to the client as errors. | ||
| #if KJ_NO_EXCEPTIONS | ||
| #error "KJ_NO_EXCEPTIONS=1 is set but libmultiprocess requires support for exceptions. Please check build settings. On NetBSD you may also need to set -DKJ_NO_EXCEPTIONS=0 -DKJ_NO_RTTI=0 explicitly (https://github.com/capnproto/capnproto/pull/2756)" |
There was a problem hiding this comment.
While the comment about
-DKJ_NO_RTTI=0is technically correct, setting the flag isn't necessary in practice.
Thanks, I only saw the failing CI run in bitcoin/bitcoin#36058 and wasn't sure what was needed to make CI pass. I believe KJ_NO_RTTI is good to set though to improve exception error messages.
|
LGTM. ACK 4a1f400 |
4 participants
Cap'n Proto has a bug on netbsd where it incorrectly detects compiler does not support exceptions (reported and fixed in capnproto/capnproto#2756) causing exceptions thrown from bitcoin core IPC methods not be caught and leading to the CI failure reported bitcoin/bitcoin#36058.
Failure can be worked around by adding `-DKJ_NO_EXCEPTIONS=0`` to the build configuration so add an error message to detect when it would happen and suggest this.