Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/configs/sanitize.bash
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
CI_DESC="CI job running ThreadSanitizer"
CI_DIR=build-sanitize
NIX_ARGS=(--arg enableLibcxx true --argstr libcxxSanitizers "Thread")
export CXX=clang++
export CXXFLAGS="-ggdb -Werror -Wall -Wextra -Wpedantic -Wthread-safety -Wno-unused-parameter -fsanitize=thread"
CMAKE_ARGS=()
Expand Down
3 changes: 3 additions & 0 deletions include/mp/proxy-io.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class LoggingErrorHandler : public kj::TaskSet::ErrorHandler
EventLoop& m_loop;
};

//! Log flags. Update stringify function if changed!
enum class Log {
Trace = 0,
Debug,
Expand All @@ -107,6 +108,8 @@ enum class Log {
Raise,
};

kj::StringPtr KJ_STRINGIFY(Log flags);

struct LogMessage {

//! Message to be logged
Expand Down
8 changes: 7 additions & 1 deletion shell.nix
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,17 @@
, minimal ? false # Whether to create minimal shell without extra tools (faster when cross compiling)
, capnprotoVersion ? null
, cmakeVersion ? null
, libcxxSanitizers ? null # Optional LLVM_USE_SANITIZER value to use for libc++, see https://llvm.org/docs/CMake.html
}:

let
lib = pkgs.lib;
llvm = crossPkgs.llvmPackages_20;
llvmBase = crossPkgs.llvmPackages_21;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7eb1da1 ci: Use tsan-instrumented libcxx in sanitizers job: not sure if the llvm version bump is worth mentioning, but 21 is also the version we use in the Bitcoin Core TSan job.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7eb1da1:

LLVM version 21 correctly matches bitcoin-core's tsan ci llvm version https://github.com/bitcoin/bitcoin/blob/master/ci/test/00_setup_env_native_tsan.sh#L11-L14

llvm = llvmBase // lib.optionalAttrs (libcxxSanitizers != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7eb1da1:

Applies LLVM_USE_SANITIZER if specified. This will apply the "Thread" sanitizer specified in NIX_ARGS above.

libcxx = llvmBase.libcxx.override {
devExtraCmakeFlags = [ "-DLLVM_USE_SANITIZER=${libcxxSanitizers}" ];
};
};
capnprotoHashes = {
"0.7.0" = "sha256-Y/7dUOQPDHjniuKNRw3j8dG1NI9f/aRWpf8V0WzV9k8=";
"0.7.1" = "sha256-3cBpVmpvCXyqPUXDp12vCFCk32ZXWpkdOliNH37UwWE=";
Expand Down
13 changes: 13 additions & 0 deletions src/mp/proxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <kj/debug.h>
#include <kj/function.h>
#include <kj/memory.h>
#include <kj/string.h>
#include <map>
#include <memory>
#include <optional>
Expand Down Expand Up @@ -430,4 +431,16 @@ std::string LongThreadName(const char* exe_name)
return g_thread_context.thread_name.empty() ? ThreadName(exe_name) : g_thread_context.thread_name;
}

kj::StringPtr KJ_STRINGIFY(Log v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ca3c05d:

This function is required by KJ for it to stringify Log objects https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#stringification

{
switch (v) {
case Log::Trace: return "Trace";
case Log::Debug: return "Debug";
case Log::Info: return "Info";
case Log::Warning: return "Warning";
case Log::Error: return "Error";
case Log::Raise: return "Raise";
}
return "<Log?>";
}
} // namespace mp
18 changes: 9 additions & 9 deletions test/mp/test/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@
#include <capnp/rpc.h>
#include <condition_variable>
#include <cstring>
#include <exception>
#include <functional>
#include <future>
#include <iostream>
#include <kj/async.h>
#include <kj/async-io.h>
#include <kj/common.h>
Expand All @@ -24,14 +22,15 @@
#include <kj/test.h>
#include <memory>
#include <mp/proxy.h>
#include "mp/proxy.capnp.h"
#include <mp/proxy.capnp.h>
#include <mp/proxy-io.h>
#include "mp/util.h"
#include <mp/util.h>
#include <optional>
#include <set>
#include <stdexcept>
#include <string>
#include <string_view>
#include <system_error>
#include <thread>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -67,9 +66,10 @@ class TestSetup

TestSetup(bool client_owns_connection = true)
: thread{[&] {
EventLoop loop("mptest", [](mp::LogMessage log_data) {
std::cout << "LOG" << (int)log_data.level << ": " << log_data.message << "\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. This was a placeholder that I forgot to revisit.

if (log_data.level == mp::Log::Raise) throw std::runtime_error(log_data.message);
EventLoop loop("mptest", [](mp::LogMessage log) {
// Info logs are not printed by default, but will be shown with `mptest --verbose`
KJ_LOG(INFO, log.level, log.message);
if (log.level == mp::Log::Raise) throw std::runtime_error(log.message);
});
auto pipe = loop.m_io_context.provider->newTwoWayPipe();

Expand Down Expand Up @@ -333,9 +333,9 @@ KJ_TEST("Make simultaneous IPC callbacks with same request_thread and callback_t
{
signal.get_future().get();
}
catch(const std::exception& e)
catch (const std::future_error& e)
{
KJ_EXPECT(e.what() == std::string("Future already retrieved"));
KJ_EXPECT(e.code() == std::make_error_code(std::future_errc::future_already_retrieved));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c332774:

Not important, but I think the following is worth mentioning:
Maybe we don't need the KJ_EXPECT here at all? The signal itself is a prop for testing, and the catch block already catches the error we expect. I don't think it will be detrimental to the test to remove the KJ_EXPECT call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I KJ_EXPECT checking for the right error code is pretty inessential to the test and would be ok to drop. I don't think I see any downsides of keeping it though, assuming it still passes and correctly describes what's expected to happen during the test.

Copy link
Member

@Sjors Sjors Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the error code check unless it causes problems later. It makes the test easier to understand too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

}
};

Expand Down