The main thing this change does is add a simple github actions CI job to build the project, run tests, and do checks with iwyu and clang-tidy.
It also fixes bitcoin CI warnings reported by fanquake in https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2988011933 that show up in the logs but do not cause errors, as well as a number of other warnings that do not seem to show up in bitcoin CI. Individual commit messages describe the fixes in detail.
src/mp/gen.cpp:82:13: error: function 'ForEachMethod' is within a recursive call chain [misc-no-recursion,-warnings-as-errors]
82 | static void ForEachMethod(const capnp::InterfaceSchema& interface, const std::function<void(const capnp::InterfaceSchema& interface, const capnp::InterfaceSchema::Method)>& callback)
include/mp/proxy-types.h:203:98: error: 'values' used after it was forwarded [bugprone-use-after-move,-warnings-as-errors]
203 | CustomBuildField(TypeList<LocalTypes...>(), Priority<3>(), context, std::forward<Values>(values)...,
| ^
include/mp/proxy-types.h:202:9: note: forward occurred here
202 | if (CustomHasValue(context, std::forward<Values>(values)...)) {
| ^
include/mp/proxy-types.h:333:74: error: 'arg2' used after it was forwarded [bugprone-use-after-move,-warnings-as-errors]
333 | next_fn.handleChain(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), typename S::Second(),
| ^
include/mp/proxy-types.h:332:9: note: forward occurred here
332 | handleChain(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), typename S::First());
include/mp/proxy-types.h:399:70: error: 'values' used after it was forwarded [bugprone-use-after-move,-warnings-as-errors]
399 | ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
| ^
include/mp/proxy-types.h:396:17: note: forward occurred here
396 | MaybeBuildField(std::integral_constant<bool, Accessor::in>(), ParamList(), invoke_context,
include/mp/type-number.h:53:77: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
53 | typename std::enable_if<std::is_enum<LocalType>::value>::type* enable = 0)
| ^
| nullptr
mp/include/mp/util.h:173:5: error: use '= default' to define a trivial destructor [modernize-use-equals-default,-warnings-as-errors]
173 | ~Lock() MP_RELEASE() {}
| ^ ~~
| = default;
This error should not be a real problem because code is taking an invalid
reference to an empty object that has no state and could never be used. But
taking the reference could technically be undefined behavior.
Reported by fanquake <fanquake@gmail.com>
https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2988011933
https://cirrus-ci.com/task/6552721135763456
https://api.cirrus-ci.com/v1/task/6552721135763456/logs/ci.log
include/mp/proxy-types.h:134:5: error: Address of stack memory associated with temporary object of type '(lambda at include/mp/proxy-types.h:134:51)' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference [clang-analyzer-core.StackAddressEscape,-warnings-as-errors]
134 | return ReadDestEmplace{TypeList<LocalType>(), [&](auto&&... args) -> decltype(auto) {
| ^
build/test/mp/test/foo.capnp.proxy-server.c++:51:12: note: Calling 'serverInvoke<mp::ProxyServer<mp::test::messages::FooInterface>, capnp::CallContext<mp::test::messages::FooInterface::PassCustomParams, mp::test::messages::FooInterface::PassCustomResults>, mp::ServerField<1, mp::Accessor<mp::foo_fields::Arg, 17>, mp::ServerRet<mp::Accessor<mp::foo_fields::Result, 18>, mp::ServerCall>>>'
51 | return serverInvoke(*this, call_context, MakeServerField<1, Accessor<foo_fields::Arg, FIELD_IN | FIELD_BOXED>>(Make<ServerRet, Accessor<foo_fields::Result, FIELD_OUT | FIELD_BOXED>>(ServerCall())));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:739:16: note: Calling 'ReplaceVoid<(lambda at include/mp/proxy-types.h:739:28), (lambda at include/mp/proxy-types.h:740:13)>'
739 | return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); },
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
740 | [&]() { return kj::Promise<CallContext>(kj::mv(call_context)); })
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:700:19: note: 'is_same_v' is true
700 | if constexpr (std::is_same_v<decltype(fn()), void>) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:700:5: note: Taking true branch
700 | if constexpr (std::is_same_v<decltype(fn()), void>) {
| ^
include/mp/proxy-types.h:701:9: note: Calling 'operator()'
701 | fn();
| ^~~~
include/mp/proxy-types.h:739:43: note: Calling 'ServerField::invoke'
739 | return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); },
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:563:16: note: Calling 'PassField<mp::Accessor<mp::foo_fields::Arg, 17>, mp::test::FooCustom, mp::ServerInvokeContext<mp::ProxyServer<mp::test::messages::FooInterface>, capnp::CallContext<mp::test::messages::FooInterface::PassCustomParams, mp::test::messages::FooInterface::PassCustomResults>>, const mp::ServerRet<mp::Accessor<mp::foo_fields::Result, 18>, mp::ServerCall> &, mp::TypeList<>>'
563 | return PassField<Accessor>(Priority<2>(),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
564 | typename Split<argc, ArgTypes>::First(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
565 | server_context,
| ~~~~~~~~~~~~~~~
566 | this->parent(),
| ~~~~~~~~~~~~~~~
567 | typename Split<argc, ArgTypes>::Second(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
568 | std::forward<Args>(args)...);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:304:5: note: Calling 'MaybeReadField<mp::TypeList<mp::test::FooCustom>, mp::InvokeContext &, mp::StructField<mp::Accessor<mp::foo_fields::Arg, 17>, const mp::test::messages::FooInterface::PassCustomParams::Reader>, mp::ReadDestEmplace<mp::test::FooCustom, (lambda at include/mp/proxy-types.h:305:83)>>'
304 | MaybeReadField(std::integral_constant<bool, Accessor::in>(), TypeList<ArgType>(), invoke_context,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
305 | Make<StructField, Accessor>(params), ReadDestEmplace(TypeList<ArgType>(), [&](auto&&... args) -> auto& {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
306 | param.emplace(std::forward<decltype(args)>(args)...);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
307 | return *param;
| ~~~~~~~~~~~~~~
308 | }));
| ~~~
include/mp/proxy-types.h:276:5: note: Calling 'ReadField<mp::test::FooCustom, mp::InvokeContext &, mp::StructField<mp::Accessor<mp::foo_fields::Arg, 17>, const mp::test::messages::FooInterface::PassCustomParams::Reader>, mp::ReadDestEmplace<mp::test::FooCustom, (lambda at include/mp/proxy-types.h:305:83)>>'
276 | ReadField(std::forward<Args>(args)...);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:175:12: note: Calling 'CustomReadField<mp::StructField<mp::Accessor<mp::foo_fields::Arg, 17>, const mp::test::messages::FooInterface::PassCustomParams::Reader>, mp::ReadDestEmplace<mp::test::FooCustom, (lambda at include/mp/proxy-types.h:305:83)>>'
175 | return CustomReadField(TypeList<RemoveCvRef<LocalTypes>...>(), Priority<2>(), std::forward<Args>(args)...);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test/mp/test/foo-types.h:51:12: note: Calling 'ReadDestEmplace::update'
51 | return read_dest.update([&](FooCustom& value) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
52 | value.v1 = ReadField(TypeList<std::string>(), invoke_context, mp::Make<mp::ValueField>(custom.getV1()), ReadDestTemp<std::string>());
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
53 | value.v2 = custom.getV2();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
54 | });
| ~~
include/mp/proxy-types.h:112:23: note: 'is_const_v' is false
112 | if constexpr (std::is_const_v<std::remove_reference_t<std::invoke_result_t<EmplaceFn>>>) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:112:9: note: Taking false branch
112 | if constexpr (std::is_const_v<std::remove_reference_t<std::invoke_result_t<EmplaceFn>>>) {
| ^
include/mp/proxy-types.h:122:13: note: Calling 'operator()'
122 | update_fn(temp);
| ^~~~~~~~~~~~~~~
test/mp/test/foo-types.h:52:113: note: Calling 'ReadDestTemp<std::basic_string<char>>'
52 | value.v1 = ReadField(TypeList<std::string>(), invoke_context, mp::Make<mp::ValueField>(custom.getV1()), ReadDestTemp<std::string>());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:134:5: note: Address of stack memory associated with temporary object of type '(lambda at include/mp/proxy-types.h:134:51)' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference
134 | return ReadDestEmplace{TypeList<LocalType>(), [&](auto&&... args) -> decltype(auto) {
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
135 | return LocalType{std::forward<decltype(args)>(args)...};
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
136 | }};
| ~
Reported by fanquake <fanquake@gmail.com>
https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2988011933
https://cirrus-ci.com/task/6552721135763456
https://api.cirrus-ci.com/v1/task/6552721135763456/logs/ci.log
include/mp/type-number.h:55:32: error: The value '0' provided to the cast expression is not in the valid range of values for 'FooEnum' [clang-analyzer-optin.core.EnumCastOutOfRange,-warnings-as-errors]
55 | return read_dest.construct(static_cast<LocalType>(input.get()));
| ^
test/mp/test/foo.h:26:12: note: enum declared here
26 | enum class FooEnum : uint8_t { ONE = 1, TWO = 2, };
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/test/mp/test/foo.capnp.proxy-server.c++:63:12: note: Calling 'serverInvoke<mp::ProxyServer<mp::test::messages::FooInterface>, capnp::CallContext<mp::test::messages::FooInterface::PassEnumParams, mp::test::messages::FooInterface::PassEnumResults>, mp::ServerField<1, mp::Accessor<mp::foo_fields::Arg, 1>, mp::ServerRet<mp::Accessor<mp::foo_fields::Result, 2>, mp::ServerCall>>>'
63 | return serverInvoke(*this, call_context, MakeServerField<1, Accessor<foo_fields::Arg, FIELD_IN>>(Make<ServerRet, Accessor<foo_fields::Result, FIELD_OUT>>(ServerCall())));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:739:16: note: Calling 'ReplaceVoid<(lambda at include/mp/proxy-types.h:739:28), (lambda at include/mp/proxy-types.h:740:13)>'
739 | return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); },
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
740 | [&]() { return kj::Promise<CallContext>(kj::mv(call_context)); })
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:700:19: note: 'is_same_v' is true
700 | if constexpr (std::is_same_v<decltype(fn()), void>) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:700:5: note: Taking true branch
700 | if constexpr (std::is_same_v<decltype(fn()), void>) {
| ^
include/mp/proxy-types.h:701:9: note: Calling 'operator()'
701 | fn();
| ^~~~
include/mp/proxy-types.h:739:43: note: Calling 'ServerField::invoke'
739 | return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); },
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:563:16: note: Calling 'PassField<mp::Accessor<mp::foo_fields::Arg, 1>, mp::test::FooEnum, mp::ServerInvokeContext<mp::ProxyServer<mp::test::messages::FooInterface>, capnp::CallContext<mp::test::messages::FooInterface::PassEnumParams, mp::test::messages::FooInterface::PassEnumResults>>, const mp::ServerRet<mp::Accessor<mp::foo_fields::Result, 2>, mp::ServerCall> &, mp::TypeList<>>'
563 | return PassField<Accessor>(Priority<2>(),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
564 | typename Split<argc, ArgTypes>::First(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
565 | server_context,
| ~~~~~~~~~~~~~~~
566 | this->parent(),
| ~~~~~~~~~~~~~~~
567 | typename Split<argc, ArgTypes>::Second(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
568 | std::forward<Args>(args)...);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:304:5: note: Calling 'MaybeReadField<mp::TypeList<mp::test::FooEnum>, mp::InvokeContext &, mp::StructField<mp::Accessor<mp::foo_fields::Arg, 1>, const mp::test::messages::FooInterface::PassEnumParams::Reader>, mp::ReadDestEmplace<mp::test::FooEnum, (lambda at include/mp/proxy-types.h:305:83)>>'
304 | MaybeReadField(std::integral_constant<bool, Accessor::in>(), TypeList<ArgType>(), invoke_context,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
305 | Make<StructField, Accessor>(params), ReadDestEmplace(TypeList<ArgType>(), [&](auto&&... args) -> auto& {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
306 | param.emplace(std::forward<decltype(args)>(args)...);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
307 | return *param;
| ~~~~~~~~~~~~~~
308 | }));
| ~~~
include/mp/proxy-types.h:276:5: note: Calling 'ReadField<mp::test::FooEnum, mp::InvokeContext &, mp::StructField<mp::Accessor<mp::foo_fields::Arg, 1>, const mp::test::messages::FooInterface::PassEnumParams::Reader>, mp::ReadDestEmplace<mp::test::FooEnum, (lambda at include/mp/proxy-types.h:305:83)>>'
276 | ReadField(std::forward<Args>(args)...);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:175:12: note: Calling 'CustomReadField<mp::test::FooEnum, mp::StructField<mp::Accessor<mp::foo_fields::Arg, 1>, const mp::test::messages::FooInterface::PassEnumParams::Reader>, mp::ReadDestEmplace<mp::test::FooEnum, (lambda at include/mp/proxy-types.h:305:83)>>'
175 | return CustomReadField(TypeList<RemoveCvRef<LocalTypes>...>(), Priority<2>(), std::forward<Args>(args)...);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/type-number.h:55:55: note: Calling 'StructField::get'
55 | return read_dest.construct(static_cast<LocalType>(input.get()));
| ^~~~~~~~~~~
include/mp/proxy-types.h:40:41: note: Calling 'Arg::get'
40 | decltype(auto) get() const { return Accessor::get(this->m_struct); }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/test/mp/test/foo.capnp.proxy.h:198:82: note: Calling 'Reader::getArg'
198 | template<typename S> static auto get(S&& s) -> decltype(s.getArg()) { return s.getArg(); }
| ^~~~~~~~~~
build/test/mp/test/foo.capnp.h:7042:10: note: Calling 'StructReader::getDataField'
7042 | return _reader.getDataField< ::int32_t>(
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7043 | ::capnp::bounded<0>() * ::capnp::ELEMENTS);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/capnp/layout.h:1099:7: note: Assuming the condition is false
1099 | if ((offset + ONE * ELEMENTS) * capnp::bitsPerElement<T>() <= dataSize) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/capnp/layout.h:1099:3: note: Taking false branch
1099 | if ((offset + ONE * ELEMENTS) * capnp::bitsPerElement<T>() <= dataSize) {
| ^
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/capnp/layout.h:1102:5: note: Returning zero
1102 | return static_cast<T>(0);
| ^~~~~~~~~~~~~~~~~~~~~~~~
build/test/mp/test/foo.capnp.h:7042:10: note: Returning from 'StructReader::getDataField'
7042 | return _reader.getDataField< ::int32_t>(
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7043 | ::capnp::bounded<0>() * ::capnp::ELEMENTS);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/test/mp/test/foo.capnp.h:7042:3: note: Returning zero
7042 | return _reader.getDataField< ::int32_t>(
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7043 | ::capnp::bounded<0>() * ::capnp::ELEMENTS);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/test/mp/test/foo.capnp.proxy.h:198:82: note: Returning from 'Reader::getArg'
198 | template<typename S> static auto get(S&& s) -> decltype(s.getArg()) { return s.getArg(); }
| ^~~~~~~~~~
build/test/mp/test/foo.capnp.proxy.h:198:75: note: Returning zero
198 | template<typename S> static auto get(S&& s) -> decltype(s.getArg()) { return s.getArg(); }
| ^~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:40:41: note: Returning from 'Arg::get'
40 | decltype(auto) get() const { return Accessor::get(this->m_struct); }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:40:34: note: Returning zero
40 | decltype(auto) get() const { return Accessor::get(this->m_struct); }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/type-number.h:55:55: note: Returning from 'StructField::get'
55 | return read_dest.construct(static_cast<LocalType>(input.get()));
| ^~~~~~~~~~~
include/mp/type-number.h:55:32: note: The value '0' provided to the cast expression is not in the valid range of values for 'FooEnum'
55 | return read_dest.construct(static_cast<LocalType>(input.get()));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Reported by fanquake <fanquake@gmail.com>
https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2988011933
https://cirrus-ci.com/task/6552721135763456
https://api.cirrus-ci.com/v1/task/6552721135763456/logs/ci.log
Error is spurious and comes from kj/async-inl.h and should be suppressed in the
next version of capnproto https://github.com/capnproto/capnproto/pull/2334
The error is a clang-analyzer false positive that comes from ABI-specific code
in Cap'n Proto that gets the starting function address (that can be passed to
addr2line) from a lambda or function object. This code calls a helper to get
the starting function address from a pointer-to-member-function, which in this
case is the the operator() member function. That code handles pointers to
virtual member functions, so it checks if the pointer is virtual by testing its
low-order bit, and if set, assumes the first bytes of the object are a vtable
pointer, and does pointer arithmetic with the vtable address. Clang-tidy
complains about this because it does not know the vtable address is valid,
assuming incorrectly it is a "garbage value".
This change turns off the UndefinedBinaryOperatorResult altogether instead of
suppressing this one instance because clang-tidy incorrectly considers this
error to come from "main file" of the translation unit (see
https://clang.llvm.org/extra/clang-tidy/, https://stackoverflow.com/a/47611238,
https://reviews.llvm.org/D26418). So it is not suppressed even though the
header is included via -isystem and clang-tidy --dump-config shows
"SystemHeaders: false". It is also not suppressed when exclude patterns are
added to the .clang-tidy configuration like:
HeaderFilterRegex: '.*'
ExcludeHeaderFilterRegex: '.*/include/kj/async-inl\.h$'
This has no effect because ExcludeHeaderFilterRegex does not override the
isInMainFile condition (https://github.com/llvm/llvm-project/pull/91400/files).
Adding NOLINT to the getLocalServer() line in types-context.h at the boundary
between libmultiprocess and Cap'n Proto code also does not suppress the error.
It does suppress clang-tidy "note:" lines below the NOLINT point in the call
stack, making the error messages shorter, but the only way of suppressing the
error completely seems to be either adding NOLINT inside the Cap'n Proto
header, which requires a patch, or adding it to all top-level callers of the
getLocalServer() function in .cpp files, which seems impractical and overbroad,
and I didn't attempt.
Complete error output is:
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/kj/async-inl.h:609:37: error: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors]
609 | return *(void**)(*(char**)obj + voff);
| ^
build/test/mp/test/foo.capnp.proxy-server.c++:93:12: note: Calling 'serverInvoke<mp::ProxyServer<mp::test::messages::FooFn>, capnp::CallContext<mp::test::messages::FooFn::CallParams, mp::test::messages::FooFn::CallResults>, mp::ServerField<0, mp::Accessor<mp::foo_fields::Context, 17>, mp::ServerRet<mp::Accessor<mp::foo_fields::Result, 2>, mp::ServerCall>>>'
93 | return serverInvoke(*this, call_context, MakeServerField<0, Accessor<foo_fields::Context, FIELD_IN | FIELD_BOXED>>(Make<ServerRet, Accessor<foo_fields::Result, FIELD_OUT>>(ServerCall())));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:739:16: note: Calling 'ReplaceVoid<(lambda at include/mp/proxy-types.h:739:28), (lambda at include/mp/proxy-types.h:740:13)>'
739 | return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); },
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
740 | [&]() { return kj::Promise<CallContext>(kj::mv(call_context)); })
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:700:19: note: 'is_same_v' is false
700 | if constexpr (std::is_same_v<decltype(fn()), void>) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:700:5: note: Taking false branch
700 | if constexpr (std::is_same_v<decltype(fn()), void>) {
| ^
include/mp/proxy-types.h:704:16: note: Calling 'operator()'
704 | return fn();
| ^~~~
include/mp/proxy-types.h:739:43: note: Calling 'ServerField::invoke'
739 | return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); },
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/proxy-types.h:563:16: note: Calling 'PassField<mp::Accessor<mp::foo_fields::Context, 17>, mp::ServerInvokeContext<mp::ProxyServer<mp::test::messages::FooFn>, capnp::CallContext<mp::test::messages::FooFn::CallParams, mp::test::messages::FooFn::CallResults>>, mp::ServerRet<mp::Accessor<mp::foo_fields::Result, 2>, mp::ServerCall>, mp::TypeList<>>'
563 | return PassField<Accessor>(Priority<2>(),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
564 | typename Split<argc, ArgTypes>::First(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
565 | server_context,
| ~~~~~~~~~~~~~~~
566 | this->parent(),
| ~~~~~~~~~~~~~~~
567 | typename Split<argc, ArgTypes>::Second(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
568 | std::forward<Args>(args)...);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/mp/type-context.h:151:12: note: Calling 'CapabilityServerSet::getLocalServer'
151 | return server.m_context.connection->m_threads.getLocalServer(thread_client)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/capnp/capability.h:1274:10: note: Calling 'Promise::then'
1274 | return getLocalServerInternal(client)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1275 | .then([](void* server) -> kj::Maybe<typename T::Server&> {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1276 | if (server == nullptr) {
| ~~~~~~~~~~~~~~~~~~~~~~~~
1277 | return nullptr;
| ~~~~~~~~~~~~~~~
1278 | } else {
| ~~~~~~~~
1279 | return *reinterpret_cast<typename T::Server*>(server);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1280 | }
| ~
1281 | });
| ~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/kj/async-inl.h:1295:32: note: Calling 'GetFunctorStartAddress::apply'
1295 | void* continuationTracePtr = _::GetFunctorStartAddress<_::FixVoid<T>&&>::apply(func);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/kj/async-inl.h:677:12: note: Calling 'PtmfHelper::apply'
677 | return PtmfHelper::from<ReturnType, Decay<Func>, ParamTypes...>(
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
678 | &Decay<Func>::operator()).apply(&func);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/kj/async-inl.h:606:9: note: Assuming the condition is true
606 | if (voff & 1) {
| ^~~~~~~~
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/kj/async-inl.h:606:5: note: Taking true branch
606 | if (voff & 1) {
| ^
/nix/store/46kiq9naswgbqfc14kc9nxcbgd0rv0m2-capnproto-1.1.0/include/kj/async-inl.h:609:37: note: The left operand of '+' is a garbage value
609 | return *(void**)(*(char**)obj + voff);
| ~~~~~~~~~~~~ ^Not sure why CI does not seem to run as part of this PR, but CI was successful in the branch behind the PR:
Maybe this PR has to be merged first before CI will run on any PRs.
I’ll submit a patch to cap’n proto to avoid the clang-analyzer-core.UndefinedBinaryOperatorResult error that’s disabled in the last commit of this PR 48ecb5f88989807fb7ce5672b60ca9b48f6b9454. Commit cb94a432971a0e3416da927e16776ddc85dcda15 tests a patch that seems to work.
Ideally we’d never see this error because cap’n proto headers are included with -isystem and we disable warnings from system headers, but clang seems to have quirks which cause this error to be associated with the “main file” of our translation unit and shows this error anyway without a way of filtering it out: https://reviews.llvm.org/D26418
Maybe this PR has to be merged first before CI will run on any PRs.
maybe the “actions” has to be enabled? At least on https://github.com/bitcoin/bitcoin/actions I see the tab, but https://github.com/bitcoin-core/libmultiprocess/actions is a 404 right now. See https://github.com/bitcoin-core/libmultiprocess/settings/actions
See https://github.com/bitcoin-core/libmultiprocess/settings/actions
Thanks! That seems right. I can’t change the setting there since I see “This setting has been disabled by organization administrators” but I assume if gets changed from “Disable actions” to one of the other settings this should start to work.
re: #184 (comment)
I’ll submit a patch to cap’n proto to avoid the
clang-analyzer-core.UndefinedBinaryOperatorResulterror that’s disabled in the last commit of this PR
Opened https://github.com/capnproto/capnproto/pull/2334 with this
Updated 48ecb5f88989807fb7ce5672b60ca9b48f6b9454 -> 80f1c2ef32423f53f0dfff0aeb9ce8b02376a873 (pr/ci.1 -> pr/ci.2, compare, CI) just updating the last commit message to mention the patch
17--modernize-concat-nested-namespaces,
18--modernize-deprecated-headers,
19--modernize-use-nodiscard,
20--modernize-use-trailing-return-type,
21--modernize-use-using,
22+bitcoin-*,
re: #184 (review)
In commit “tidy: copy clang-tidy file from bitcoin core” (c5498aa11ba6476b80b9bee3946fac131663cc93)
nit: This requires the tidy plugin, which isn’t available here. Could document, or ignore, up to you.
Thanks, removed in a followup (this commit just copies configuration verbatim trying to reproduce https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2988011933)
0@@ -0,0 +1,4 @@
1+#!/usr/bin/env bash
2+set -euo pipefail
re: #184 (review)
In commit “ci: add github actions job testing clang-20, clang-tidy, and iwyu” (3ca914c32f9348a0ee1f335fb12c06ab7e65fa3d)
Thanks, added tracing, but I don’t think there is a shorter way of writing -o pipefail. It also seems to be used in some guix scripts in the bitcoin repo and I feel like it is just common boilerplate you see in bash scripts. (FWIW most of this commit came from chatgpt)
-x means “eXit on error” or “eXecution tracing” or something else (same for the other options). But I can look it up, and this is just a style nit.
re: #184 (review)
I guess it is just me not knowing from reading this line if
-xmeans “eXit on error” or “eXecution tracing” or something else (same for the other options). But I can look it up, and this is just a style nit.
Makes sense, latest push switches to long form
When I update the subtree to use this in https://github.com/bitcoin/bitcoin/pull/31802 my macOS build fails:
0/Users/sjors/dev/bitcoin/src/ipc/capnp/protocol.cpp:45:42: error: no matching constructor for initialization of 'std::unique_lock<std::mutex>'
1 45 | std::unique_lock<std::mutex> lock(m_loop->m_mutex);
2
3/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/include/c++/v1/__mutex/unique_lock.h:40:52: note: candidate constructor not viable: no known conversion from 'Mutex' to 'mutex_type &' (aka 'std::mutex &') for 1st argument
4 40 | _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI explicit unique_lock(mutex_type& __m)
5
6...
Is this expected?
Other than that the code changes in 80f1c2ef32423f53f0dfff0aeb9ce8b02376a873 seem fine, with a combination of pleasing tidy and skipping some checks. I’d like to test it on top of #31802 to make sure we caught all the Pokemons though.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Also add IWYU to the default job, since using libc++ instead of libstdc++
causes IWYU to give slightly different recommendations. Better to comply with
both sets of recommendations so IWYU is usable on more platforms.
Bitcoin tidy plugin isn't currently enabled here so they don't do anything.
Suggested by maflcko <6399679+maflcko@users.noreply.github.com>
https://github.com/bitcoin-core/libmultiprocess/pull/184#discussion_r2161151944
Updated 80f1c2ef32423f53f0dfff0aeb9ce8b02376a873 -> f15724a3ddc7ac7330c8aedb9f11c14361a9133d (pr/ci.2 -> pr/ci.3, compare, CI) expanding ci job to cover libc++ and gcc, rewriting a commit message, and making a few other tweaks.
re: #184 (comment)
When I update the subtree to use this in bitcoin/bitcoin#31802 my macOS build fails:
I misread this error and didn’t realize it was coming from the bitcoin build not the libmultiprocess build, and assumed it might be caused a libc++ / libstdc++ difference (which is why I added libstdc++ ci job).
But I think this error is just caused by #160 and should be fixed by https://github.com/bitcoin/bitcoin/commit/d234efe92ec8b9f2bad63b6be46309b7cccccd24 from https://github.com/bitcoin/bitcoin/pull/32345
re: #184 (comment)
Actions should be available in this repository now.
Thanks I saw the setting on https://github.com/bitcoin-core/libmultiprocess/settings/actions was changed from “Disable actions” to “Allow bitcoin-core actions and reusable workflows.” Though after another push it still seems like PR actions don’t run here, but not sure if that is because of settings, or because no actions are defined on the master branch right now.
To experiment I did change “Allow bitcoin-core, and select non-bitcoin-core, actions and reusable workflows” and filled in “Allow specified actions and reusable workflows” list with:
So maybe that will have an effect if there is another push.
Should probably add documentation about this, but with latest push it should be possible to run the two CI jobs locally on linux if nix is installed with:
0CI_CONFIG=ci/configs/gcc-libstdc++.sh ci/run.sh
1CI_CONFIG=ci/configs/clang-libc++.sh ci/run.sh
re: #184 (comment)
When I update the subtree to use this in bitcoin/bitcoin#31802 my macOS build fails:
I misread this error and didn’t realize it was coming from the bitcoin build not the libmultiprocess build, and assumed it might be caused a libc++ / libstdc++ difference (which is why I added libstdc++ ci job).
But I think this error is just caused by #160 and should be fixed by bitcoin/bitcoin@d234efe from bitcoin/bitcoin#32345
Oh I see. #160 isn’t currently part of the Bitcoin Core subtree.
Indeed if I start from Bitcoin Core master @ e95bfc1d537e06d223dfcaec510181aa3d3f8386 and update the subtree to 258a617c1eecb7c8f4f6cb1c5fa3f5f3501d5e8f (current master), I get the same error. And it goes away with https://github.com/bitcoin/bitcoin/commit/d234efe92ec8b9f2bad63b6be46309b7cccccd24.
So it’s unrelated to this PR.
I opened https://github.com/Sjors/bitcoin/pull/90 on top of https://github.com/bitcoin/bitcoin/pull/31802 to test the changes here.
The linter trips up over the ci file names:
0[07:00:04.814] File 'src/ipc/libmultiprocess/ci/configs/clang-libc++.sh' does not match the allowed filename regexp ('^[a-zA-Z0-9/_.@][a-zA-Z0-9/_.@-]*$').
1[07:00:04.814] File 'src/ipc/libmultiprocess/ci/configs/gcc-libstdc++.sh' does not match the allowed filename regexp ('^[a-zA-Z0-9/_.@][a-zA-Z0-9/_.@-]*$').
2[07:00:04.814] File 'src/ipc/libmultiprocess/ci/configs/clang-libc++.sh' does not match the allowed source filename regexp ('^[a-z0-9_./-]+$'), or the exception regexp (^src/(secp256k1/|minisketch/|test/fuzz/FuzzedDataProvider.h)).
3[07:00:04.814] File 'src/ipc/libmultiprocess/ci/configs/gcc-libstdc++.sh' does not match the allowed source filename regexp ('^[a-z0-9_./-]+$'), or the exception regexp (^src/(secp256k1/|minisketch/|test/fuzz/FuzzedDataProvider.h)).
4[07:00:04.814] ERROR: There were 4 failed tests in the lint-files.py lint test. Please resolve the above errors.
Could rename them here or change the linter when you update the subtree.
I was unable to run the nix ci scripts on macOS:
0+ '[' x ']'
1+ source ci/configs/gcc-libstdc++.sh
2++ CMAKE_ARGS=(-DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter")
3+ cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug '-DCMAKE_CXX_FLAGS=-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter'
4CMake Error: Error: generator : Ninja
5Does not match the generator used previously: Unix Makefiles
6Either remove the CMakeCache.txt file and CMakeFiles directory or choose a different binary directory.
Log: https://gist.github.com/Sjors/670468320ae9034f261d7fa5d9d33347
It also fails on Ubuntu 25.04 with nix 2.14.1:
0$ CI_CONFIG=ci/configs/gcc-libstdc++.sh ci/run.sh
1+ '[' x ']'
2+ source ci/configs/gcc-libstdc++.sh
3++ CMAKE_ARGS=(-DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter")
4+ for stage in configure build test
5+ nix-shell --pure --keep CI_CONFIG --run ci/scripts/configure.sh shell.nix
6error:
7 … while calling the 'import' builtin
8
9 at /home/sjors/dev/libmultiprocess/shell.nix:1:10:
10
11 1| { pkgs ? import <nixpkgs> {}
12 | ^
13 2| , libcxx ? false # Whether to use libc++ toolchain and libraries instead of libstdc++
14
15 … while calling the 'findFile' builtin
16
17 at /home/sjors/dev/libmultiprocess/shell.nix:1:17:
18
19 1| { pkgs ? import <nixpkgs> {}
20 | ^
21 2| , libcxx ? false # Whether to use libc++ toolchain and libraries instead of libstdc++
22
23 error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I)
24
25 at «none»:0: (source not available)
Suggested by maflcko <6399679+maflcko@users.noreply.github.com>
https://github.com/bitcoin-core/libmultiprocess/pull/184#discussion_r2163106467
91@@ -92,7 +92,7 @@ struct StructField
92 template <typename LocalType, typename EmplaceFn>
93 struct ReadDestEmplace
94 {
95- ReadDestEmplace(TypeList<LocalType>, EmplaceFn&& emplace_fn) : m_emplace_fn(emplace_fn) {}
96+ ReadDestEmplace(TypeList<LocalType>, EmplaceFn emplace_fn) : m_emplace_fn(std::move(emplace_fn)) {}
97
98 //! Simple case. If ReadField impementation calls this construct() method
re: #184 (review)
unrelated nit, found by the llm: ` impementation -> implementation [spelling error in comment]
Thanks fixed in latest push
Actions should be available in this repository now.
Thanks I saw the setting on https://github.com/bitcoin-core/libmultiprocess/settings/actions was changed from “Disable actions” to “Allow bitcoin-core actions and reusable workflows.” Though after another push it still seems like PR actions don’t run here, but not sure if that is because of settings, or because no actions are defined on the master branch right now.
To experiment I did change “Allow bitcoin-core, and select non-bitcoin-core, actions and reusable workflows” and filled in “Allow specified actions and reusable workflows” list with:
* actions/checkout@* * cachix/install-nix-action@*,So maybe that will have an effect if there is another push.
Perhaps closing the PR and reopening it would help?
In any case, I suggest submitting a separate PR that only introduces the new workflow as a stub, without the Run CI script step, merging it promptly to ensure that all settings are correct.
Updated f15724a3ddc7ac7330c8aedb9f11c14361a9133d -> 52f036f94b7f90b157533d771842ef7697073d75 (pr/ci.3 -> pr/ci.4, compare, CI) making suggested changes, adding readme, consolidating ci scripts.
re: #184 (comment)
The linter trips up over the ci file names:
Thanks, renamed the files to fix this.
I was unable to run the nix ci scripts on macOS:
Thanks for testing, it looks like the build is clashing with an existing “build” directory. I updated scripts to use separate build directories now to avoid this problem.
It also fails on Ubuntu 25.04 with nix 2.14.1:
I had a difficult time trying to reproduce this. I tried single & multiuser nix installs and did not see this error. But then I noticed ubuntu seems to have a nix-setup-systemd package and saw the reported error error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I) because it looks the ubuntu setup does not install nixpkgs. Adding a nix channel manually fixed the problem:
0nix-channel --add https://nixos.org/channels/nixpkgs-unstable
1nix-channel --update
re: #184 (comment)
Should probably add documentation about this
Added ci/README.md file now
re: #184 (comment)
Perhaps closing the PR and reopening it would help?
Thanks that is a good idea, will try that! I would like to avoid opening a separate PR so this can be reviewed in a single round. Since CI does run successfully in the branch pushes, and I’m not too concerned about CI status not showing up on the bottom of this page.
It still complains on macOS:
0...
1[100%] Built target mpprinter
2make[2]: Doel 'example/CMakeFiles/mpexamples.dir/all' is niet opnieuw gemaakt vanwege fouten.
3make[1]: *** [CMakeFiles/Makefile2:1438: example/CMakeFiles/mpexamples.dir/rule] Fout 2
4make[1]: Doel 'mpexamples' is niet opnieuw gemaakt vanwege fouten.
5make: *** [Makefile:686: mpexamples] Fout 2
(in Dutch, which is itself weird)
Doing a git clean -dfx doesn’t fix it.
The LLVM one doesn’t work either, but differently: https://gist.github.com/Sjors/56b3bd67d0deaa58fd2561d86a993565
The Ubuntu suggestion did fix it for me. Si I’m not too worried about macOS not working.
Thanks for testing and too bad this isn’t working. It could be useful to have the CI scripts working and running conveniently on macos, so if you want to create a separate issue for this, it’d be welcome.
But good that you found a workaround, and I think this PR is probably useful enough getting CI working on Ubuntu and fixing lint errors, even if it is less portable than I might have hoped.
0@@ -0,0 +1,15 @@
1+CI_DESC="CI job using LLVM-based libraries and tools (clang, libc++, clang-tidy, iwyu) and testing Ninja"
2+CI_DIR=build-llvm
3+NIX_ARGS=(
4+ --arg libcxx true
5+)
6+CMAKE_ARGS=(
7+ -G Ninja
8+ -DCMAKE_CXX_COMPILER=clang++
9+ -DCMAKE_CXX_STANDARD_LIBRARY=libc++
I doubt that relying on the undocumented CMAKE_CXX_STANDARD_LIBRARY behaves as one might expect in this context. We definitely want:
0 -DCMAKE_CXX_COMPILER="clang++;-stdlib=libc++"
-stdlib=libc++ a permanent part of the compiler invocation, whereas CXXFLAGS may be altered by the build system.
whereas CXXFLAGS may be altered by the build system.
In this PR we are already setting CMAKE_CXX_FLAGS with a number of other flags. If you’re saying CMake might randomly alter those flags, then we should move all flags to the CMAKE_CXX_COMPILER no? Otherwise, it’s not exactly clear why we can’t set -stdlib=libc++ there as well.
If you’re saying CMake might randomly alter those flags…
I’m not blaming CMake. Just assuming that CMakeListss.txt can potentially process CMAKE_CXX_FLAGS in some way. I agree this sounds a bit vague. If it helps, here’s a quote form the CMake docs:
Note: Options that are required to make the compiler work correctly can be included as items in a list; they can not be changed.
I’m not opposed to using your suggestion.
FWIW, neither of these suggested changes seem to work and both result in:
0clang++: error: argument unused during compilation: '-stdlib=libc++' [-Werror,-Wunused-command-line-argument]
because the stdlib option is a linker option not a compile option. If I add the flag to CMAKE_EXE_LINKER_FLAGS it avoids that error, but then linking fails with lots of undefined symbol errors because libstdc++ headers are included but libc++ is linked.
Setting CMAKE_CXX_STANDARD_LIBRARY seems to be a clean way of choosing the library and avoiding this problem, and is only being set as part of this CI configuration, not part of the project itself. So I think I will just add a comment remarking that this option is experimental and may not work in the future.
There seem to be other ways of choosing the library (https://chatgpt.com/share/685bf4fc-b99c-800a-9342-d1d7458178b5) but this is simpler than any alternative and will probably be documented eventually and if it stops working the CI job can be changed without affecting the project itself.
because the stdlib option is a linker option not a compile option.
Sure about that?
re: #184 (review)
Sure about that?
Not at all… just assuming based on “argument unused during compilation” above. I’m happy to try other suggestions
CMAKE_CXX_STANDARD_LIBRARY variable is a valid approach.
Looking into the verbose build log, I still can’t convince myself that using the
CMAKE_CXX_STANDARD_LIBRARYvariable is a valid approach.
Not sure if you saw the chatgpt summary, but it linked to https://gitlab.kitware.com/cmake/cmake/-/issues/20851 which implements this setting and https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html which says internal variables “are subject to change, and not recommended for use in project code.”
This PR is only using the setting in a CI config file that’s not part of project code and nothing downstream will use or rely on. So I don’t think there is a problem here, but if there is one I could try an alternate approach or just drop commit 403f63a891b1cf0a41e2771ef99261e34ae1f082 since it is not essential
The resulting binaries show the opposite. For example,
0$ cmake -B build-llvm -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_CXX_STANDARD_LIBRARY=libc++
1$ cmake --build build-llvm -t mpexample
2$ ldd build-llvm/test/mptest
3 linux-vdso.so.1 (0x00007b7285999000)
4 libkj-test-1.0.1.so => /lib/x86_64-linux-gnu/libkj-test-1.0.1.so (0x00007b728596f000)
5 libcapnp-rpc-1.0.1.so => /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so (0x00007b728588e000)
6 libcapnp-1.0.1.so => /lib/x86_64-linux-gnu/libcapnp-1.0.1.so (0x00007b7285573000)
7 libkj-async-1.0.1.so => /lib/x86_64-linux-gnu/libkj-async-1.0.1.so (0x00007b728549e000)
8 libkj-1.0.1.so => /lib/x86_64-linux-gnu/libkj-1.0.1.so (0x00007b7285420000)
9 libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007b7285000000)
10 libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007b7285337000)
11 libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007b7285309000)
12 libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007b7284c00000)
13 /lib64/ld-linux-x86-64.so.2 (0x00007b728599b000)
The using of the libstdc++ is obvious.
UPD. Using CMake 3.31.8.
https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html says:
“if this variable is not defined, it is initialized and stored in the cache using values from environment”
That means, if you set CMAKE_CXX_FLAGS (which you should never do), then CXXFLAGS may not be used (wich is the reason why you should not set CMAKE_CXX_FLAGS).
Oh nice find. So I guess the option doesn’t even do anything here? The other changes in shell.nix do seem to work and Cap’n Proto binaries are built with libc++, but I can confirm ldd shows libmultiprocess binaries using a mix of libc++ and libstdc++ so CMAKE_CXX_STANDARD_LIBRARY is not having the desired effect.
0 linux-vdso.so.1 (0x00007f781796b000)
1 libkj-test.so.1.1.0 => /nix/store/q4dhry7vwy2pyybgipnqmnrwnc3x62qn-capnproto-1.1.0/lib/libkj-test.so.1.1.0 (0x00007f781781a000)
2 libcapnp-rpc.so.1.1.0 => /nix/store/q4dhry7vwy2pyybgipnqmnrwnc3x62qn-capnproto-1.1.0/lib/libcapnp-rpc.so.1.1.0 (0x00007f7817711000)
3 libcapnp.so.1.1.0 => /nix/store/q4dhry7vwy2pyybgipnqmnrwnc3x62qn-capnproto-1.1.0/lib/libcapnp.so.1.1.0 (0x00007f7817652000)
4 libkj-async.so.1.1.0 => /nix/store/q4dhry7vwy2pyybgipnqmnrwnc3x62qn-capnproto-1.1.0/lib/libkj-async.so.1.1.0 (0x00007f7817572000)
5 libkj.so.1.1.0 => /nix/store/q4dhry7vwy2pyybgipnqmnrwnc3x62qn-capnproto-1.1.0/lib/libkj.so.1.1.0 (0x00007f78174de000)
6 libpthread.so.0 => /nix/store/cg9s562sa33k78m63njfn1rw47dp9z0i-glibc-2.40-66/lib/libpthread.so.0 (0x00007f78174d9000)
7 libstdc++.so.6 => /nix/store/7c0v0kbrrdc2cqgisi78jdqxn73n3401-gcc-14.2.1.20250322-lib/lib/libstdc++.so.6 (0x00007f7817200000)
8 libm.so.6 => /nix/store/cg9s562sa33k78m63njfn1rw47dp9z0i-glibc-2.40-66/lib/libm.so.6 (0x00007f7817117000)
9 libgcc_s.so.1 => /nix/store/7c0v0kbrrdc2cqgisi78jdqxn73n3401-gcc-14.2.1.20250322-lib/lib/libgcc_s.so.1 (0x00007f78174ab000)
10 libc.so.6 => /nix/store/cg9s562sa33k78m63njfn1rw47dp9z0i-glibc-2.40-66/lib/libc.so.6 (0x00007f7816e00000)
11 /nix/store/cg9s562sa33k78m63njfn1rw47dp9z0i-glibc-2.40-66/lib/ld-linux-x86-64.so.2 => /nix/store/cg9s562sa33k78m63njfn1rw47dp9z0i-glibc-2.40-66/lib64/ld-linux-x86-64.so.2 (0x00007f781796d000)
12 libc++.so.1 => /nix/store/1gpni0qga8q9jkagxr9m4xzfzanq8avf-libcxx-20.1.5/lib/libc++.so.1 (0x00007f7816ceb000)
13 libc++abi.so.1 => /nix/store/1gpni0qga8q9jkagxr9m4xzfzanq8avf-libcxx-20.1.5/lib/libc++abi.so.1 (0x00007f78170cc000)
14 libatomic.so.1 => /nix/store/7c0v0kbrrdc2cqgisi78jdqxn73n3401-gcc-14.2.1.20250322-lib/lib/libatomic.so.1 (0x00007f781749d000)
I’m very surprised calling a c++ library which is compiled with libc++ from c++ code compiled with libstdc++ could even work, and had assumed if the build was working and tests were passing that would imply the same library was used everywhere. But I guess mixing libraries works because cap’n proto c++ api avoids types like std::string and std::vector, so the two runtime libraries don’t actually need to interact with each other.
I’ll see if there is some simple way to fix this, otherwise I’ll just drop the commit 403f63a891b1cf0a41e2771ef99261e34ae1f082 from the PR. Thanks for catching the problem!
re: #184 (review)
The using of the libstdc++ is obvious.
This should be fixed now. Now it links with only libc++ not libc++ and libstdc++ when I run locally.
re: #184 (review)
That means, if you set
CMAKE_CXX_FLAGS(which you should never do), thenCXXFLAGSmay not be used (wich is the reason why you should not setCMAKE_CXX_FLAGS). @purpleKarrot just saw this comment (sorry the thread appears all mixed up). It sound like you’re asking to change the two config files to use CXXFLAGS environment variable instead of CMAKE_CXX_FLAGS command line option. I think that makes sense because even though CMAKE_CXX_FLAGS is empty by default, hypothetically if a toolchain or similar file might wanted to influenceCMAKE_CXX_FLAGSwithCMAKE_CXX_FLAGS_INITthe command line here would clobber that value while the environment variable would be combined with it.
I guess this logic also extends to CMAKE_CXX_COMPILER and CXX would be recommended instead?
I’ll try switching to environment variables in the next push if that makes sense.
re: #184 (review)
I’ll try switching to environment variables in the next push if that makes sense.
Done now
80@@ -81,6 +81,8 @@ function(target_capnp_sources target include_prefix)
81 DEPENDS ${capnp_file}
82 VERBATIM
83 )
84+ # Skip linting for capnp-generated files but keep it for mpgen-generated ones
85+ set_source_files_properties(${capnp_file}.c++ PROPERTIES SKIP_LINTING TRUE)
SKIP_LINTING property has been added in CMake 3.27. However this project has:https://github.com/bitcoin-core/libmultiprocess/blob/258a617c1eecb7c8f4f6cb1c5fa3f5f3501d5e8f/CMakeLists.txt#L5
re: #184 (review)
Thanks, added note it will be ignored. (There are open prs to change the policy version and the minimum version, too, but those should be orthogonal to each other, and to this change adding CI jobs)
66@@ -51,6 +67,7 @@ configure_file(include/mp/config.h.in "${CMAKE_CURRENT_BINARY_DIR}/include/mp/co
67
68 # Generated C++ Capn'Proto schema files
69 capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)
70+set_source_files_properties("${MP_PROXY_SRCS}" PROPERTIES SKIP_LINTING TRUE)
6+CMAKE_ARGS=(
7+ -G Ninja
8+ -DCMAKE_CXX_COMPILER=clang++
9+ -DCMAKE_CXX_STANDARD_LIBRARY=libc++
10+ -DCMAKE_CXX_FLAGS="-Werror -Wall -Wextra -Wpedantic -Wthread-safety-analysis -Wno-unused-parameter"
11+ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
14+#include <memory>
15 #include <stddef.h>
16 #include <tuple>
17 #include <type_traits>
18 #include <utility>
19+#include <variant>
This is pretty odd. If I remove it, IWYU says:
0include/mp/proxy.h should add these lines:
1#include <variant> // for tuple
In this case maybe it would make sense to use a no_include pragma, since asking to include <variant> for tuple seems like an IWYU bug?
For other cases involving <cstring>, IWYU just wants it for memcpy, which I don’t see used explicitly, but is likely used in some macro somewhere. I’d probably just opt to keep the <cstring> includes. (Unless we can identify the macro add some appropriate mapping or export pragma.)
I’d agree with Marco and generally want to go along with IWYU recommendations instead of fighting them to save time, but happy to apply any changes if there are suggestions.
In this case maybe it would make sense to use a
no_includepragma, since asking to include for tuple seems like an IWYU bug?
no, i don’t think no_include is correct. As soon as you really use std::variant, the pragma will prevent it from being added at all, so the program may fail to compile for some users.
re: #184 (review)
Makes sense, added keep pragma.
1@@ -2,12 +2,17 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5+#include <init.capnp.h>
6+#include <init.capnp.proxy.h>
7+
8+#include <cstring>
re: #184 (review)
Added keep pragma here too
205+ cpp_server << "// IWYU pragma: no_include <kj/memory.h>\n";
206+ cpp_server << "// IWYU pragma: no_include <memory>\n";
207+ cpp_server << "#include <" << include_path << ".proxy.h>\n";
208+ cpp_server << "#include <" << include_path << ".proxy-types.h> // IWYU pragma: keep\n";
209+ cpp_server << "#include <capnp/generated-header-support.h>\n";
210+ cpp_server << "#include <cstring>\n";
re: #184 (review)
Added keep pragma here too
224+ cpp_client << "// IWYU pragma: no_include <memory>\n";
225+ cpp_client << "#include <" << include_path << ".h> // IWYU pragma: keep\n";
226+ cpp_client << "#include <" << include_path << ".proxy.h>\n";
227+ cpp_client << "#include <" << include_path << ".proxy-types.h> // IWYU pragma: keep\n";
228+ cpp_client << "#include <capnp/generated-header-support.h>\n";
229+ cpp_client << "#include <cstring>\n";
re: #184 (review)
Added keep pragma here too
Thanks for the review! I pushed another update since the CI_CLEAN option was not implemented correctly. (It was always wiping the build directory when running locally.) There is also a new commit adding a cross-compiled 32-bit job (in an attempt to reproduce mptest segfaults reported https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3003863654 and https://github.com/Sjors/bitcoin/pull/90#issuecomment-3003870162.
I’ll try to avoid making more changes here and add new jobs (sanitizers) in future PRs.
Updated 1bfed25a14762b7b2f4d11a1faaa86679af0707c -> 757e13a7554644d7ee1e3e85a9c56beeaffd5249 (pr/ci.7 -> pr/ci.8, compare, CI) with CI_CLEAN fix and new gnu32 CI job.
ACK 757e13a7554644d7ee1e3e85a9c56beeaffd5249
I ran the three ci scripts on Ubuntu.
We’ll need to be careful when updating the subtree in https://github.com/bitcoin/bitcoin/pull/32345. But debugging that should be easier with the new CI coverage added in this PR.
for reference the error is:
0actions/checkout@v4 and cachix/install-nix-action@v31 are not allowed to be used in bitcoin-core/libmultiprocess. Actions in this workflow must be: within a repository owned by bitcoin-core or matching the following: actions/checkout@*
1cachix/install-nix-action@*.
https://github.com/bitcoin-core/libmultiprocess/actions/runs/15907709886
actions/checkout@*,cachix/install-nix-action@* separated by a comma instead of a newline, and that might have helped.