From f90b8d3a38ace3935c4f929b8a954c9df07848ed Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Wed, 4 Dec 2024 22:22:24 +0000 Subject: [PATCH 01/20] chore(profiling): gh action with sanitized native tests --- .github/workflows/profiling-native.yml | 36 ++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 .github/workflows/profiling-native.yml diff --git a/.github/workflows/profiling-native.yml b/.github/workflows/profiling-native.yml new file mode 100644 index 00000000000..1226c45fb16 --- /dev/null +++ b/.github/workflows/profiling-native.yml @@ -0,0 +1,36 @@ +name: ProfilingNativeTests + +on: + push: + paths: + - ddtrace/internal/datadog/profiling/** + branches: + - main + - 'mq-working-branch**' + pull_request: + workflow_dispatch: {} + +jobs: + profiling-native-tests: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] + sanitizer: ["safety", "thread"] + + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 1 + + - uses: actions-rust-lang/setup-rust-toolchain@v1 + - name: Install latest Rust + run: rustup update stable && rustup default stable + + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + + - name: Run profiling native tests + run: ./ddtrace/internal/datadog/profiling/build_standalone.sh --${{matrix.sanitizer}} RelWithDebInfo dd_wrapper_test From 457850e9d6746c74da7950fa5a94a643a3007984 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 16:39:35 +0000 Subject: [PATCH 02/20] fix(profiling): recreate locks after fork --- .../internal/datadog/profiling/dd_wrapper/src/profile.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp index 6f986f4896b..60ff5127418 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp @@ -201,6 +201,9 @@ Datadog::Profile::collect(const ddog_prof_Sample& sample, int64_t endtime_ns) void Datadog::Profile::postfork_child() { - profile_mtx.unlock(); + profile_mtx.~mutex(); + new (&profile_mtx) std::mutex(); + string_table_mtx.~mutex(); + new (&string_table_mtx) std::mutex(); cycle_buffers(); } From 1eb7ae523621463297e9878062700cfb8753bcb8 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 16:43:09 +0000 Subject: [PATCH 03/20] release note --- .../notes/profiling-reset-lock-51a032479617f038.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml diff --git a/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml b/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml new file mode 100644 index 00000000000..fc6966f1d3d --- /dev/null +++ b/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes an issue where calling ``unlock()`` on unlocked mutex could result in + an undefined behavior. + Fixes an issue where the child process could deadlock after ``fork()`` as + it didn't reset the locks. + From 4a99f139ebe18411a2b8a435e68a05f7bf1c5f49 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 19:33:35 +0000 Subject: [PATCH 04/20] also recreate thread_info_map_lock --- ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index c05ae45477e..ca4b7e4cbd0 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -66,6 +66,9 @@ _stack_v2_atfork_child() // The only thing we need to do at fork is to propagate the PID to echion // so we don't even reveal this function to the user _set_pid(getpid()); + // We also need to recreate the lock used by echion + thread_info_map_lock.~mutex(); + new (&thread_info_map_lock) std::mutex(); ThreadSpanLinks::postfork_child(); } From 289fc2f607765178028de9987baf16e434b5f0e4 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 15:29:52 +0000 Subject: [PATCH 05/20] chore(profiling): update fork death native test --- .../dd_wrapper/test/test_forking.cpp | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp index 1c5ecb322c2..3a617b04f24 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp @@ -3,12 +3,15 @@ #include #include +#include // Initiate an upload in a separate thread, otherwise we won't be mid-upload during fork -void -upload_in_thread() +void* +upload_in_thread(void*) { - std::thread([&]() { ddup_upload(); }).detach(); + ddup_upload(); + + return nullptr; } [[noreturn]] void @@ -24,6 +27,7 @@ profile_in_child(unsigned int num_threads, unsigned int run_time_ns, std::atomic launch_samplers(ids, 10e3, new_threads, done); std::this_thread::sleep_for(std::chrono::nanoseconds(run_time_ns)); done.store(true); + join_samplers(new_threads, done); ddup_upload(); std::exit(0); } @@ -51,7 +55,13 @@ sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) // Collect some profiling data for a few ms, then upload in a thread before forking std::this_thread::sleep_for(std::chrono::milliseconds(500)); - upload_in_thread(); + join_samplers(threads, done); + + pthread_t thread; + if (pthread_create(&thread, nullptr, upload_in_thread, nullptr) != 0) { + std::cout << "failed to create thread" << std::endl; + std::exit(1); + } // Fork, wait in the parent for child to finish pid_t pid = fork(); @@ -60,6 +70,8 @@ sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) profile_in_child(num_threads, 500e3, done); // Child profiles for 500ms } + pthread_join(thread, nullptr); + // Parent int status; done.store(true); @@ -87,7 +99,11 @@ fork_stress_test(unsigned int num_threads, unsigned int sleep_time_ns, unsigned launch_samplers(ids, sleep_time_ns, threads, done); std::vector children; - upload_in_thread(); + pthread_t thread; + if (pthread_create(&thread, nullptr, upload_in_thread, nullptr) != 0) { + std::cout << "failed to create thread" << std::endl; + std::exit(1); + } while (num_children > 0) { pid_t pid = fork(); if (pid == 0) { @@ -101,7 +117,7 @@ fork_stress_test(unsigned int num_threads, unsigned int sleep_time_ns, unsigned // Parent int status; done.store(true); - upload_in_thread(); + ddup_upload(); for (pid_t pid : children) { waitpid(pid, &status, 0); if (!is_exit_normal(status)) { From dcec7d4a13bc75ce4ec20219a981f82bafdfdaad Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 16:46:14 +0000 Subject: [PATCH 06/20] join after ddup_upload() to make it more realistic --- .../internal/datadog/profiling/dd_wrapper/test/test_forking.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp index 3a617b04f24..d576ae0fa2b 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp @@ -55,7 +55,6 @@ sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) // Collect some profiling data for a few ms, then upload in a thread before forking std::this_thread::sleep_for(std::chrono::milliseconds(500)); - join_samplers(threads, done); pthread_t thread; if (pthread_create(&thread, nullptr, upload_in_thread, nullptr) != 0) { @@ -77,6 +76,7 @@ sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) done.store(true); waitpid(pid, &status, 0); ddup_upload(); + join_samplers(threads, done); if (!is_exit_normal(status)) { std::exit(1); } From 6ff4b9cbd3f07119e7332230e1475f53fba4f3f8 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 16:46:55 +0000 Subject: [PATCH 07/20] delete as not used --- .../dd_wrapper/test/test_forking.cpp | 44 ------------------- 1 file changed, 44 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp index d576ae0fa2b..b058edb7a99 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp @@ -83,50 +83,6 @@ sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) std::exit(0); } -// Really try to break things with many forks -void -fork_stress_test(unsigned int num_threads, unsigned int sleep_time_ns, unsigned int num_children) -{ - configure("my_test_service", "my_test_env", "0.0.1", "https://127.0.0.1:9126", "cpython", "3.10.6", "3.100", 256); - std::atomic done(false); - std::vector threads; - std::vector ids; - for (unsigned int i = 0; i < num_threads; i++) { - ids.push_back(i); - } - - // ddup is configured, launch threads - launch_samplers(ids, sleep_time_ns, threads, done); - - std::vector children; - pthread_t thread; - if (pthread_create(&thread, nullptr, upload_in_thread, nullptr) != 0) { - std::cout << "failed to create thread" << std::endl; - std::exit(1); - } - while (num_children > 0) { - pid_t pid = fork(); - if (pid == 0) { - // Child - profile_in_child(num_threads, 500e3, done); // Child profiles for 500ms - } - children.push_back(pid); - num_children--; - } - - // Parent - int status; - done.store(true); - ddup_upload(); - for (pid_t pid : children) { - waitpid(pid, &status, 0); - if (!is_exit_normal(status)) { - std::exit(1); - } - } - std::exit(0); -} - TEST(ForkDeathTest, SampleInThreadsAndForkNormal) { // Same memory leak as before--whatever From 6b67da964e40d6ef9a214629ab4fff3ace41610b Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 20:00:25 +0000 Subject: [PATCH 08/20] use pthread for emulate samplers --- .../dd_wrapper/test/test_forking.cpp | 49 +++++++++++++++++-- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp index b058edb7a99..a4ff1d0d638 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp @@ -38,20 +38,59 @@ is_exit_normal(int status) return WIFEXITED(status) && WEXITSTATUS(status) == 0; } +struct EmulateSamplerArg +{ + unsigned int id; + unsigned int sleep_time_ns; + std::atomic* done; +}; + +void* +emulate_sampler_wrapper(void* argp) +{ + EmulateSamplerArg* arg = reinterpret_cast(argp); + + emulate_sampler(arg->id, arg->sleep_time_ns, *arg->done); + + return nullptr; +} + +void +launch_pthread_samplers(const std::vector& args, std::vector& threads) +{ + for (unsigned int i = 0; i < args.size(); i++) { + auto arg = args[i]; + auto pthread_handle = threads[i]; + pthread_create(&pthread_handle, nullptr, emulate_sampler_wrapper, reinterpret_cast(&arg)); + } +} + +void +join_pthread_samplers(std::vector& threads, std::atomic& done) +{ + done.store(true); + for (auto& handle : threads) { + pthread_join(handle, nullptr); + } +} + // Validates that sampling/uploads work around forks void sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) { configure("my_test_service", "my_test_env", "0.0.1", "https://127.0.0.1:9126", "cpython", "3.10.6", "3.100", 256); std::atomic done(false); - std::vector threads; + std::vector thread_handles; std::vector ids; - for (unsigned int i = 0; i < num_threads; i++) { - ids.push_back(i); + std::vector args; + + for (unsigned int i = 0; i < ids.size(); i++) { + auto id = ids[i]; + args.push_back(EmulateSamplerArg{ id, sleep_time_ns, &done }); } // ddup is configured, launch threads - launch_samplers(ids, sleep_time_ns, threads, done); + launch_pthread_samplers(args, thread_handles); // Collect some profiling data for a few ms, then upload in a thread before forking std::this_thread::sleep_for(std::chrono::milliseconds(500)); @@ -76,7 +115,7 @@ sample_in_threads_and_fork(unsigned int num_threads, unsigned int sleep_time_ns) done.store(true); waitpid(pid, &status, 0); ddup_upload(); - join_samplers(threads, done); + join_pthread_samplers(thread_handles, done); if (!is_exit_normal(status)) { std::exit(1); } From 1465f846cdd4dc553225d2ae238a66dbd947ad28 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 20:16:27 +0000 Subject: [PATCH 09/20] reset string table --- .../datadog/profiling/dd_wrapper/include/profile.hpp | 1 + .../datadog/profiling/dd_wrapper/src/profile.cpp | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp index d79c99f75f7..12adf4f35b1 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp @@ -37,6 +37,7 @@ class Profile std::deque string_storage{}; StringTable strings{}; std::mutex string_table_mtx{}; + void reset_string_table(); // Configuration SampleType type_mask{ 0 }; diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp index 60ff5127418..2416ff13b64 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp @@ -201,9 +201,19 @@ Datadog::Profile::collect(const ddog_prof_Sample& sample, int64_t endtime_ns) void Datadog::Profile::postfork_child() { + // DEV: Need to reset locks in child to avoid deadlock, and we recreate + // the data structures these lock protect. profile_mtx.~mutex(); new (&profile_mtx) std::mutex(); + cycle_buffers(); string_table_mtx.~mutex(); new (&string_table_mtx) std::mutex(); - cycle_buffers(); + reset_string_table(); +} + +void +Datadog::Profile::reset_string_table() +{ + std::lock_guard lock(string_table_mtx); + string_storage.clear(); } From cb168ed7f337b7d962d9ab4d2397a4cad3da09f7 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Thu, 5 Dec 2024 20:20:37 +0000 Subject: [PATCH 10/20] reset after fork --- ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index ca4b7e4cbd0..01587503824 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -69,6 +69,10 @@ _stack_v2_atfork_child() // We also need to recreate the lock used by echion thread_info_map_lock.~mutex(); new (&thread_info_map_lock) std::mutex(); + { + std::lock_guard lock(thread_info_map_lock); + thread_info_map.clear(); + } ThreadSpanLinks::postfork_child(); } From 7cd6df2723fb3896edeaaa6e226bc716d9eaa649 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Fri, 6 Dec 2024 14:25:19 -0500 Subject: [PATCH 11/20] Update releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml Co-authored-by: Emmett Butler <723615+emmettbutler@users.noreply.github.com> --- releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml b/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml index fc6966f1d3d..4401b00a4fd 100644 --- a/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml +++ b/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml @@ -3,6 +3,7 @@ fixes: - | Fixes an issue where calling ``unlock()`` on unlocked mutex could result in an undefined behavior. + - | Fixes an issue where the child process could deadlock after ``fork()`` as it didn't reset the locks. From 3599a664e5a046be492a2007b3aa5089cb51d6c2 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 17:55:53 +0000 Subject: [PATCH 12/20] revert changes --- .../profiling/dd_wrapper/include/profile.hpp | 15 ---------- .../profiling/dd_wrapper/src/profile.cpp | 30 +------------------ .../profiling/stack_v2/src/sampler.cpp | 7 ----- ...profiling-reset-lock-51a032479617f038.yaml | 9 ------ 4 files changed, 1 insertion(+), 60 deletions(-) delete mode 100644 releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp b/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp index 12adf4f35b1..59204384b6a 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp @@ -4,12 +4,10 @@ #include "types.hpp" #include -#include #include #include #include #include -#include #include extern "C" @@ -19,10 +17,6 @@ extern "C" namespace Datadog { -// Unordered containers don't get heterogeneous lookup until gcc-10, so for now use this -// strategy to dedup + store strings. -using StringTable = std::unordered_set; - // Serves to collect individual samples, as well as lengthen the scope of string data class Profile { @@ -33,12 +27,6 @@ class Profile std::atomic first_time{ true }; std::mutex profile_mtx{}; - // Storage for strings - std::deque string_storage{}; - StringTable strings{}; - std::mutex string_table_mtx{}; - void reset_string_table(); - // Configuration SampleType type_mask{ 0 }; unsigned int max_nframes{ g_default_max_nframes }; @@ -70,9 +58,6 @@ class Profile ddog_prof_Profile& profile_borrow(); void profile_release(); - // String table manipulation - std::string_view insert_or_get(std::string_view str); - // constref getters const ValueIndex& val(); diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp index 2416ff13b64..f9f7a3e9585 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp @@ -162,21 +162,6 @@ Datadog::Profile::one_time_init(SampleType type, unsigned int _max_nframes) first_time.store(false); } -std::string_view -Datadog::Profile::insert_or_get(std::string_view str) -{ - const std::lock_guard lock(string_table_mtx); // Serialize access - - auto str_it = strings.find(str); - if (str_it != strings.end()) { - return *str_it; - } - - string_storage.emplace_back(str); - strings.insert(string_storage.back()); - return string_storage.back(); -} - const Datadog::ValueIndex& Datadog::Profile::val() { @@ -201,19 +186,6 @@ Datadog::Profile::collect(const ddog_prof_Sample& sample, int64_t endtime_ns) void Datadog::Profile::postfork_child() { - // DEV: Need to reset locks in child to avoid deadlock, and we recreate - // the data structures these lock protect. - profile_mtx.~mutex(); - new (&profile_mtx) std::mutex(); + profile_mtx.unlock(); cycle_buffers(); - string_table_mtx.~mutex(); - new (&string_table_mtx) std::mutex(); - reset_string_table(); -} - -void -Datadog::Profile::reset_string_table() -{ - std::lock_guard lock(string_table_mtx); - string_storage.clear(); } diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index 01587503824..c05ae45477e 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -66,13 +66,6 @@ _stack_v2_atfork_child() // The only thing we need to do at fork is to propagate the PID to echion // so we don't even reveal this function to the user _set_pid(getpid()); - // We also need to recreate the lock used by echion - thread_info_map_lock.~mutex(); - new (&thread_info_map_lock) std::mutex(); - { - std::lock_guard lock(thread_info_map_lock); - thread_info_map.clear(); - } ThreadSpanLinks::postfork_child(); } diff --git a/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml b/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml deleted file mode 100644 index 4401b00a4fd..00000000000 --- a/releasenotes/notes/profiling-reset-lock-51a032479617f038.yaml +++ /dev/null @@ -1,9 +0,0 @@ ---- -fixes: - - | - Fixes an issue where calling ``unlock()`` on unlocked mutex could result in - an undefined behavior. - - | - Fixes an issue where the child process could deadlock after ``fork()`` as - it didn't reset the locks. - From 87973ad5f83883e8de763244b51461f849797bd1 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 18:44:02 +0000 Subject: [PATCH 13/20] take address of vector elem --- .../datadog/profiling/dd_wrapper/test/test_forking.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp index a4ff1d0d638..1aaadd62b61 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/test_forking.cpp @@ -56,12 +56,10 @@ emulate_sampler_wrapper(void* argp) } void -launch_pthread_samplers(const std::vector& args, std::vector& threads) +launch_pthread_samplers(std::vector& args, std::vector& pthread_handles) { for (unsigned int i = 0; i < args.size(); i++) { - auto arg = args[i]; - auto pthread_handle = threads[i]; - pthread_create(&pthread_handle, nullptr, emulate_sampler_wrapper, reinterpret_cast(&arg)); + pthread_create(&pthread_handles[i], nullptr, emulate_sampler_wrapper, reinterpret_cast(&args[i])); } } From 586ef820d0b617fdd8b55fdffd76e595f150f763 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 19:14:58 +0000 Subject: [PATCH 14/20] merge changes from fix pr --- .github/workflows/profiling-native.yml | 31 +++++++++++++------ .../profiling/dd_wrapper/test/CMakeLists.txt | 9 ++++-- .../profiling/dd_wrapper/test/TSan.supp | 2 ++ 3 files changed, 30 insertions(+), 12 deletions(-) create mode 100644 ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp diff --git a/.github/workflows/profiling-native.yml b/.github/workflows/profiling-native.yml index 1226c45fb16..1ed0379a314 100644 --- a/.github/workflows/profiling-native.yml +++ b/.github/workflows/profiling-native.yml @@ -1,21 +1,24 @@ -name: ProfilingNativeTests +name: Profiling Native Tests with Sanitizers on: push: paths: - ddtrace/internal/datadog/profiling/** + - branches: - main - - 'mq-working-branch**' + - "mq-working-branch**" pull_request: workflow_dispatch: {} jobs: - profiling-native-tests: - runs-on: ubuntu-latest + test: + runs-on: ${{ matrix.os }} + timeout-minutes: 5 strategy: fail-fast: false matrix: + os: [ubuntu-24.04] python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] sanitizer: ["safety", "thread"] @@ -24,13 +27,21 @@ jobs: with: fetch-depth: 1 - - uses: actions-rust-lang/setup-rust-toolchain@v1 - - name: Install latest Rust - run: rustup update stable && rustup default stable - - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} - - name: Run profiling native tests - run: ./ddtrace/internal/datadog/profiling/build_standalone.sh --${{matrix.sanitizer}} RelWithDebInfo dd_wrapper_test + - name: Install llvm 19 + run: | + # Ubuntu-24.04 GH actions image has llvm-18, but we use 19 as it's + # the latest one available. + wget https://apt.llvm.org/llvm.sh + chmod +x llvm.sh + sudo ./llvm.sh 19 + + - name: Run tests with sanitizers + run: | + # DEV: We currently have tests in dd_wrapper and stack_v2, setting + # stack_v2 here will also run tests in dd_wrapper. Revisit this when + # that changes. + ./ddtrace/internal/datadog/profiling/build_standalone.sh --${{matrix.sanitizer}} RelWithDebInfo stack_v2_test diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt b/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt index e7fe2ceb3ed..0be6098cd2a 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/CMakeLists.txt @@ -1,7 +1,7 @@ FetchContent_Declare( googletest GIT_REPOSITORY https://github.com/google/googletest.git - GIT_TAG release-1.11.0) + GIT_TAG v1.15.2) set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) @@ -21,7 +21,12 @@ function(dd_wrapper_add_test name) target_link_libraries(${name} PRIVATE gmock gtest_main dd_wrapper nlohmann_json::nlohmann_json) add_ddup_config(${name}) - gtest_discover_tests(${name}) + gtest_discover_tests(${name} + PROPERTIES + # We start new threads after fork(), and we want to continue + # running the tests after that instead of dying. + ENVIRONMENT "TSAN_OPTIONS=die_after_fork=0:suppressions=${CMAKE_CURRENT_SOURCE_DIR}/TSan.supp" + ) set_target_properties(${name} PROPERTIES INSTALL_RPATH "$ORIGIN/..") diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp new file mode 100644 index 00000000000..85efae630d6 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp @@ -0,0 +1,2 @@ +# Probably a false positive from lock-free data structure +race:moodycamel::ConcurrentQueue From 37c8f979f2e53a963ad6c1e2ccf33dcb9304d645 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 19:16:21 +0000 Subject: [PATCH 15/20] use taegyunkim/algorithm-include --- ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 9fc78ee8046..50c8d056c79 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -37,7 +37,7 @@ endif() # Add echion set(ECHION_COMMIT - "9d5bcc5867d7aefff73c837adcba4ef46eecebc6" + "ed744987f224fae3f93c842b2b5ffb083984ff8b" CACHE STRING "Commit hash of echion to use") FetchContent_Declare( echion From e0cd6478885cdd94f334dc307e557f8ba1c44daf Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 19:25:15 +0000 Subject: [PATCH 16/20] update tsan suppressions --- .../internal/datadog/profiling/dd_wrapper/test/TSan.supp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp index 85efae630d6..97e90a35285 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp @@ -1,2 +1,5 @@ -# Probably a false positive from lock-free data structure -race:moodycamel::ConcurrentQueue +# libdd is not compiled with sanitizers so probably that's why we get these +# data races from ddog_ArrayQueue and Datadog::Sample operations +race:ddog_ArrayQueue +race:Datadog::Sample::push_ +race:Datadog::Sample::flush_ From 71d84b81c1534d465405e20cabd8ef7741921def Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 19:28:25 +0000 Subject: [PATCH 17/20] also run when profiling python files are changed --- .github/workflows/profiling-native.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/profiling-native.yml b/.github/workflows/profiling-native.yml index 1ed0379a314..d302c3fe3df 100644 --- a/.github/workflows/profiling-native.yml +++ b/.github/workflows/profiling-native.yml @@ -4,7 +4,7 @@ on: push: paths: - ddtrace/internal/datadog/profiling/** - - + - ddtrace/profiling/** branches: - main - "mq-working-branch**" From 9ec78a80abfa81900cb6c3c76f7441c1fece6c1f Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 19:28:36 +0000 Subject: [PATCH 18/20] do not persist git credentials --- .github/workflows/profiling-native.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/profiling-native.yml b/.github/workflows/profiling-native.yml index d302c3fe3df..2e1fd50bf47 100644 --- a/.github/workflows/profiling-native.yml +++ b/.github/workflows/profiling-native.yml @@ -25,6 +25,7 @@ jobs: steps: - uses: actions/checkout@v4 with: + persist-credentials: false fetch-depth: 1 - uses: actions/setup-python@v5 From ac4d5144fdba180e352866846e82838f3f706ecf Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 19:37:40 +0000 Subject: [PATCH 19/20] update tsan suppressions --- ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp b/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp index 97e90a35285..bc9e9d23668 100644 --- a/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp +++ b/ddtrace/internal/datadog/profiling/dd_wrapper/test/TSan.supp @@ -1,5 +1,4 @@ # libdd is not compiled with sanitizers so probably that's why we get these # data races from ddog_ArrayQueue and Datadog::Sample operations race:ddog_ArrayQueue -race:Datadog::Sample::push_ -race:Datadog::Sample::flush_ +race:Datadog::Sample:: From 4217e3d3a4a2bc5468d3507eff1676cbccc96aea Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 9 Dec 2024 20:08:44 +0000 Subject: [PATCH 20/20] update paths --- .github/workflows/profiling-native.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/profiling-native.yml b/.github/workflows/profiling-native.yml index 2e1fd50bf47..98722552dbd 100644 --- a/.github/workflows/profiling-native.yml +++ b/.github/workflows/profiling-native.yml @@ -2,13 +2,13 @@ name: Profiling Native Tests with Sanitizers on: push: - paths: - - ddtrace/internal/datadog/profiling/** - - ddtrace/profiling/** branches: - main - "mq-working-branch**" pull_request: + paths: + - ddtrace/internal/datadog/profiling/** + - ddtrace/profiling/** workflow_dispatch: {} jobs: