From c971b3ad7904d3f169cc1e2bc7be3258b244a4ee Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 3 Jun 2024 06:02:14 -0400 Subject: [PATCH 01/11] chore: tool to analyze C++ compilation time Can be run from any folder Usage: `barretenberg/cpp/scripts/analyze_compile_time.sh clang16 all` --- .../cpp/scripts/analyze_compilation_time.sh | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 barretenberg/cpp/scripts/analyze_compilation_time.sh diff --git a/barretenberg/cpp/scripts/analyze_compilation_time.sh b/barretenberg/cpp/scripts/analyze_compilation_time.sh new file mode 100644 index 00000000000..154a2ec336b --- /dev/null +++ b/barretenberg/cpp/scripts/analyze_compilation_time.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +set -eu + +PRESET="${1:-wasm-threads}" +TARGET="${2:-barretenberg.wasm}" + +# Move above script dir. +cd $(dirname $0)/.. + +if ! [ -d ~/ClangBuildAnalyzer ] ; then + git clone https://github.com/aras-p/ClangBuildAnalyzer ~/ClangBuildAnalyzer + pushd ~/ClangBuildAnalyzer + mkdir -p build && cd build && cmake .. && make -j + popd +fi +rm -rf build-$PRESET-compiler-profile +cmake -DCMAKE_CXX_FLAGS=-ftime-trace --preset "$PRESET" -Bbuild-$PRESET-compiler-profile +cd build-$PRESET-compiler-profile +ninja $TARGET + +~/ClangBuildAnalyzer/build/ClangBuildAnalyzer --all . compile-profile.json +~/ClangBuildAnalyzer/build/ClangBuildAnalyzer --analyze compile-profile.json From 4727a6d50dcfaccefbb676b45d37957a26cb9a75 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 3 Jun 2024 10:03:16 +0000 Subject: [PATCH 02/11] chmod a+x --- .../cpp/scripts/analyze_compile_time.sh | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100755 barretenberg/cpp/scripts/analyze_compile_time.sh diff --git a/barretenberg/cpp/scripts/analyze_compile_time.sh b/barretenberg/cpp/scripts/analyze_compile_time.sh new file mode 100755 index 00000000000..154a2ec336b --- /dev/null +++ b/barretenberg/cpp/scripts/analyze_compile_time.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +set -eu + +PRESET="${1:-wasm-threads}" +TARGET="${2:-barretenberg.wasm}" + +# Move above script dir. +cd $(dirname $0)/.. + +if ! [ -d ~/ClangBuildAnalyzer ] ; then + git clone https://github.com/aras-p/ClangBuildAnalyzer ~/ClangBuildAnalyzer + pushd ~/ClangBuildAnalyzer + mkdir -p build && cd build && cmake .. && make -j + popd +fi +rm -rf build-$PRESET-compiler-profile +cmake -DCMAKE_CXX_FLAGS=-ftime-trace --preset "$PRESET" -Bbuild-$PRESET-compiler-profile +cd build-$PRESET-compiler-profile +ninja $TARGET + +~/ClangBuildAnalyzer/build/ClangBuildAnalyzer --all . compile-profile.json +~/ClangBuildAnalyzer/build/ClangBuildAnalyzer --analyze compile-profile.json From edcb7a032d3891416a1f828b6bf8efbb4f81ecd3 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 09:54:59 +0000 Subject: [PATCH 03/11] feat: bb check circuit traces --- barretenberg/cpp/CMakeLists.txt | 10 ++++- barretenberg/cpp/cmake/backward-cpp.cmake | 11 +++++ barretenberg/cpp/cmake/module.cmake | 26 ++++++++++++ barretenberg/cpp/src/CMakeLists.txt | 12 ++++++ .../cpp/src/barretenberg/bb/CMakeLists.txt | 12 ++++++ .../standard_circuit_checker.hpp | 3 ++ .../arithmetization/arithmetization.hpp | 42 +++++++++++++++++++ 7 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 barretenberg/cpp/cmake/backward-cpp.cmake diff --git a/barretenberg/cpp/CMakeLists.txt b/barretenberg/cpp/CMakeLists.txt index f277ebfa089..999072502fc 100644 --- a/barretenberg/cpp/CMakeLists.txt +++ b/barretenberg/cpp/CMakeLists.txt @@ -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") @@ -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) +# TODO turn off +option(CHECK_CIRCUIT_STACKTRACES "Enable (slow) stack traces for check circuit" ON) if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "arm64") message(STATUS "Compiling for ARM.") @@ -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) @@ -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) diff --git a/barretenberg/cpp/cmake/backward-cpp.cmake b/barretenberg/cpp/cmake/backward-cpp.cmake new file mode 100644 index 00000000000..b0d70b5e2cb --- /dev/null +++ b/barretenberg/cpp/cmake/backward-cpp.cmake @@ -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() \ No newline at end of file diff --git a/barretenberg/cpp/cmake/module.cmake b/barretenberg/cpp/cmake/module.cmake index 71fa5cd7dbe..6ba42e9ef35 100644 --- a/barretenberg/cpp/cmake/module.cmake +++ b/barretenberg/cpp/cmake/module.cmake @@ -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) @@ -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 diff --git a/barretenberg/cpp/src/CMakeLists.txt b/barretenberg/cpp/src/CMakeLists.txt index 4d0468e3962..96832e6a842 100644 --- a/barretenberg/cpp/src/CMakeLists.txt +++ b/barretenberg/cpp/src/CMakeLists.txt @@ -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() diff --git a/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt b/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt index dc17b35d001..031861aa4c2 100644 --- a/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt +++ b/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt @@ -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() \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp b/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp index 545f4996728..f3ad6e648e8 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp @@ -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; } diff --git a/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp b/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp index fa1808adf9c..f45cbdb8df8 100644 --- a/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp +++ b/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp @@ -6,6 +6,9 @@ #include #include #include +#ifdef CHECK_CIRCUIT_STACKTRACES +#include +#endif namespace bb { @@ -32,6 +35,31 @@ namespace bb { * We should only do this if it becomes necessary or convenient. */ +#ifdef CHECK_CIRCUIT_STACKTRACES +struct BbStackTrace : backward::StackTrace { + BbStackTrace() + { + load_here(32); + // NOTE these only make sense when running tests + skip_n_firsts(2); + for (size_t i = 0; i < 9; i++) { + _stacktrace.pop_back(); + } + } +}; +struct StackTraces { + std::vector stack_traces; + void populate() { stack_traces.emplace_back(); } + void print(size_t gate_idx) const { backward::Printer{}.print(stack_traces.at(gate_idx)); } + // Don't interfere with equality semantics of structs that include this in debug builds + bool operator==(const StackTraces& other) const + { + static_cast(other); + return true; + } +}; +#endif + /** * @brief Basic structure for storing gate data in a builder * @@ -46,6 +74,11 @@ template class ExecutionTr using Selectors = std::array; using Wires = std::array; +#ifdef CHECK_CIRCUIT_STACKTRACES + // If enabled, we keep slow stack traces to be able to correlate gates with code locations where they were added + StackTraces stack_traces; +#endif + Wires wires; // vectors of indices into a witness variables array Selectors selectors; bool has_ram_rom = false; // does the block contain RAM/ROM gates @@ -83,6 +116,9 @@ template class StandardArith { public: void populate_wires(const uint32_t& idx_1, const uint32_t& idx_2, const uint32_t& idx_3) { +#ifdef CHECK_CIRCUIT_STACKTRACES + this->stack_traces.populate(); +#endif this->wires[0].emplace_back(idx_1); this->wires[1].emplace_back(idx_2); this->wires[2].emplace_back(idx_3); @@ -130,6 +166,9 @@ template class UltraArith { public: void populate_wires(const uint32_t& idx_1, const uint32_t& idx_2, const uint32_t& idx_3, const uint32_t& idx_4) { +#ifdef CHECK_CIRCUIT_STACKTRACES + this->stack_traces.populate(); +#endif this->wires[0].emplace_back(idx_1); this->wires[1].emplace_back(idx_2); this->wires[2].emplace_back(idx_3); @@ -246,6 +285,9 @@ template class UltraHonkArith { public: void populate_wires(const uint32_t& idx_1, const uint32_t& idx_2, const uint32_t& idx_3, const uint32_t& idx_4) { +#ifdef CHECK_CIRCUIT_STACKTRACES + this->stack_traces.populate(); +#endif this->wires[0].emplace_back(idx_1); this->wires[1].emplace_back(idx_2); this->wires[2].emplace_back(idx_3); From 50491fd36ab943524b78c47dc751be62ff44bd20 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 10:04:25 +0000 Subject: [PATCH 04/11] more stack printing --- .../circuit_checker/ultra_circuit_checker.cpp | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp index e213801cb22..f15afe970d2 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp @@ -40,7 +40,7 @@ template 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) { info("Failed at block idx = ", block_idx); return false; } @@ -49,7 +49,7 @@ template 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; } @@ -78,51 +78,56 @@ bool UltraCircuitChecker::check_block(Builder& builder, populate_values(builder, block, values, tag_data, memory_data, idx); result = result && check_relation(values, params); - if (result == false) { + if (!result) { info("Failed Arithmetic relation at row idx = ", idx); return false; } result = result && check_relation(values, params); - if (result == false) { + if (!result) { info("Failed Elliptic relation at row idx = ", idx); return false; } result = result && check_relation(values, params); - if (result == false) { + if (!result) { info("Failed Auxiliary relation at row idx = ", idx); return false; } result = result && check_relation(values, params); - if (result == false) { + if (!result) { info("Failed DeltaRangeConstraint relation at row idx = ", idx); return false; } result = result && check_lookup(values, lookup_hash_table); - if (result == false) { + if (!result) { info("Failed Lookup check relation at row idx = ", idx); return false; } if constexpr (IsMegaBuilder) { result = result && check_relation(values, params); - if (result == false) { + if (!result) { info("Failed PoseidonInternal relation at row idx = ", idx); return false; } result = result && check_relation(values, params); - if (result == false) { + if (!result) { info("Failed PoseidonExternal relation at row idx = ", idx); return false; } result = result && check_databus_read(values, builder); - if (result == false) { + if (!result) { info("Failed databus read at row idx = ", idx); return false; } } - if (result == false) { + if (!result) { info("Failed at row idx = ", idx); return false; } +#ifdef CHECK_CIRCUIT_STACKTRACES + if (!result) { + block.stack_traces.print(idx); + } +#endif } return result; From f3087b14499af69a85b6366778d803098408ff8b Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 11:34:44 +0000 Subject: [PATCH 05/11] checkpoint, standard working except for assert_equal --- .../circuit_checker/ultra_circuit_checker.cpp | 44 ++++++++----------- .../standard_circuit_builder.cpp | 1 - 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp index f15afe970d2..55e7d3f44a1 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp @@ -71,6 +71,14 @@ 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) { @@ -79,55 +87,41 @@ bool UltraCircuitChecker::check_block(Builder& builder, result = result && check_relation(values, params); if (!result) { - info("Failed Arithmetic relation at row idx = ", idx); - return false; + return report_fail("Failed Arithmetic relation at row idx = ", idx); } result = result && check_relation(values, params); if (!result) { - info("Failed Elliptic relation at row idx = ", idx); - return false; + return report_fail("Failed Elliptic relation at row idx = ", idx); } result = result && check_relation(values, params); if (!result) { - info("Failed Auxiliary relation at row idx = ", idx); - return false; + return report_fail("Failed Auxiliary relation at row idx = ", idx); } result = result && check_relation(values, params); if (!result) { - info("Failed DeltaRangeConstraint relation at row idx = ", idx); - return false; + return report_fail("Failed DeltaRangeConstraint relation at row idx = ", idx); } result = result && check_lookup(values, lookup_hash_table); if (!result) { - info("Failed Lookup check relation at row idx = ", idx); - return false; + return report_fail("Failed Lookup check relation at row idx = ", idx); } if constexpr (IsMegaBuilder) { result = result && check_relation(values, params); if (!result) { - info("Failed PoseidonInternal relation at row idx = ", idx); - return false; + return report_fail("Failed PoseidonInternal relation at row idx = ", idx); } result = result && check_relation(values, params); if (!result) { - info("Failed PoseidonExternal relation at row idx = ", idx); - return false; + return report_fail("Failed PoseidonExternal relation at row idx = ", idx); } result = result && check_databus_read(values, builder); if (!result) { - info("Failed databus read at row idx = ", idx); - return false; + return report_fail("Failed databus read at row idx = ", idx); } } if (!result) { - info("Failed at row idx = ", idx); - return false; + return report_fail("Failed at row idx = ", idx); } -#ifdef CHECK_CIRCUIT_STACKTRACES - if (!result) { - block.stack_traces.print(idx); - } -#endif } return result; @@ -228,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); diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp index 351c3ed0eb4..ad0938731d5 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp @@ -279,7 +279,6 @@ std::vector StandardCircuitBuilder_::decompose_into_base4_accumula accumulator_idx = new_accumulator_idx; } } - this->assert_equal(witness_index, accumulator_idx, msg); return accumulators; } From 91924bb8bdbff8244266e9828804cf08a38f3855 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 12:09:43 +0000 Subject: [PATCH 06/11] ultra circuit builder checker --- .../circuit_checker/ultra_circuit_builder.test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp index 483f1c52966..6f814a95844 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp @@ -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); @@ -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); @@ -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); @@ -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); From 49c79d7365f5b25683d988ddead012f7e283a600 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 12:17:15 +0000 Subject: [PATCH 07/11] preset and off by default --- barretenberg/cpp/CMakeLists.txt | 4 ++-- barretenberg/cpp/CMakePresets.json | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/barretenberg/cpp/CMakeLists.txt b/barretenberg/cpp/CMakeLists.txt index 999072502fc..9e1ad5d4ced 100644 --- a/barretenberg/cpp/CMakeLists.txt +++ b/barretenberg/cpp/CMakeLists.txt @@ -34,8 +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) -# TODO turn off -option(CHECK_CIRCUIT_STACKTRACES "Enable (slow) stack traces for check circuit" ON) +# 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.") diff --git a/barretenberg/cpp/CMakePresets.json b/barretenberg/cpp/CMakePresets.json index 4b4639a1769..bf66966c0ca 100644 --- a/barretenberg/cpp/CMakePresets.json +++ b/barretenberg/cpp/CMakePresets.json @@ -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", From a79187f2546ea2c9436ca3aa1496fbd3970d238a Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 13:34:58 +0000 Subject: [PATCH 08/11] clang16-dbg-fast-circuit-check-traces --- barretenberg/cpp/CMakePresets.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/barretenberg/cpp/CMakePresets.json b/barretenberg/cpp/CMakePresets.json index bf66966c0ca..b18d0604fd6 100644 --- a/barretenberg/cpp/CMakePresets.json +++ b/barretenberg/cpp/CMakePresets.json @@ -415,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", @@ -490,12 +495,7 @@ "configurePreset": "wasm", "inheritConfigureEnvironment": true, "jobs": 0, - "targets": [ - "barretenberg.wasm", - "barretenberg", - "wasi", - "env" - ] + "targets": ["barretenberg.wasm", "barretenberg", "wasi", "env"] }, { "name": "wasm-dbg", From e5fe1396034c42c87deca84a0efd52f33c059050 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 09:48:57 -0400 Subject: [PATCH 09/11] Delete barretenberg/cpp/scripts/analyze_compilation_time.sh --- .../cpp/scripts/analyze_compilation_time.sh | 22 ------------------- 1 file changed, 22 deletions(-) delete mode 100644 barretenberg/cpp/scripts/analyze_compilation_time.sh diff --git a/barretenberg/cpp/scripts/analyze_compilation_time.sh b/barretenberg/cpp/scripts/analyze_compilation_time.sh deleted file mode 100644 index 154a2ec336b..00000000000 --- a/barretenberg/cpp/scripts/analyze_compilation_time.sh +++ /dev/null @@ -1,22 +0,0 @@ -#!/usr/bin/env bash -set -eu - -PRESET="${1:-wasm-threads}" -TARGET="${2:-barretenberg.wasm}" - -# Move above script dir. -cd $(dirname $0)/.. - -if ! [ -d ~/ClangBuildAnalyzer ] ; then - git clone https://github.com/aras-p/ClangBuildAnalyzer ~/ClangBuildAnalyzer - pushd ~/ClangBuildAnalyzer - mkdir -p build && cd build && cmake .. && make -j - popd -fi -rm -rf build-$PRESET-compiler-profile -cmake -DCMAKE_CXX_FLAGS=-ftime-trace --preset "$PRESET" -Bbuild-$PRESET-compiler-profile -cd build-$PRESET-compiler-profile -ninja $TARGET - -~/ClangBuildAnalyzer/build/ClangBuildAnalyzer --all . compile-profile.json -~/ClangBuildAnalyzer/build/ClangBuildAnalyzer --analyze compile-profile.json From bbc0d9ccd6c0ac51dd524001b78b2513571be663 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 14:37:29 +0000 Subject: [PATCH 10/11] readme --- barretenberg/README.md | 65 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/barretenberg/README.md b/barretenberg/README.md index 57cc64e37c0..3907614d0ee 100644 --- a/barretenberg/README.md +++ b/barretenberg/README.md @@ -287,3 +287,68 @@ python3 -m http.server ``` and tunnel the port through ssh. + +### Debugging Verifification Failures + +The CicuitChecker::check_circuit function is used to get the gate index and block information about a failing circuit constraint. +If you are in a scenario where you have a failing call to check_circuit and wish to get more information out of it than just the gate index, you can use this feature to get a stack trace, see example below. + +Usage instructions: +- On ubuntu (or our mainframe accounts) use `sudo apt-get install libdw-dev` to support trace printing +- Use `cmake --preset clang16-dbg-fast-circuit-check-traces` and `cmake --build --preset clang16-dbg-fast-circuit-check-traces` to enable the backward-cpp dependency through the CHECK_CIRCUIT_STACKTRACES CMake variable. +- Run any case where you have a failing check_circuit call, you will now have a stack trace illuminating where this constraint was added in code. + +Caveats: +- This works best for code that is not overly generic, i.e. where just the sequence of function calls carries a lot of information. It is possible to tag extra data along with the stack trace, this can be done as a followup, please leave feedback if desired. +- There are certain functions like `assert_equals` that can cause gates that occur _before_ them to fail. If this would be useful to automatically report, please leave feedback. + +Example: +``` +[ RUN ] standard_circuit_constructor.test_check_circuit_broken +Stack trace (most recent call last): +#4 Source "_deps/gtest-src/googletest/src/gtest.cc", line 2845, in Run + 2842: if (!Test::HasFatalFailure() && !Test::IsSkipped()) { + 2843: // This doesn't throw as all user code that can throw are wrapped into + 2844: // exception handling code. + >2845: test->Run(); + 2846: } + 2847: + 2848: if (test != nullptr) { +#3 Source "_deps/gtest-src/googletest/src/gtest.cc", line 2696, in Run + 2693: // GTEST_SKIP(). + 2694: if (!HasFatalFailure() && !IsSkipped()) { + 2695: impl->os_stack_trace_getter()->UponLeavingGTest(); + >2696: internal::HandleExceptionsInMethodIfSupported(this, &Test::TestBody, + 2697: "the test body"); + 2698: } +#2 | Source "_deps/gtest-src/googletest/src/gtest.cc", line 2657, in HandleSehExceptionsInMethodIfSupported + | 2655: #if GTEST_HAS_EXCEPTIONS + | 2656: try { + | >2657: return HandleSehExceptionsInMethodIfSupported(object, method, location); + | 2658: } catch (const AssertionException&) { // NOLINT + | 2659: // This failure was reported already. + Source "_deps/gtest-src/googletest/src/gtest.cc", line 2621, in HandleExceptionsInMethodIfSupported + 2618: } + 2619: #else + 2620: (void)location; + >2621: return (object->*method)(); + 2622: #endif // GTEST_HAS_SEH + 2623: } +#1 Source "/mnt/user-data/adam/aztec-packages/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_builder.test.cpp", line 464, in TestBody + 461: uint32_t d_idx = circuit_constructor.add_variable(d); + 462: circuit_constructor.create_add_gate({ a_idx, b_idx, c_idx, fr::one(), fr::one(), fr::neg_one(), fr::zero() }); + 463: + > 464: circuit_constructor.create_add_gate({ d_idx, c_idx, a_idx, fr::one(), fr::neg_one(), fr::neg_one(), fr::zero() }); + 465: + 466: bool result = CircuitChecker::check(circuit_constructor); + 467: EXPECT_EQ(result, false); +#0 Source "/mnt/user-data/adam/aztec-packages/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp", line 22, in create_add_gate + 19: { + 20: this->assert_valid_variables({ in.a, in.b, in.c }); + 21: + > 22: blocks.arithmetic.populate_wires(in.a, in.b, in.c); + 23: blocks.arithmetic.q_m().emplace_back(FF::zero()); + 24: blocks.arithmetic.q_1().emplace_back(in.a_scaling); + 25: blocks.arithmetic.q_2().emplace_back(in.b_scaling); +gate number4 +``` \ No newline at end of file From b3c8175126e698b57c0742142ca29a773f113a08 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 4 Jun 2024 15:11:34 +0000 Subject: [PATCH 11/11] sync --- barretenberg/cpp/cmake/module.cmake | 12 ++++++++++++ .../cpp/src/barretenberg/eccvm/eccvm_prover.cpp | 1 - .../arithmetization/arithmetization.hpp | 13 ++++--------- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/barretenberg/cpp/cmake/module.cmake b/barretenberg/cpp/cmake/module.cmake index 6ba42e9ef35..8365188dc2f 100644 --- a/barretenberg/cpp/cmake/module.cmake +++ b/barretenberg/cpp/cmake/module.cmake @@ -255,6 +255,18 @@ function(barretenberg_module MODULE_NAME) benchmark::benchmark ${TBB_IMPORTED_TARGETS} ) + if(CHECK_CIRCUIT_STACKTRACES) + target_link_libraries( + ${BENCHMARK_NAME}_bench_objects + PUBLIC + Backward::Interface + ) + target_link_options( + ${BENCHMARK_NAME}_bench + PRIVATE + -ldw -lelf + ) + endif() # enable msgpack downloading via dependency (solves race condition) add_dependencies(${BENCHMARK_NAME}_bench_objects msgpack-c) diff --git a/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp b/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp index 0633561940b..43cd7248f11 100644 --- a/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp +++ b/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp @@ -33,7 +33,6 @@ ECCVMProver::ECCVMProver(CircuitBuilder& builder, const std::shared_ptr(key->circuit_size); - info("circuit_size"); transcript->send_to_verifier("circuit_size", circuit_size); } diff --git a/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp b/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp index f45cbdb8df8..4d3514f20cf 100644 --- a/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp +++ b/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp @@ -37,15 +37,7 @@ namespace bb { #ifdef CHECK_CIRCUIT_STACKTRACES struct BbStackTrace : backward::StackTrace { - BbStackTrace() - { - load_here(32); - // NOTE these only make sense when running tests - skip_n_firsts(2); - for (size_t i = 0; i < 9; i++) { - _stacktrace.pop_back(); - } - } + BbStackTrace() { load_here(32); } }; struct StackTraces { std::vector stack_traces; @@ -98,6 +90,9 @@ template class ExecutionTr for (auto& p : selectors) { p.reserve(size_hint); } +#ifdef CHECK_CIRCUIT_STACKTRACES + stack_traces.stack_traces.reserve(size_hint); +#endif } uint32_t get_fixed_size() const { return fixed_size; }