Fix tracy (stellar#4500)

This change returns tracy to kinda-working on soroban. It does not work on soroban v21 (you still just get an opaque span for such InvokeHostFunction calls) but does: - Build for soroban v22 - Even when running with `CXX=clang++-12 CXXFLAGS=-stdlib=libc++` as in CI - Work for soroban v22 (not-crash and actually produce tracy sub-spans for soroban v22) - Roll back the stale cost-adjustments made for SOROBAN_TEST_EXTRA_PROTOCOL=22 mode - As a bonus, simplify lifetime/startup and eliminate a layer of locking in the memory-tracking code There were three major issues to deal with: - We weren't passing relevant toolchain variables from the Makefile through to cargo invocations, at all. - We weren't _setting_ CXXSTDLIB which is a special one that build.rs needs to build C++ code. - We were using tracy in `manual-lifetime` mode because older versions of the rust `tracy-client` crate required it, and this just can't really work when there are multiple copies of `tracy-client` linked into the final executable, they fight over who gets to initialize tracy. By abandoning those old versions of `tracy-client` and only enabling new ones that allow _turning off_ `manual-lifetime`, we're in a much better state. The multiple copies of `tracy-client` all expect tracy to initialize itself, and it does so, and everything's fine. I did _not_ yet conditionalize-out the memory-tracking code. I might push a further commit here to do so. It seems like it's fairly expensive to have on by default when using tracy, and I'm not sure anyone benefits from it most of the time.
anupsdf · Oct 18, 2024 · 2493152 · 2493152
2 parents d60775c + 702be85
commit 2493152
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 128 deletions.
diff --git a/Builds/VisualStudio/stellar-core.vcxproj b/Builds/VisualStudio/stellar-core.vcxproj
@@ -325,7 +325,7 @@ exit /b 0
       <FunctionLevelLinking>true</FunctionLevelLinking>
       <IntrinsicFunctions>true</IntrinsicFunctions>
       <SDLCheck>true</SDLCheck>
-      <PreprocessorDefinitions>CEREAL_THREAD_SAFE;USE_POSTGRES;ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION=1;USE_SPDLOG;FMT_HEADER_ONLY=1;BUILD_TESTS;WIN32_LEAN_AND_MEAN;NOMINMAX;ASIO_STANDALONE;_WINSOCK_DEPRECATED_NO_WARNINGS;SODIUM_STATIC;ASIO_SEPARATE_COMPILATION;ASIO_ERROR_CATEGORY_NOEXCEPT=noexcept;TRACY_ENABLE;TRACY_ON_DEMAND;TRACY_NO_BROADCAST;TRACY_ONLY_LOCALHOST;TRACY_DELAYED_INIT;TRACY_MANUAL_LIFETIME;USE_TRACY;_CRT_SECURE_NO_WARNINGS;_WIN32_WINNT=0x0601;WIN32;_MBCS;_CRT_NONSTDC_NO_DEPRECATE;YY_NO_UNISTD_H;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>CEREAL_THREAD_SAFE;USE_POSTGRES;ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION=1;USE_SPDLOG;FMT_HEADER_ONLY=1;BUILD_TESTS;WIN32_LEAN_AND_MEAN;NOMINMAX;ASIO_STANDALONE;_WINSOCK_DEPRECATED_NO_WARNINGS;SODIUM_STATIC;ASIO_SEPARATE_COMPILATION;ASIO_ERROR_CATEGORY_NOEXCEPT=noexcept;TRACY_ENABLE;TRACY_ON_DEMAND;TRACY_NO_BROADCAST;TRACY_ONLY_LOCALHOST;TRACY_DELAYED_INIT;USE_TRACY;_CRT_SECURE_NO_WARNINGS;_WIN32_WINNT=0x0601;WIN32;_MBCS;_CRT_NONSTDC_NO_DEPRECATE;YY_NO_UNISTD_H;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <AdditionalIncludeDirectories>src;../../src;../../lib;../../lib/tracy/public/tracy;../../lib/spdlog/include;../../lib/libmedida/src;../../lib/soci/src/core;../../lib/autocheck/include;../../lib/cereal/include;../../lib/asio/asio/include;../../lib/xdrpp;../../lib/libsodium/src/libsodium/include;../../lib/fmt/include;../../lib/util;../..;src/$(Configuration)/generated;../../lib/sqlite;c:\Program Files\PostgreSQL\15\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <BrowseInformation>false</BrowseInformation>
       <MultiProcessorCompilation>true</MultiProcessorCompilation>
@@ -390,7 +390,7 @@ exit /b 0
       <FunctionLevelLinking>true</FunctionLevelLinking>
       <IntrinsicFunctions>true</IntrinsicFunctions>
       <SDLCheck>true</SDLCheck>
-      <PreprocessorDefinitions>CEREAL_THREAD_SAFE;USE_POSTGRES;USE_SPDLOG;FMT_HEADER_ONLY=1;BUILD_TESTS;WIN32_LEAN_AND_MEAN;NOMINMAX;ASIO_STANDALONE;_WINSOCK_DEPRECATED_NO_WARNINGS;SODIUM_STATIC;ASIO_SEPARATE_COMPILATION;ASIO_ERROR_CATEGORY_NOEXCEPT=noexcept;TRACY_ENABLE;TRACY_ON_DEMAND;TRACY_NO_BROADCAST;TRACY_ONLY_LOCALHOST;TRACY_DELAYED_INIT;TRACY_MANUAL_LIFETIME;USE_TRACY;_CRT_SECURE_NO_WARNINGS;_WIN32_WINNT=0x0601;WIN32;_MBCS;_CRT_NONSTDC_NO_DEPRECATE;YY_NO_UNISTD_H;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>CEREAL_THREAD_SAFE;USE_POSTGRES;USE_SPDLOG;FMT_HEADER_ONLY=1;BUILD_TESTS;WIN32_LEAN_AND_MEAN;NOMINMAX;ASIO_STANDALONE;_WINSOCK_DEPRECATED_NO_WARNINGS;SODIUM_STATIC;ASIO_SEPARATE_COMPILATION;ASIO_ERROR_CATEGORY_NOEXCEPT=noexcept;TRACY_ENABLE;TRACY_ON_DEMAND;TRACY_NO_BROADCAST;TRACY_ONLY_LOCALHOST;TRACY_DELAYED_INIT;USE_TRACY;_CRT_SECURE_NO_WARNINGS;_WIN32_WINNT=0x0601;WIN32;_MBCS;_CRT_NONSTDC_NO_DEPRECATE;YY_NO_UNISTD_H;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <AdditionalIncludeDirectories>src;../../src;../../lib;../../lib/tracy/public/tracy;../../lib/spdlog/include;../../lib/libmedida/src;../../lib/soci/src/core;../../lib/autocheck/include;../../lib/cereal/include;../../lib/asio/asio/include;../../lib/xdrpp;../../lib/libsodium/src/libsodium/include;../../lib/fmt/include;../../lib/util;../..;src/$(Configuration)/generated;../../lib/sqlite;c:\Program Files\PostgreSQL\15\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <BrowseInformation>false</BrowseInformation>
       <MultiProcessorCompilation>true</MultiProcessorCompilation>

diff --git a/configure.ac b/configure.ac
@@ -347,7 +347,7 @@ AC_ARG_ENABLE(tracy,
     AS_HELP_STRING([--enable-tracy],
         [Enable 'tracy' profiler/tracer client stub]))
 AM_CONDITIONAL(USE_TRACY, [test x$enable_tracy = xyes])
-tracy_CFLAGS='-DTRACY_ENABLE -DTRACY_ON_DEMAND -DTRACY_NO_BROADCAST -DTRACY_ONLY_LOCALHOST -DTRACY_ONLY_IPV4 -DTRACY_DELAYED_INIT -DTRACY_MANUAL_LIFETIME'
+tracy_CFLAGS='-DTRACY_ENABLE -DTRACY_ON_DEMAND -DTRACY_NO_BROADCAST -DTRACY_ONLY_LOCALHOST -DTRACY_ONLY_IPV4 -DTRACY_DELAYED_INIT'
 if test x"$enable_tracy" = xyes; then
     case "${host_os}" in
         *darwin*)

diff --git a/src/Makefile.am b/src/Makefile.am
@@ -8,8 +8,54 @@ include $(srcdir)/src.mk
 
 noinst_HEADERS = $(SRC_H_FILES)
 
+# We sometimes specify a CXXFLAGS setting like -stdlib=libc++ that causes the
+# stdlib to change. When this makes it way down to Rust code building C++ code
+# for one of its libraries (specifically tracy-client-sys) this needs to be
+# accompanied by an _explicit_ instruction to invoke the linker with -lc++. This
+# is done by setting the CXXSTDLIB flag, which Rust's C++-building machinery is
+# sensitive to. Rust passes-on, but does not look inside, CXXFLAGS itself to
+# realize that it needs this setting.
+CXXSTDLIB := $(if $(findstring -stdlib=libc++,$(CXXFLAGS)),c++,$(if $(findstring -stdlib=libstdc++,$(CXXFLAGS)),stdc++,))
+
+# If we are building with -fsanitize=address, any build.rs-built C++ code will
+# depend on asan, so we need to tell rustc to link with in the asan runtime as
+# well. Interestingly, passing -Clink-arg=-fsanitize=address to rustc does not
+# work, despite being the recommended approach, it seems rustc passes some other
+# flags to the linker that conflict with -fsanitize=address.
+RUSTFLAGS_ASAN := $(if $(findstring -fsanitize=address,$(CXXFLAGS)),-Clink-arg=-lasan,)
+
 if USE_TRACY
+# NB: this unfortunately long list has to be provided here and kept in sync with
+# the list of features for tracy-client in src/rust/Cargo.toml.  This is because
+# the soroban sub-crates are built independently and know nothing about
+# src/rust/Cargo.toml
+#
+# NB also: this list does _not_ and should not include the manual-lifetime
+# feature. Versions of the rust tracy-client crate before 0.17 (eg. version
+# 0.16.4 which we shipped in soroban 21.x) always used the C++ tracy code in
+# manual-lifetime mode, and thus required careful coordination with
+# stellar-core's C++ code to start and stop tracy correctly. However this became
+# intractable when we started building multiple soroban libraries as .rlibs with
+# separate copies of tracy-client simultaneously, as they would each have their
+# own rust-metadata-hash-qualified copy of a rust global variable tracking the
+# initialization state of tracy (and of course tracy's C++ code itself cannot be
+# started multiple times idempotently in manual-lifetime mode, that would be too
+# easy). So we have now disabled tracy-client (0.16.4) in soroban 21.x and only
+# enabled it in soroban 22.x (and later), which has tracy-client 0.17, which
+# finally supports _turning off_ manual-lifetime mode. Once off, tracy's C++
+# code initializes itself on its own, and everything works fine. There is only
+# one copy of tracy in the final linked executable and it has a single atomic
+# pointer controlling its initialization state.
 CARGO_FEATURE_TRACY = --features tracy
+CARGO_FEATURE_TRACY += --features tracy-client/enable
+CARGO_FEATURE_TRACY += --features tracy-client/ondemand
+CARGO_FEATURE_TRACY += --features tracy-client/delayed-init
+CARGO_FEATURE_TRACY += --features tracy-client/system-tracing
+CARGO_FEATURE_TRACY += --features tracy-client/sampling
+CARGO_FEATURE_TRACY += --features tracy-client/code-transfer
+CARGO_FEATURE_TRACY += --features tracy-client/timer-fallback
+CARGO_FEATURE_TRACY += --features tracy-client/only-localhost
+CARGO_FEATURE_TRACY += --features tracy-client/only-ipv4
 else
 CARGO_FEATURE_TRACY =
 endif
@@ -167,14 +213,22 @@ $(RUST_DEP_TREE_STAMP): $(wildcard rust/soroban/*/Cargo.*) Makefile
 # them back together. Or something. Anyways it works and is the only portable
 # and robust solution we've found to preventing the StableCrateId collision.
 #
-# We also have to be somewhat selective about the versions we pass the
-# `next` feature to (only the most recent soroban) and the versions we
-# pass the `tracy` feature to (only those post-p22 versions that support
-# the tracy delayed-init feature).
+# We also have to be somewhat selective about the versions we pass the `next`
+# feature to (only the most recent soroban) and the versions we pass the `tracy`
+# features to (only those post-p22 versions that support the tracy delayed-init
+# feature).
 #
 # We also have to sequentialize the builds of the sorobans after the
-# dep-tree-stamp files and before the stellar-core build, because rustup
-# gets invoked and it is not concurrency-safe.
+# dep-tree-stamp files and before the stellar-core build, because rustup gets
+# invoked and it is not concurrency-safe.
+#
+# We also have to set pass all the CC, CXX, CFLAGS and CXXFLAGS we were called
+# with explicitly, because make does not implicitly export them to its rules,
+# but cargo needs them to build C++ files in build.rs files.
+#
+# We also have to pass CXXSTDLIB to those build.rs files, because they are
+# sensitive to CXXFLAGS but also don't inspect them to see if they're setting
+# -stdlib=libc++ or -stdlib=libstdc++
 $(SOROBAN_LIBS_STAMP): $(wildcard rust/soroban/p*/Cargo.lock) Makefile $(RUST_DEP_TREE_STAMP) $(SRC_RUST_FILES)
 	for proto in $(ALL_SOROBAN_PROTOCOLS) ; \
 	do \
@@ -191,7 +245,8 @@ $(SOROBAN_LIBS_STAMP): $(wildcard rust/soroban/p*/Cargo.lock) Makefile $(RUST_DE
 			;; \
 		esac ; \
 		cd $(abspath $(RUST_BUILD_DIR))/soroban/$$proto && \
-		RUSTFLAGS=-Cmetadata=$$proto \
+		CC="$(CC)" CXX="$(CXX)" LD="$(LD)" CFLAGS="$(CFLAGS)" CXXFLAGS="$(CXXFLAGS)" CXXSTDLIB="$(CXXSTDLIB)" LDFLAGS="$(LDFLAGS)" \
+		RUSTFLAGS="-Cmetadata=$$proto $(RUSTFLAGS_ASAN)" \
 		CARGO_NET_GIT_FETCH_WITH_CLI=true cargo build \
 			--package soroban-env-host \
 			--$(RUST_PROFILE) \
@@ -215,6 +270,8 @@ $(SOROBAN_LIBS_STAMP): $(wildcard rust/soroban/p*/Cargo.lock) Makefile $(RUST_DE
 # rustc` invocation, along with `-L dependency=...` flags to tell cargo where to
 # find indirect deps of those .rlibs.
 $(LIBRUST_STELLAR_CORE): $(RUST_HOST_DEPFILES) $(SRC_RUST_FILES) Makefile $(SOROBAN_LIBS_STAMP)
+	CC="$(CC)" CXX="$(CXX)" LD="$(LD)" CFLAGS="$(CFLAGS)" CXXFLAGS="$(CXXFLAGS)" CXXSTDLIB="$(CXXSTDLIB)" LDFLAGS="$(LDFLAGS)" \
+	RUSTFLAGS="$(RUSTFLAGS_ASAN)" \
 	CARGO_NET_GIT_FETCH_WITH_CLI=true cargo rustc \
 		--package stellar-core \
 		--$(RUST_PROFILE) \
@@ -290,7 +347,7 @@ distclean-local: fuzz-clean
 endif # USE_AFL_FUZZ
 
 clean-local:
-	rm -rf $(top_builddir)/target $(top_builddir)/src/rust/soroban/*/target
+	rm -rf $(top_builddir)/target $(top_builddir)/src/rust/soroban/*/target $(RUST_DEP_TREE_STAMP) $(SOROBAN_LIBS_STAMP)
 
 CLEANFILES = $(BUILT_SOURCES) *~ */*~ stellar*.log
 MAINTAINERCLEANFILES = $(srcdir)/Makefile.in $(srcdir)/*~ $(srcdir)/*/*~

diff --git a/src/main/main.cpp b/src/main/main.cpp
@@ -280,24 +280,6 @@ checkStellarCoreMajorVersionProtocolIdentity()
 
 #ifdef USE_TRACY
 
-std::mutex TRACY_MUTEX;
-static int TRACY_NOT_STARTED = 0;
-static int TRACY_RUNNING = 1;
-static int TRACY_STOPPED = 2;
-static int TRACY_STATE = TRACY_NOT_STARTED;
-
-// Call this only with mutex held
-bool
-tracyEnabled(std::lock_guard<std::mutex> const& _guard)
-{
-    if (TRACY_STATE == TRACY_NOT_STARTED)
-    {
-        stellar::rust_bridge::start_tracy();
-        TRACY_STATE = TRACY_RUNNING;
-    }
-    return TRACY_STATE != TRACY_STOPPED;
-}
-
 #ifdef __has_feature
 #if __has_feature(address_sanitizer)
 #define ASAN_ENABLED
@@ -313,45 +295,31 @@ void*
 operator new(std::size_t count)
 {
     auto ptr = malloc(count);
-    std::lock_guard<std::mutex> guard(TRACY_MUTEX);
-    if (tracyEnabled(guard))
-    {
-        TracyAlloc(ptr, count);
-    }
+    // "Secure" here means "tolerant of calls outside the
+    // lifeitme of the tracy client".
+    TracySecureAlloc(ptr, count);
     return ptr;
 }
 
 void
 operator delete(void* ptr) noexcept
 {
-    std::lock_guard<std::mutex> guard(TRACY_MUTEX);
-    if (tracyEnabled(guard))
-    {
-        TracyFree(ptr);
-    }
+    TracySecureFree(ptr);
     free(ptr);
 }
 
 void*
 operator new[](std::size_t count)
 {
     auto ptr = malloc(count);
-    std::lock_guard<std::mutex> guard(TRACY_MUTEX);
-    if (tracyEnabled(guard))
-    {
-        TracyAlloc(ptr, count);
-    }
+    TracySecureAlloc(ptr, count);
     return ptr;
 }
 
 void
 operator delete[](void* ptr) noexcept
 {
-    std::lock_guard<std::mutex> guard(TRACY_MUTEX);
-    if (tracyEnabled(guard))
-    {
-        TracyFree(ptr);
-    }
+    TracySecureFree(ptr);
     free(ptr);
 }
 #endif // ASAN_ENABLED
@@ -368,11 +336,6 @@ main(int argc, char* const* argv)
     // At least print a backtrace in any circumstance
     // that would call std::terminate
     std::set_terminate(printBacktraceAndAbort);
-#ifdef USE_TRACY
-    // The rust tracy client library is fussy about trying
-    // to own the tracy startup path.
-    rust_bridge::start_tracy();
-#endif
     Logging::init();
     if (sodium_init() != 0)
     {
@@ -388,12 +351,5 @@ main(int argc, char* const* argv)
     checkXDRFileIdentity();
 
     int res = handleCommandLine(argc, argv);
-#ifdef USE_TRACY
-    {
-        std::lock_guard<std::mutex> guard(TRACY_MUTEX);
-        ___tracy_shutdown_profiler();
-        TRACY_STATE = TRACY_STOPPED;
-    }
-#endif
     return res;
 }
diff --git a/src/rust/Cargo.toml b/src/rust/Cargo.toml
@@ -30,11 +30,15 @@ itertools = "=0.10.5"
 # There is a table on the README for https://crates.io/crates/tracy-client that
 # maps versions of tracy-client to C++ tracy releases. The lib/tracy submodule
 # of this repo should be pinned out to a matched C++ tracy release version.
+#
+# Also note: this feature list also has to be passed _on the command line_ (i.e.
+# in the Makefile.am) when we separately build the soroban sub-crates because
+# they are separate cargo invocations and do not see this list, or even this
+# file, at all.
 tracy-client = { version = "=0.17.0", features = [
     "enable",
     "ondemand",
     "delayed-init",
-    "manual-lifetime",
     "system-tracing",
     "sampling",
     "code-transfer",

diff --git a/src/rust/src/lib.rs b/src/rust/src/lib.rs
@@ -176,7 +176,6 @@ mod rust_bridge {
     // The extern "Rust" block declares rust stuff we're going to export to C++.
     #[namespace = "stellar::rust_bridge"]
     extern "Rust" {
-        fn start_tracy();
         fn to_base64(b: &CxxVector<u8>, mut s: Pin<&mut CxxString>);
         fn from_base64(s: &CxxString, mut b: Pin<&mut CxxVector<u8>>);
         fn check_sensible_soroban_config_for_protocol(core_max_proto: u32);
@@ -882,64 +881,15 @@ mod test_extra_protocol {
         ledger_info.protocol_version = new_protocol;
 
         match new_protocol {
-            22 => {
-                use p22::contract::{
-                    inplace_modify_cxxbuf_encoded_type, xdr::ContractCostParams,
-                    xdr::ContractCostType as CT,
-                };
-                // We have to modify some cost parameters in the ledger info and
-                // add some new ones to simulate p22. Note that a p21
-                // ContractCostParams structure will decode properly as a p22
-                // one, so we can just use the inplace-modify helper function.
-                inplace_modify_cxxbuf_encoded_type::<ContractCostParams>(
-                    &mut ledger_info.cpu_cost_params,
-                    |params: &mut ContractCostParams| {
-                        let mut cpu = params.0.to_vec();
-
-                        // We only adjust the cost types that changed dramatically
-                        // between 21 and 22. The rest are left as-is, they're close
-                        // enough to not matter for side-by-side testing.
-
-                        cpu[CT::VmInstantiation as usize].const_term = 31271;
-                        cpu[CT::VmInstantiation as usize].linear_term = 57504;
-
-                        cpu[CT::ParseWasmInstructions as usize].const_term = 37421;
-                        cpu[CT::ParseWasmInstructions as usize].linear_term = 32;
-
-                        cpu[CT::ParseWasmFunctions as usize].const_term = 0;
-                        cpu[CT::ParseWasmFunctions as usize].linear_term = 84156;
-
-                        cpu[CT::ParseWasmDataSegmentBytes as usize].const_term = 0;
-                        cpu[CT::ParseWasmDataSegmentBytes as usize].linear_term = 14;
-
-                        params.0 = cpu
-                            .try_into()
-                            .map_err(|_| "failed to convert VecM back to array")?;
-                        Ok(())
-                    },
-                )?;
-
-                inplace_modify_cxxbuf_encoded_type::<ContractCostParams>(
-                    &mut ledger_info.mem_cost_params,
-                    |params: &mut ContractCostParams| {
-                        let mut mem = params.0.to_vec();
-
-                        mem[CT::ParseWasmInstructions as usize].const_term = 13980;
-                        mem[CT::ParseWasmInstructions as usize].linear_term = 215;
-
-                        mem[CT::ParseWasmFunctions as usize].const_term = 0;
-                        mem[CT::ParseWasmFunctions as usize].linear_term = 23056;
-
-                        mem[CT::ParseWasmDataSegmentBytes as usize].const_term = 0;
-                        mem[CT::ParseWasmDataSegmentBytes as usize].linear_term = 129;
-
-                        params.0 = mem
-                            .try_into()
-                            .map_err(|_| "failed to convert VecM back to array")?;
-                        Ok(())
-                    },
-                )?;
-            }
+            // At present no adjustments need to be made, only new costs exist
+            // in p22, not changes to existing ones.
+            //
+            // FIXME: any changes to cost types should be centralized and pulled
+            // from the same location, having multiple copies like this is bad,
+            // we already have a bug open on the problem occurring between
+            // budget defaults in soroban and upgrades.
+            //
+            // See: https://github.com/stellar/stellar-core/issues/4496
             _ => (),
         }
 
@@ -1055,10 +1005,3 @@ pub(crate) fn compute_write_fee_per_1kb(
     let hm = get_host_module_for_protocol(config_max_protocol, protocol_version)?;
     Ok((hm.compute_write_fee_per_1kb)(bucket_list_size, fee_config))
 }
-
-fn start_tracy() {
-    #[cfg(feature = "tracy")]
-    tracy_client::Client::start();
-    #[cfg(not(feature = "tracy"))]
-    panic!("called start_tracy from non-cfg(feature=\"tracy\") build")
-}