Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bb): stack traces for check_circuit #6851

Merged
merged 16 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from 12 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
10 changes: 9 additions & 1 deletion barretenberg/cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ configure_file(
# Add doxygen build command
find_package(Doxygen)
if (DOXYGEN_FOUND)
add_custom_target(build_docs
add_custom_target(build_docs
COMMAND ${DOXYGEN_EXECUTABLE} ${PROJECT_SOURCE_DIR}/docs/Doxyfile
WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
COMMENT "Generate documentation with Doxygen")
Expand All @@ -34,6 +34,8 @@ option(DISABLE_TBB "Intel Thread Building Blocks" ON)
option(COVERAGE "Enable collecting coverage from tests" OFF)
option(ENABLE_ASAN "Address sanitizer for debugging tricky memory corruption" OFF)
option(ENABLE_HEAVY_TESTS "Enable heavy tests when collecting coverage" OFF)
# Note: Must do 'sudo apt-get install libdw-dev' or equivalent
option(CHECK_CIRCUIT_STACKTRACES "Enable (slow) stack traces for check circuit" OFF)

if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "arm64")
message(STATUS "Compiling for ARM.")
Expand All @@ -45,6 +47,10 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "a
set(DISABLE_TBB 0)
endif()

if(CHECK_CIRCUIT_STACKTRACES)
add_compile_options(-DCHECK_CIRCUIT_STACKTRACES)
endif()

if(ENABLE_ASAN)
add_compile_options(-fsanitize=address)
add_link_options(-fsanitize=address)
Expand Down Expand Up @@ -136,6 +142,8 @@ include(cmake/gtest.cmake)
include(cmake/benchmark.cmake)
include(cmake/module.cmake)
include(cmake/msgpack.cmake)
include(cmake/backward-cpp.cmake)

add_subdirectory(src)
if (ENABLE_ASAN AND NOT(FUZZING))
find_program(LLVM_SYMBOLIZER_PATH NAMES llvm-symbolizer-16)
Expand Down
22 changes: 16 additions & 6 deletions barretenberg/cpp/CMakePresets.json
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,16 @@
"LDFLAGS": "-O2 -gdwarf-4"
}
},
{
"name": "clang16-dbg-fast-circuit-check-traces",
"displayName": "Optimized debug build with Clang-16 with stack traces for failing circuit checks",
"description": "Build with globally installed Clang-16 in optimized debug mode with stack traces for failing circuit checks",
"inherits": "clang16-dbg-fast",
"binaryDir": "build-debug-fast-circuit-check-traces",
"cacheVariables": {
"CHECK_CIRCUIT_STACKTRACES": "ON"
}
},
{
"name": "clang16-assert",
"binaryDir": "build-assert",
Expand Down Expand Up @@ -405,6 +415,11 @@
"inherits": "default",
"configurePreset": "clang16-dbg-fast"
},
{
"name": "clang16-dbg-fast-circuit-check-traces",
"inherits": "clang16-dbg-fast",
"configurePreset": "clang16-dbg-fast-circuit-check-traces"
},
{
"name": "clang16-assert",
"inherits": "default",
Expand Down Expand Up @@ -480,12 +495,7 @@
"configurePreset": "wasm",
"inheritConfigureEnvironment": true,
"jobs": 0,
"targets": [
"barretenberg.wasm",
"barretenberg",
"wasi",
"env"
]
"targets": ["barretenberg.wasm", "barretenberg", "wasi", "env"]
},
{
"name": "wasm-dbg",
Expand Down
11 changes: 11 additions & 0 deletions barretenberg/cpp/cmake/backward-cpp.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
if(CHECK_CIRCUIT_STACKTRACES)
include(FetchContent)

# Also requires one of: libbfd (gnu binutils), libdwarf, libdw (elfutils)
FetchContent_Declare(backward
GIT_REPOSITORY https://github.com/bombela/backward-cpp
GIT_TAG 51f0700452cf71c57d43c2d028277b24cde32502
SYSTEM # optional, the Backward include directory will be treated as system directory
)
FetchContent_MakeAvailable(backward)
endif()
26 changes: 26 additions & 0 deletions barretenberg/cpp/cmake/module.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ function(barretenberg_module MODULE_NAME)
${TBB_IMPORTED_TARGETS}
)

if(CHECK_CIRCUIT_STACKTRACES)
target_link_libraries(
${MODULE_NAME}_objects
PUBLIC
Backward::Interface
)
target_link_options(
${MODULE_NAME}
PRIVATE
-ldw -lelf
)
endif()

# enable msgpack downloading via dependency (solves race condition)
add_dependencies(${MODULE_NAME} msgpack-c)
add_dependencies(${MODULE_NAME}_objects msgpack-c)
Expand Down Expand Up @@ -88,6 +101,19 @@ function(barretenberg_module MODULE_NAME)
)
list(APPEND exe_targets ${MODULE_NAME}_tests)

if(CHECK_CIRCUIT_STACKTRACES)
target_link_libraries(
${MODULE_NAME}_test_objects
PUBLIC
Backward::Interface
)
target_link_options(
${MODULE_NAME}_tests
PRIVATE
-ldw -lelf
)
endif()

if(WASM)
target_link_options(
${MODULE_NAME}_tests
Expand Down
12 changes: 12 additions & 0 deletions barretenberg/cpp/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -188,4 +188,16 @@ if(WASM)
-nostartfiles -Wl,--no-entry,--export-dynamic
)

if(CHECK_CIRCUIT_STACKTRACES)
target_link_libraries(
barretenberg.wasm
PUBLIC
Backward::Interface
)
target_link_options(
barretenberg.wasm
PRIVATE
-ldw -lelf
)
endif()
endif()
12 changes: 12 additions & 0 deletions barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,16 @@ if (NOT(FUZZING))
barretenberg
env
)
if(CHECK_CIRCUIT_STACKTRACES)
target_link_libraries(
bb
PUBLIC
Backward::Interface
)
target_link_options(
bb
PRIVATE
-ldw -lelf
)
endif()
endif()
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ class StandardCircuitChecker {
FF gate_sum = block.q_m()[i] * left * right + block.q_1()[i] * left + block.q_2()[i] * right +
block.q_3()[i] * output + block.q_c()[i];
if (!gate_sum.is_zero()) {
#ifdef CHECK_CIRCUIT_STACKTRACES
block.stack_traces.print(i);
#endif
info("gate number", i);
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate)
fr h = fr(8);

{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
UltraCircuitBuilder circuit_constructor;
auto a_idx = circuit_constructor.add_variable(a);
auto b_idx = circuit_constructor.add_variable(b);
auto c_idx = circuit_constructor.add_variable(c);
Expand All @@ -339,7 +339,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate)
}

{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
UltraCircuitBuilder circuit_constructor;
auto a_idx = circuit_constructor.add_variable(a);
auto b_idx = circuit_constructor.add_variable(b);
auto c_idx = circuit_constructor.add_variable(c);
Expand All @@ -355,7 +355,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate)
EXPECT_EQ(result, false);
}
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
UltraCircuitBuilder circuit_constructor;
auto a_idx = circuit_constructor.add_variable(a);
auto b_idx = circuit_constructor.add_variable(b);
auto c_idx = circuit_constructor.add_variable(c);
Expand All @@ -371,7 +371,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate)
EXPECT_EQ(result, false);
}
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
UltraCircuitBuilder circuit_constructor;
auto a_idx = circuit_constructor.add_variable(a);
auto c_idx = circuit_constructor.add_variable(c);
auto d_idx = circuit_constructor.add_variable(d);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ template <typename Builder> bool UltraCircuitChecker::check(const Builder& build
size_t block_idx = 0;
for (auto& block : builder.blocks.get()) {
result = result && check_block(builder, block, tag_data, memory_data, lookup_hash_table);
if (result == false) {
if (!result) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just couldn't stand looking at bool == false :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Snob :)

info("Failed at block idx = ", block_idx);
return false;
}
Expand All @@ -49,7 +49,7 @@ template <typename Builder> bool UltraCircuitChecker::check(const Builder& build

// Tag check is only expected to pass after entire execution trace (all blocks) have been processed
result = result && check_tag_data(tag_data);
if (result == false) {
if (!result) {
info("Failed tag check.");
return false;
}
Expand All @@ -71,57 +71,56 @@ bool UltraCircuitChecker::check_block(Builder& builder,
params.eta_two = memory_data.eta_two;
params.eta_three = memory_data.eta_three;

auto report_fail = [&](const char* message, size_t row_idx) {
info(message, row_idx);
#ifdef CHECK_CIRCUIT_STACKTRACES
block.stack_traces.print(row_idx);
#endif
return false;
};

// Perform checks on each gate defined in the builder
bool result = true;
for (size_t idx = 0; idx < block.size(); ++idx) {

populate_values(builder, block, values, tag_data, memory_data, idx);

result = result && check_relation<Arithmetic>(values, params);
if (result == false) {
info("Failed Arithmetic relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed Arithmetic relation at row idx = ", idx);
}
result = result && check_relation<Elliptic>(values, params);
if (result == false) {
info("Failed Elliptic relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed Elliptic relation at row idx = ", idx);
}
result = result && check_relation<Auxiliary>(values, params);
if (result == false) {
info("Failed Auxiliary relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed Auxiliary relation at row idx = ", idx);
}
result = result && check_relation<DeltaRangeConstraint>(values, params);
if (result == false) {
info("Failed DeltaRangeConstraint relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed DeltaRangeConstraint relation at row idx = ", idx);
}
result = result && check_lookup(values, lookup_hash_table);
if (result == false) {
info("Failed Lookup check relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed Lookup check relation at row idx = ", idx);
}
if constexpr (IsMegaBuilder<Builder>) {
result = result && check_relation<PoseidonInternal>(values, params);
if (result == false) {
info("Failed PoseidonInternal relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed PoseidonInternal relation at row idx = ", idx);
}
result = result && check_relation<PoseidonExternal>(values, params);
if (result == false) {
info("Failed PoseidonExternal relation at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed PoseidonExternal relation at row idx = ", idx);
}
result = result && check_databus_read(values, builder);
if (result == false) {
info("Failed databus read at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed databus read at row idx = ", idx);
}
}
if (result == false) {
info("Failed at row idx = ", idx);
return false;
if (!result) {
return report_fail("Failed at row idx = ", idx);
}
}

Expand Down Expand Up @@ -223,8 +222,8 @@ void UltraCircuitChecker::populate_values(
values.w_l = builder.get_variable(block.w_l()[idx]);
values.w_r = builder.get_variable(block.w_r()[idx]);
values.w_o = builder.get_variable(block.w_o()[idx]);
// Note: memory_data contains indices into the block to which RAM/ROM gates were added so we need to check that we
// are indexing into the correct block before updating the w_4 value.
// Note: memory_data contains indices into the block to which RAM/ROM gates were added so we need to check that
// we are indexing into the correct block before updating the w_4 value.
if (block.has_ram_rom && memory_data.read_record_gates.contains(idx)) {
values.w_4 = compute_memory_record_term(
values.w_l, values.w_r, values.w_o, memory_data.eta, memory_data.eta_two, memory_data.eta_three);
Expand Down
Loading
Loading