From 917b9c7b0fe5de92f3a75f192f17956997bafa78 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Wed, 29 Jul 2026 18:17:37 +0200 Subject: [PATCH 1/7] fix(data-pipeline): contain combined FFI panics Enable `catch_panic` when the data pipeline FFI is bundled through the profiling FFI. Without feature propagation, a Rust panic can cross the combined C library boundary and terminate the host process. Add a packaged C example that triggers a capacity overflow and verifies that the API returns `DDOG_TRACE_EXPORTER_ERROR_CODE_PANIC` instead. APMSP-3830 --- examples/ffi/CMakeLists.txt | 4 +++ .../ffi/trace_exporter_panic_containment.c | 29 +++++++++++++++++++ libdd-profiling-ffi/Cargo.toml | 2 +- 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 examples/ffi/trace_exporter_panic_containment.c diff --git a/examples/ffi/CMakeLists.txt b/examples/ffi/CMakeLists.txt index 60a061278f..a118920c3a 100644 --- a/examples/ffi/CMakeLists.txt +++ b/examples/ffi/CMakeLists.txt @@ -86,6 +86,10 @@ add_executable(trace_exporter trace_exporter.c) target_link_libraries(trace_exporter PRIVATE Datadog::Profiling) set_vcruntime_link_type(trace_exporter ${VCRUNTIME_LINK_TYPE}) +add_executable(trace_exporter_panic_containment trace_exporter_panic_containment.c) +target_link_libraries(trace_exporter_panic_containment PRIVATE Datadog::Profiling) +set_vcruntime_link_type(trace_exporter_panic_containment ${VCRUNTIME_LINK_TYPE}) + add_executable(array_queue array_queue.cpp) target_compile_features(array_queue PRIVATE cxx_std_20) target_link_libraries(array_queue PRIVATE Datadog::Profiling) diff --git a/examples/ffi/trace_exporter_panic_containment.c b/examples/ffi/trace_exporter_panic_containment.c new file mode 100644 index 0000000000..b449402adf --- /dev/null +++ b/examples/ffi/trace_exporter_panic_containment.c @@ -0,0 +1,29 @@ +// Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include + +int main(void) +{ + ddog_TracerTraceChunks *chunks = NULL; + ddog_TraceExporterError *err = ddog_tracer_trace_chunks_new(SIZE_MAX, &chunks); + + if (err == NULL) { + fprintf(stderr, "capacity overflow unexpectedly succeeded\n"); + if (chunks != NULL) { + ddog_tracer_trace_chunks_free(chunks); + } + return 1; + } + + int status = 0; + if (err->code != DDOG_TRACE_EXPORTER_ERROR_CODE_PANIC) { + fprintf(stderr, "capacity overflow returned error code %d instead of panic\n", err->code); + status = 1; + } + + ddog_trace_exporter_error_free(err); + return status; +} diff --git a/libdd-profiling-ffi/Cargo.toml b/libdd-profiling-ffi/Cargo.toml index 44ca2e5c93..c5b3fb1020 100644 --- a/libdd-profiling-ffi/Cargo.toml +++ b/libdd-profiling-ffi/Cargo.toml @@ -22,7 +22,7 @@ cbindgen = ["build_common/cbindgen", "libdd-common-ffi/cbindgen", "libdd-shared- ddtelemetry-ffi = ["dep:libdd-telemetry-ffi"] datadog-log-ffi = ["dep:libdd-log-ffi"] symbolizer = ["symbolizer-ffi"] -data-pipeline-ffi = ["dep:libdd-data-pipeline-ffi"] +data-pipeline-ffi = ["dep:libdd-data-pipeline-ffi", "libdd-data-pipeline-ffi/catch_panic"] # Enable zstd compression for the agentless trace intake sender. data-pipeline-compression = ["data-pipeline-ffi", "libdd-data-pipeline-ffi/compression"] crashtracker-ffi = ["dep:libdd-crashtracker-ffi"] From 1137f431c1390836671cb4abad4f0242db825f40 Mon Sep 17 00:00:00 2001 From: Edmund Kump Date: Fri, 31 Jul 2026 18:54:13 -0400 Subject: [PATCH 2/7] also include catch panic feature libdd-library-config-ffi --- libdd-profiling-ffi/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libdd-profiling-ffi/Cargo.toml b/libdd-profiling-ffi/Cargo.toml index c5b3fb1020..e382f8bfd9 100644 --- a/libdd-profiling-ffi/Cargo.toml +++ b/libdd-profiling-ffi/Cargo.toml @@ -31,7 +31,7 @@ crashtracker-collector = ["crashtracker-ffi", "libdd-crashtracker-ffi/collector" # Enables the use of this library to receiver crash-info from a suitable collector crashtracker-receiver = ["crashtracker-ffi", "libdd-crashtracker-ffi/receiver"] demangler = ["crashtracker-ffi", "libdd-crashtracker-ffi/demangler"] -datadog-library-config-ffi = ["dep:libdd-library-config-ffi"] +datadog-library-config-ffi = ["dep:libdd-library-config-ffi", "libdd-library-config-ffi/catch_panic"] ddcommon-ffi = ["dep:libdd-common-ffi"] ddsketch-ffi = ["dep:libdd-ddsketch-ffi"] datadog-ffe-ffi = ["dep:datadog-ffe-ffi"] From 1d69a8396575869716763024d69d9064f5b8b5bb Mon Sep 17 00:00:00 2001 From: Edmund Kump Date: Sun, 2 Aug 2026 12:36:51 -0400 Subject: [PATCH 3/7] add lint check to CI to make sure catch panic feature enabled --- .github/CODEOWNERS | 1 + .github/workflows/lint.yml | 25 +++++++ scripts/check_ffi_panic_containment.sh | 94 ++++++++++++++++++++++++++ 3 files changed, 120 insertions(+) create mode 100755 scripts/check_ffi_panic_containment.sh diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index eb579c55b5..5a392f685c 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -79,6 +79,7 @@ rust-toolchain.toml @DataDog/libdatadog rustfmt.toml @DataDog/libdatadog-core scripts/check_cargo_metadata.sh @DataDog/libdatadog-core scripts/check_crypto_providers.sh @DataDog/libdatadog-core +scripts/check_ffi_panic_containment.sh @DataDog/libdatadog-core scripts/commits-since-release.sh @DataDog/libdatadog-core scripts/create-release.sh @DataDog/apm-common-components-core scripts/crates-to-package.sh @DataDog/libdatadog-core diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 3ba3d0405f..a514d5f196 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -70,6 +70,31 @@ jobs: uses: devops-actions/actionlint@c6744a34774e4e1c1df0ff66bdb07ec7ee480ca0 # 0.1.9 with: shellcheck_opts: '-e SC2086' + ffi-panic-containment: + needs: setup + runs-on: ubuntu-latest + name: "Aggregated FFI crates keep catch_panic enabled" + env: + CARGO_TERM_COLOR: always + CARGO_INCREMENTAL: 0 + steps: + - name: Checkout sources + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # 4.2.2 + - name: Install ${{ needs.setup.outputs.rust-version }} toolchain + run: | + rustup set profile minimal + rustup install ${{ needs.setup.outputs.rust-version }} + rustup default ${{ needs.setup.outputs.rust-version }} + - name: Cache [rust] + uses: Swatinem/rust-cache@f13886b937689c021905a6b90929199931d60db1 # 2.8.1 + with: + cache-targets: true # cache build artifacts + cache-bin: true # cache the ~/.cargo/bin directory + # Deliberately not gated on crates-count: the guarded invariant is a + # property of the feature graph, and the manifest edge that carries it + # can regress without any crate being reported as changed. + - name: Check FFI panic containment + run: ./scripts/check_ffi_panic_containment.sh rustfmt: needs: setup if: needs.setup.outputs.crates-count != '0' diff --git a/scripts/check_ffi_panic_containment.sh b/scripts/check_ffi_panic_containment.sh new file mode 100755 index 0000000000..446624a355 --- /dev/null +++ b/scripts/check_ffi_panic_containment.sh @@ -0,0 +1,94 @@ +#!/bin/bash +# Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ +# SPDX-License-Identifier: Apache-2.0 +# +# Asserts that every FFI sub-crate aggregated into libdd-profiling-ffi keeps +# its `catch_panic` feature enabled in the combined artifact. +# +# libdd-profiling-ffi pulls its sub-crates with `default-features = false`, and +# `catch_panic` is a *default* feature of the crates that have one. Dropping the +# defaults silently degrades `catch_panic!` to a bare call, so a Rust panic +# crosses the non-unwinding `extern "C"` boundary and aborts the host process. +# TODO: APMSP-3874 - evaluate if catch_panic should even be an optional feature. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT_DIR="$(dirname "$SCRIPT_DIR")" + +cd "$ROOT_DIR" + +AGGREGATOR="libdd-profiling-ffi" + +METADATA=$(cargo metadata --no-deps --format-version 1) + +# has_catch_panic +# 0 if the workspace crate declares a `catch_panic` feature, 1 otherwise. +has_catch_panic() { + printf '%s' "$METADATA" | jq -e --arg name "$1" \ + '.packages[] | select(.name == $name) | .features | has("catch_panic")' > /dev/null 2>&1 +} + +# activating_feature +# The aggregator feature listing `dep:`, empty if there is none. +activating_feature() { + printf '%s' "$METADATA" | jq -r --arg agg "$AGGREGATOR" --arg dep "$1" \ + 'first(.packages[] | select(.name == $agg) | .features | to_entries[] + | select(any(.value[]; . == "dep:" + $dep)) | .key)' +} + +mapfile -t OPTIONAL_DEPS < <(printf '%s' "$METADATA" | jq -r --arg agg "$AGGREGATOR" \ + '.packages[] | select(.name == $agg) | .dependencies[] | select(.optional) | .name' | sort -u) + +if [ "${#OPTIONAL_DEPS[@]}" -eq 0 ]; then + echo "no optional dependencies found for $AGGREGATOR" >&2 + exit 2 +fi + +errors=0 +checked=0 +skipped=0 + +for dep in "${OPTIONAL_DEPS[@]}"; do + if ! has_catch_panic "$dep"; then + skipped=$((skipped + 1)) + continue + fi + + feature=$(activating_feature "$dep") + if [ -z "$feature" ]; then + echo "ERROR: no $AGGREGATOR feature activates optional dependency $dep" >&2 + exit 2 + fi + + checked=$((checked + 1)) + + output=$(cargo tree -p "$AGGREGATOR" --features "$feature" --edges features -i "$dep" 2>&1) || { + echo "ERROR: cargo tree failed for $AGGREGATOR --features $feature -i $dep:" >&2 + echo "$output" | sed 's/^/ /' >&2 + exit 2 + } + + if grep -qF "$dep feature \"catch_panic\"" <<<"$output"; then + echo "ok: $AGGREGATOR --features $feature keeps $dep/catch_panic" + else + echo "FAIL: $AGGREGATOR --features $feature does not enable $dep/catch_panic" + echo " A panic in $dep would abort the host process instead of returning an error." + echo " Fix: add \"$dep/catch_panic\" to the \"$feature\" feature in $AGGREGATOR/Cargo.toml" + echo "$output" | sed 's/^/ /' + errors=$((errors + 1)) + fi +done + +if [ "$checked" -eq 0 ]; then + echo "no aggregated crate declares a catch_panic feature -- discovery is broken" >&2 + exit 2 +fi + +if [ "$errors" -gt 0 ]; then + echo + echo "FFI panic containment check failed: $errors violation(s) across $checked edge(s)" + exit 1 +fi + +echo "FFI panic containment check passed for $checked edge(s) ($skipped crate(s) without the feature skipped)" From 0a2775af94b3f379fc326d325981ba9522d97c50 Mon Sep 17 00:00:00 2001 From: Edmund Kump Date: Sun, 2 Aug 2026 13:21:45 -0400 Subject: [PATCH 4/7] update example to include non-panic call works correctly too --- .../ffi/trace_exporter_panic_containment.c | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/examples/ffi/trace_exporter_panic_containment.c b/examples/ffi/trace_exporter_panic_containment.c index b449402adf..5fa9401609 100644 --- a/examples/ffi/trace_exporter_panic_containment.c +++ b/examples/ffi/trace_exporter_panic_containment.c @@ -1,6 +1,9 @@ // Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 +// Proves a Rust panic is contained inside the combined profiling artifact and +// surfaced to C as an error rather than aborting the process. + #include #include #include @@ -8,7 +11,24 @@ int main(void) { ddog_TracerTraceChunks *chunks = NULL; - ddog_TraceExporterError *err = ddog_tracer_trace_chunks_new(SIZE_MAX, &chunks); + + // Positive control: without it, a build that failed every call would still + // satisfy the panic assertion below. + ddog_TraceExporterError *err = ddog_tracer_trace_chunks_new(0, &chunks); + if (err != NULL) { + fprintf(stderr, "trace_chunks_new(0) failed with error code %d\n", err->code); + ddog_trace_exporter_error_free(err); + return 1; + } + if (chunks == NULL) { + fprintf(stderr, "trace_chunks_new(0) returned success with a null handle\n"); + return 1; + } + ddog_tracer_trace_chunks_free(chunks); + // The panic path never writes out_handle, so reset before reusing it. + chunks = NULL; + + err = ddog_tracer_trace_chunks_new(SIZE_MAX, &chunks); if (err == NULL) { fprintf(stderr, "capacity overflow unexpectedly succeeded\n"); From 7b56f2c751c730863bc6a92bb67f54965149db38 Mon Sep 17 00:00:00 2001 From: Julio Date: Mon, 3 Aug 2026 15:33:19 +0200 Subject: [PATCH 5/7] chore: Address PR concerns --- .github/CODEOWNERS | 1 - .github/workflows/lint.yml | 25 -- AGENTS.md | 2 +- builder/Cargo.toml | 3 +- builder/src/bin/release.rs | 30 +- builder/src/features.rs | 281 ++++++++++++++++++ builder/src/lib.rs | 1 + builder/src/profiling.rs | 8 +- examples/ffi/CMakeLists.txt | 4 - examples/ffi/trace_exporter.c | 35 ++- .../ffi/trace_exporter_panic_containment.c | 49 --- libdd-profiling-ffi/Cargo.toml | 30 +- scripts/check_ffi_panic_containment.sh | 94 ------ 13 files changed, 349 insertions(+), 214 deletions(-) create mode 100644 builder/src/features.rs delete mode 100644 examples/ffi/trace_exporter_panic_containment.c delete mode 100755 scripts/check_ffi_panic_containment.sh diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 5a392f685c..eb579c55b5 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -79,7 +79,6 @@ rust-toolchain.toml @DataDog/libdatadog rustfmt.toml @DataDog/libdatadog-core scripts/check_cargo_metadata.sh @DataDog/libdatadog-core scripts/check_crypto_providers.sh @DataDog/libdatadog-core -scripts/check_ffi_panic_containment.sh @DataDog/libdatadog-core scripts/commits-since-release.sh @DataDog/libdatadog-core scripts/create-release.sh @DataDog/apm-common-components-core scripts/crates-to-package.sh @DataDog/libdatadog-core diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index a514d5f196..3ba3d0405f 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -70,31 +70,6 @@ jobs: uses: devops-actions/actionlint@c6744a34774e4e1c1df0ff66bdb07ec7ee480ca0 # 0.1.9 with: shellcheck_opts: '-e SC2086' - ffi-panic-containment: - needs: setup - runs-on: ubuntu-latest - name: "Aggregated FFI crates keep catch_panic enabled" - env: - CARGO_TERM_COLOR: always - CARGO_INCREMENTAL: 0 - steps: - - name: Checkout sources - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # 4.2.2 - - name: Install ${{ needs.setup.outputs.rust-version }} toolchain - run: | - rustup set profile minimal - rustup install ${{ needs.setup.outputs.rust-version }} - rustup default ${{ needs.setup.outputs.rust-version }} - - name: Cache [rust] - uses: Swatinem/rust-cache@f13886b937689c021905a6b90929199931d60db1 # 2.8.1 - with: - cache-targets: true # cache build artifacts - cache-bin: true # cache the ~/.cargo/bin directory - # Deliberately not gated on crates-count: the guarded invariant is a - # property of the feature graph, and the manifest edge that carries it - # can regress without any crate being reported as changed. - - name: Check FFI panic containment - run: ./scripts/check_ffi_panic_containment.sh rustfmt: needs: setup if: needs.setup.outputs.crates-count != '0' diff --git a/AGENTS.md b/AGENTS.md index 3a567aa6e4..4a52ed34e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,7 @@ libdatadog is integrated into many runtimes and languages via FFI, and runs in D - Bubble errors up to the library caller with detail — prefer structured error enums (e.g. `thiserror`) over opaque strings. - Stay free of global effects unless a feature requires them: no spawning threads, no globals, no reading environment variables behind the caller's back. - Care about performance, especially memory allocations on hot paths. -- Panics across FFI boundaries are undefined behavior. FFI entry points must catch unwinds (e.g. `std::panic::catch_unwind`) and convert them into error returns rather than letting them propagate into the caller's runtime. +- A panic that reaches an `extern "C"` boundary aborts the host process. FFI entry points must catch unwinds (e.g. `std::panic::catch_unwind`) and convert them into error returns rather than letting them propagate into the caller's runtime. Whether a release artifact gets panic containment is decided by `builder` alone, through its `catch_panic` feature (a default), which propagates to the `catch_panic` feature in `libdd-profiling-ffi/Cargo.toml`. Projects building their own flavor with `builder`'s default features off ask for `catch_panic` explicitly; leaving it out yields abort-on-panic semantics. The FFI examples are what verify containment is on for our own release process. - The C FFI does **not** offer C ABI backward-compatibility guarantees: callers (Datadog SDKs) pin to specific libdatadog versions, so `#[repr(C)]` layouts, function signatures, and enum variants may change between releases. ### Cryptography diff --git a/builder/Cargo.toml b/builder/Cargo.toml index 57001b5550..20bd8e8331 100644 --- a/builder/Cargo.toml +++ b/builder/Cargo.toml @@ -20,6 +20,7 @@ default = [ "ddsketch", "ffe", "shared-runtime", + "catch_panic", ] crashtracker = [] profiling = [] @@ -35,11 +36,11 @@ ddsketch = [] ffe = [] shared-runtime = [] otel-thread-ctx = [] +catch_panic = [] regex-lite = ["libdd-common/regex-lite"] [lib] bench = false -test = false doctest = false [dependencies] diff --git a/builder/src/bin/release.rs b/builder/src/bin/release.rs index 4a3805a3f1..c25f11604a 100644 --- a/builder/src/bin/release.rs +++ b/builder/src/bin/release.rs @@ -9,6 +9,7 @@ use builder::builder::Builder; use builder::common::Common; #[cfg(feature = "crashtracker")] use builder::crashtracker::CrashTracker; +use builder::features::{profiling_features, Selection}; #[cfg(feature = "profiling")] use builder::profiling::Profiling; use builder::utils::project_root; @@ -54,34 +55,7 @@ pub fn main() { host.clone() }; - #[allow(clippy::vec_init_then_push)] - let features = { - #[allow(unused_mut)] - let mut f: Vec = vec![]; - #[cfg(feature = "telemetry")] - f.push("ddtelemetry-ffi".to_string()); - #[cfg(feature = "data-pipeline")] - f.push("data-pipeline-ffi".to_string()); - #[cfg(feature = "data-pipeline-compression")] - f.push("data-pipeline-compression".to_string()); - #[cfg(feature = "crashtracker")] - f.push("crashtracker-ffi".to_string()); - #[cfg(feature = "symbolizer")] - f.push("symbolizer".to_string()); - #[cfg(feature = "library-config")] - f.push("datadog-library-config-ffi".to_string()); - #[cfg(feature = "log")] - f.push("datadog-log-ffi".to_string()); - #[cfg(feature = "ddsketch")] - f.push("ddsketch-ffi".to_string()); - #[cfg(feature = "ffe")] - f.push("datadog-ffe-ffi".to_string()); - #[cfg(feature = "shared-runtime")] - f.push("shared-runtime".to_string()); - #[cfg(feature = "otel-thread-ctx")] - f.push("otel-thread-ctx-ffi".to_string()); - f - }; + let features = profiling_features(&Selection::from_cargo_features()); let mut builder = Builder::new( source_path.to_str().unwrap(), diff --git a/builder/src/features.rs b/builder/src/features.rs new file mode 100644 index 0000000000..b34738acd7 --- /dev/null +++ b/builder/src/features.rs @@ -0,0 +1,281 @@ +// Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +//! Mapping from `builder`'s own Cargo features onto the `libdd-profiling-ffi` feature +//! list used to produce the combined artifact. +//! +//! Expressed as data plus a pure function rather than a wall of `#[cfg]` attributes so +//! that every combination — including the ones no CI job ever builds — can be asserted +//! in unit tests (RFC 0016, option E). `builder/src/bin/release.rs` was one of the +//! `#[cfg]` hot spots that RFC calls out. + +/// What a release build should contain. +/// +/// Downstream projects select these through `builder`'s Cargo features; see +/// `builder/Cargo.toml`. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct Selection { + pub telemetry: bool, + pub data_pipeline: bool, + pub data_pipeline_compression: bool, + pub crashtracker: bool, + pub symbolizer: bool, + pub library_config: bool, + pub log: bool, + pub ddsketch: bool, + pub ffe: bool, + pub shared_runtime: bool, + pub otel_thread_ctx: bool, + /// Contain panics at the FFI boundary, returning them to the caller as errors instead + /// of aborting the process. A `builder` default, so our own release artifacts get it; + /// a project that turns off `builder`'s default features and wants it back asks for + /// `catch_panic` like any other feature. + pub catch_panic: bool, +} + +impl Selection { + /// Reads the selection from `builder`'s own compiled Cargo features. + pub fn from_cargo_features() -> Self { + Self { + telemetry: cfg!(feature = "telemetry"), + data_pipeline: cfg!(feature = "data-pipeline"), + data_pipeline_compression: cfg!(feature = "data-pipeline-compression"), + crashtracker: cfg!(feature = "crashtracker"), + symbolizer: cfg!(feature = "symbolizer"), + library_config: cfg!(feature = "library-config"), + log: cfg!(feature = "log"), + ddsketch: cfg!(feature = "ddsketch"), + ffe: cfg!(feature = "ffe"), + shared_runtime: cfg!(feature = "shared-runtime"), + otel_thread_ctx: cfg!(feature = "otel-thread-ctx"), + catch_panic: cfg!(feature = "catch_panic"), + } + } +} + +/// `libdd-profiling-ffi` features the combined artifact always needs, whatever else is +/// selected. `ddcommon-ffi` is one of them: it is a *default* feature of that crate, and +/// [`profiling_features`] is passed alongside `--no-default-features`. +const ALWAYS: &[&str] = &["cbindgen", "ddcommon-ffi"]; + +/// The complete `--features` list for `cargo rustc -p libdd-profiling-ffi`. +/// +/// Callers **must** also pass `--no-default-features`. The list is exhaustive by design: +/// the artifact's feature set is exactly what this function returns, with nothing +/// inherited implicitly. Relying on inherited defaults is what let `catch_panic` go +/// missing from the shipped artifact in APMSP-3830 without any build failing. +pub fn profiling_features(selection: &Selection) -> Vec { + let mut features: Vec<&str> = ALWAYS.to_vec(); + + // Kept first so a reader of any emitted command line sees the panic policy immediately. + if selection.catch_panic { + features.push("catch_panic"); + } + + if selection.telemetry { + features.push("ddtelemetry-ffi"); + } + if selection.data_pipeline { + features.push("data-pipeline-ffi"); + } + if selection.data_pipeline_compression { + features.push("data-pipeline-compression"); + } + if selection.crashtracker { + features.extend([ + "crashtracker-ffi", + "crashtracker-collector", + "crashtracker-receiver", + "demangler", + ]); + } + if selection.symbolizer { + features.push("symbolizer"); + } + if selection.library_config { + features.push("datadog-library-config-ffi"); + } + if selection.log { + features.push("datadog-log-ffi"); + } + if selection.ddsketch { + features.push("ddsketch-ffi"); + } + if selection.ffe { + features.push("datadog-ffe-ffi"); + } + if selection.shared_runtime { + features.push("shared-runtime"); + } + if selection.otel_thread_ctx { + features.push("otel-thread-ctx-ffi"); + } + + features.into_iter().map(str::to_string).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Every module-selecting flag, so the exhaustive test below cannot silently stop + /// covering a field that someone adds to [`Selection`]. + const MODULE_SETTERS: &[fn(&mut Selection)] = &[ + |s| s.telemetry = true, + |s| s.data_pipeline = true, + |s| s.data_pipeline_compression = true, + |s| s.crashtracker = true, + |s| s.symbolizer = true, + |s| s.library_config = true, + |s| s.log = true, + |s| s.ddsketch = true, + |s| s.ffe = true, + |s| s.shared_runtime = true, + |s| s.otel_thread_ctx = true, + ]; + + fn selection_from_bits(bits: u32) -> Selection { + let mut selection = Selection::default(); + for (index, set) in MODULE_SETTERS.iter().enumerate() { + if bits & (1 << index) != 0 { + set(&mut selection); + } + } + selection + } + + #[test] + fn selecting_catch_panic_emits_the_feature() { + let selection = Selection { + catch_panic: true, + ..Default::default() + }; + assert!(profiling_features(&selection).contains(&"catch_panic".to_string())); + } + + #[test] + fn not_selecting_catch_panic_omits_the_feature() { + assert!(!profiling_features(&Selection::default()).contains(&"catch_panic".to_string())); + } + + /// Containment must reach libdd-profiling-ffi for every module combination a project can + /// ask for, not just the one our own release happens to build. This is the APMSP-3830 + /// regression: the feature was there for some builds and quietly absent for others. + #[test] + fn catch_panic_survives_every_module_combination() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let mut selection = selection_from_bits(bits); + selection.catch_panic = true; + assert!( + profiling_features(&selection).contains(&"catch_panic".to_string()), + "catch_panic missing despite being selected, for {selection:?}" + ); + } + } + + /// The converse: not asking for it never sneaks it in, whatever else is selected. + #[test] + fn catch_panic_is_never_implied_by_another_feature() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let selection = selection_from_bits(bits); + assert!( + !profiling_features(&selection).contains(&"catch_panic".to_string()), + "catch_panic emitted without being selected, for {selection:?}" + ); + } + } + + /// Exhaustive struct literal on purpose: no `..Default::default()`. Adding a field to + /// [`Selection`] stops this compiling, which is the prompt to also give it a branch in + /// [`profiling_features`] and an entry in `MODULE_SETTERS`. Without this, a new field + /// could be silently ignored by the mapping and every other test here would still pass. + #[test] + fn all_fields_set_emits_every_module_feature() { + let everything = Selection { + telemetry: true, + data_pipeline: true, + data_pipeline_compression: true, + crashtracker: true, + symbolizer: true, + library_config: true, + log: true, + ddsketch: true, + ffe: true, + shared_runtime: true, + otel_thread_ctx: true, + catch_panic: true, + }; + + assert_eq!( + profiling_features(&everything), + vec![ + "cbindgen", + "ddcommon-ffi", + "catch_panic", + "ddtelemetry-ffi", + "data-pipeline-ffi", + "data-pipeline-compression", + "crashtracker-ffi", + "crashtracker-collector", + "crashtracker-receiver", + "demangler", + "symbolizer", + "datadog-library-config-ffi", + "datadog-log-ffi", + "ddsketch-ffi", + "datadog-ffe-ffi", + "shared-runtime", + "otel-thread-ctx-ffi", + ] + ); + } + + /// `--no-default-features` means nothing is inherited, so the defaults the artifact + /// still needs have to be listed explicitly. + #[test] + fn always_lists_features_that_no_longer_come_from_defaults() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let features = profiling_features(&selection_from_bits(bits)); + for required in ALWAYS { + assert!( + features.contains(&required.to_string()), + "{required} missing for bits {bits:#b}" + ); + } + } + } + + #[test] + fn crashtracker_pulls_in_its_sub_features() { + let selection = Selection { + crashtracker: true, + ..Default::default() + }; + let features = profiling_features(&selection); + for expected in [ + "crashtracker-ffi", + "crashtracker-collector", + "crashtracker-receiver", + "demangler", + ] { + assert!(features.contains(&expected.to_string()), "{expected}"); + } + } + + #[test] + fn no_module_selected_emits_no_module_features() { + let features = profiling_features(&Selection::default()); + assert_eq!(features, vec!["cbindgen", "ddcommon-ffi"]); + } + + #[test] + fn emitted_list_has_no_duplicates() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let features = profiling_features(&selection_from_bits(bits)); + let mut sorted = features.clone(); + sorted.sort(); + sorted.dedup(); + assert_eq!(sorted.len(), features.len(), "duplicates for {bits:#b}"); + } + } +} diff --git a/builder/src/lib.rs b/builder/src/lib.rs index 3151e7c408..e9a465e769 100644 --- a/builder/src/lib.rs +++ b/builder/src/lib.rs @@ -6,6 +6,7 @@ pub mod builder; pub mod common; #[cfg(feature = "crashtracker")] pub mod crashtracker; +pub mod features; pub mod module; pub mod utils; diff --git a/builder/src/profiling.rs b/builder/src/profiling.rs index 1e22132974..c79461a67a 100644 --- a/builder/src/profiling.rs +++ b/builder/src/profiling.rs @@ -8,7 +8,6 @@ use anyhow::Result; use serde::Deserialize; use std::ffi::OsStr; use std::fs; -use std::ops::Add; use std::path::{Path, PathBuf}; use std::process::Command; use std::rc::Rc; @@ -132,17 +131,14 @@ impl Profiling { impl Module for Profiling { fn build(&self) -> Result<()> { - let features = self.features.to_string() + "," + "cbindgen"; - #[cfg(feature = "crashtracker")] - let features = features.add(",crashtracker-collector,crashtracker-receiver,demangler"); - // Using rustc instead of build in order to overcome issues with LTO optimization. let mut cargo_args = vec![ "rustc", "-p", CRATE_FOLDER, + "--no-default-features", "--features", - &features, + &self.features, "--target", &self.arch, ]; diff --git a/examples/ffi/CMakeLists.txt b/examples/ffi/CMakeLists.txt index a118920c3a..60a061278f 100644 --- a/examples/ffi/CMakeLists.txt +++ b/examples/ffi/CMakeLists.txt @@ -86,10 +86,6 @@ add_executable(trace_exporter trace_exporter.c) target_link_libraries(trace_exporter PRIVATE Datadog::Profiling) set_vcruntime_link_type(trace_exporter ${VCRUNTIME_LINK_TYPE}) -add_executable(trace_exporter_panic_containment trace_exporter_panic_containment.c) -target_link_libraries(trace_exporter_panic_containment PRIVATE Datadog::Profiling) -set_vcruntime_link_type(trace_exporter_panic_containment ${VCRUNTIME_LINK_TYPE}) - add_executable(array_queue array_queue.cpp) target_compile_features(array_queue PRIVATE cxx_std_20) target_link_libraries(array_queue PRIVATE Datadog::Profiling) diff --git a/examples/ffi/trace_exporter.c b/examples/ffi/trace_exporter.c index 38277aabf6..3fe7a90902 100644 --- a/examples/ffi/trace_exporter.c +++ b/examples/ffi/trace_exporter.c @@ -58,6 +58,29 @@ int log_init(const char* log_path) { return 0; } +int test_error_on_panic(void) { + ddog_TracerTraceChunks *chunks = NULL; + ddog_TraceExporterError *panic_err = ddog_tracer_trace_chunks_new(SIZE_MAX, &chunks); + + if (panic_err == NULL) { + fprintf(stderr, "capacity overflow unexpectedly succeeded\n"); + if (chunks != NULL) { ddog_tracer_trace_chunks_free(chunks); } + return 1; + } + if (panic_err->code != DDOG_TRACE_EXPORTER_ERROR_CODE_PANIC) { + fprintf(stderr, "capacity overflow returned error code %d instead of panic\n", + panic_err->code); + ddog_trace_exporter_error_free(panic_err); + return 1; + } + + // Containment worked: the panic came back as an error instead of aborting. Note the + // error *code* is deliberately not returned here — it is non-zero, so returning it + // would report this success as a failure. + ddog_trace_exporter_error_free(panic_err); + return 0; +} + int main(int argc, char** argv) { // Initialize logger with optional path from command line @@ -67,8 +90,18 @@ int main(int argc, char** argv) return 1; } - int error; + // Guard to check that the library is built with catch_panic by default. + // + // Returns directly rather than `goto error`: nothing has been allocated yet, and the + // cleanup at `error:` frees `trace_exporter` and `config`, which are declared below. + // Jumping from here would skip their initializers and free indeterminate pointers + // (gcc -Wjump-misses-init). + if (test_error_on_panic() != 0) { + return 1; + } + + int error; ddog_TraceExporter* trace_exporter = NULL; ddog_CharSlice url = DDOG_CHARSLICE_C("http://localhost:8126/"); ddog_CharSlice tracer_version = DDOG_CHARSLICE_C("v0.1"); diff --git a/examples/ffi/trace_exporter_panic_containment.c b/examples/ffi/trace_exporter_panic_containment.c deleted file mode 100644 index 5fa9401609..0000000000 --- a/examples/ffi/trace_exporter_panic_containment.c +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ -// SPDX-License-Identifier: Apache-2.0 - -// Proves a Rust panic is contained inside the combined profiling artifact and -// surfaced to C as an error rather than aborting the process. - -#include -#include -#include - -int main(void) -{ - ddog_TracerTraceChunks *chunks = NULL; - - // Positive control: without it, a build that failed every call would still - // satisfy the panic assertion below. - ddog_TraceExporterError *err = ddog_tracer_trace_chunks_new(0, &chunks); - if (err != NULL) { - fprintf(stderr, "trace_chunks_new(0) failed with error code %d\n", err->code); - ddog_trace_exporter_error_free(err); - return 1; - } - if (chunks == NULL) { - fprintf(stderr, "trace_chunks_new(0) returned success with a null handle\n"); - return 1; - } - ddog_tracer_trace_chunks_free(chunks); - // The panic path never writes out_handle, so reset before reusing it. - chunks = NULL; - - err = ddog_tracer_trace_chunks_new(SIZE_MAX, &chunks); - - if (err == NULL) { - fprintf(stderr, "capacity overflow unexpectedly succeeded\n"); - if (chunks != NULL) { - ddog_tracer_trace_chunks_free(chunks); - } - return 1; - } - - int status = 0; - if (err->code != DDOG_TRACE_EXPORTER_ERROR_CODE_PANIC) { - fprintf(stderr, "capacity overflow returned error code %d instead of panic\n", err->code); - status = 1; - } - - ddog_trace_exporter_error_free(err); - return status; -} diff --git a/libdd-profiling-ffi/Cargo.toml b/libdd-profiling-ffi/Cargo.toml index e382f8bfd9..7f7c3e07e2 100644 --- a/libdd-profiling-ffi/Cargo.toml +++ b/libdd-profiling-ffi/Cargo.toml @@ -17,12 +17,34 @@ bench = false name = "datadog_profiling_ffi" [features] -default = ["ddcommon-ffi"] +default = ["ddcommon-ffi", "catch_panic"] +# Panic containment for every aggregated FFI sub-crate that supports it. +# +# The sub-crates enable `catch_panic` in their own `default`, but this crate pulls +# them with `default-features = false`, so containment has to be re-enabled here. +# Without it `catch_panic!` degrades to a bare call and a Rust panic unwinds into a +# non-unwinding `extern "C"` frame, aborting the host process instead of returning +# an error (APMSP-3830). +# +# Weak (`?/`) on purpose: enabling this must never activate an optional dependency +# that was not otherwise requested. +# +# Every new aggregated FFI sub-crate declaring a `catch_panic` feature belongs here. +# Whether it is actually enabled is builder's decision: builder passes an exhaustive +# --features list with --no-default-features, and mirrors this as its own `catch_panic` +# default feature. Building without it is supported and yields abort-on-panic semantics. +# examples/ffi/trace_exporter.c verifies containment is on in the artifact our own +# release process produces. +catch_panic = [ + "libdd-data-pipeline-ffi?/catch_panic", + "libdd-library-config-ffi?/catch_panic", + "libdd-shared-runtime-ffi?/catch_panic", +] cbindgen = ["build_common/cbindgen", "libdd-common-ffi/cbindgen", "libdd-shared-runtime-ffi?/cbindgen", "libdd-otel-thread-ctx-ffi?/cbindgen"] ddtelemetry-ffi = ["dep:libdd-telemetry-ffi"] datadog-log-ffi = ["dep:libdd-log-ffi"] symbolizer = ["symbolizer-ffi"] -data-pipeline-ffi = ["dep:libdd-data-pipeline-ffi", "libdd-data-pipeline-ffi/catch_panic"] +data-pipeline-ffi = ["dep:libdd-data-pipeline-ffi"] # Enable zstd compression for the agentless trace intake sender. data-pipeline-compression = ["data-pipeline-ffi", "libdd-data-pipeline-ffi/compression"] crashtracker-ffi = ["dep:libdd-crashtracker-ffi"] @@ -31,11 +53,11 @@ crashtracker-collector = ["crashtracker-ffi", "libdd-crashtracker-ffi/collector" # Enables the use of this library to receiver crash-info from a suitable collector crashtracker-receiver = ["crashtracker-ffi", "libdd-crashtracker-ffi/receiver"] demangler = ["crashtracker-ffi", "libdd-crashtracker-ffi/demangler"] -datadog-library-config-ffi = ["dep:libdd-library-config-ffi", "libdd-library-config-ffi/catch_panic"] +datadog-library-config-ffi = ["dep:libdd-library-config-ffi"] ddcommon-ffi = ["dep:libdd-common-ffi"] ddsketch-ffi = ["dep:libdd-ddsketch-ffi"] datadog-ffe-ffi = ["dep:datadog-ffe-ffi"] -shared-runtime = ["dep:libdd-shared-runtime-ffi", "libdd-shared-runtime-ffi/catch_panic"] +shared-runtime = ["dep:libdd-shared-runtime-ffi"] otel-thread-ctx-ffi = ["dep:libdd-otel-thread-ctx-ffi"] regex-lite = ["libdd-common/regex-lite"] diff --git a/scripts/check_ffi_panic_containment.sh b/scripts/check_ffi_panic_containment.sh deleted file mode 100755 index 446624a355..0000000000 --- a/scripts/check_ffi_panic_containment.sh +++ /dev/null @@ -1,94 +0,0 @@ -#!/bin/bash -# Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ -# SPDX-License-Identifier: Apache-2.0 -# -# Asserts that every FFI sub-crate aggregated into libdd-profiling-ffi keeps -# its `catch_panic` feature enabled in the combined artifact. -# -# libdd-profiling-ffi pulls its sub-crates with `default-features = false`, and -# `catch_panic` is a *default* feature of the crates that have one. Dropping the -# defaults silently degrades `catch_panic!` to a bare call, so a Rust panic -# crosses the non-unwinding `extern "C"` boundary and aborts the host process. -# TODO: APMSP-3874 - evaluate if catch_panic should even be an optional feature. - -set -euo pipefail - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -ROOT_DIR="$(dirname "$SCRIPT_DIR")" - -cd "$ROOT_DIR" - -AGGREGATOR="libdd-profiling-ffi" - -METADATA=$(cargo metadata --no-deps --format-version 1) - -# has_catch_panic -# 0 if the workspace crate declares a `catch_panic` feature, 1 otherwise. -has_catch_panic() { - printf '%s' "$METADATA" | jq -e --arg name "$1" \ - '.packages[] | select(.name == $name) | .features | has("catch_panic")' > /dev/null 2>&1 -} - -# activating_feature -# The aggregator feature listing `dep:`, empty if there is none. -activating_feature() { - printf '%s' "$METADATA" | jq -r --arg agg "$AGGREGATOR" --arg dep "$1" \ - 'first(.packages[] | select(.name == $agg) | .features | to_entries[] - | select(any(.value[]; . == "dep:" + $dep)) | .key)' -} - -mapfile -t OPTIONAL_DEPS < <(printf '%s' "$METADATA" | jq -r --arg agg "$AGGREGATOR" \ - '.packages[] | select(.name == $agg) | .dependencies[] | select(.optional) | .name' | sort -u) - -if [ "${#OPTIONAL_DEPS[@]}" -eq 0 ]; then - echo "no optional dependencies found for $AGGREGATOR" >&2 - exit 2 -fi - -errors=0 -checked=0 -skipped=0 - -for dep in "${OPTIONAL_DEPS[@]}"; do - if ! has_catch_panic "$dep"; then - skipped=$((skipped + 1)) - continue - fi - - feature=$(activating_feature "$dep") - if [ -z "$feature" ]; then - echo "ERROR: no $AGGREGATOR feature activates optional dependency $dep" >&2 - exit 2 - fi - - checked=$((checked + 1)) - - output=$(cargo tree -p "$AGGREGATOR" --features "$feature" --edges features -i "$dep" 2>&1) || { - echo "ERROR: cargo tree failed for $AGGREGATOR --features $feature -i $dep:" >&2 - echo "$output" | sed 's/^/ /' >&2 - exit 2 - } - - if grep -qF "$dep feature \"catch_panic\"" <<<"$output"; then - echo "ok: $AGGREGATOR --features $feature keeps $dep/catch_panic" - else - echo "FAIL: $AGGREGATOR --features $feature does not enable $dep/catch_panic" - echo " A panic in $dep would abort the host process instead of returning an error." - echo " Fix: add \"$dep/catch_panic\" to the \"$feature\" feature in $AGGREGATOR/Cargo.toml" - echo "$output" | sed 's/^/ /' - errors=$((errors + 1)) - fi -done - -if [ "$checked" -eq 0 ]; then - echo "no aggregated crate declares a catch_panic feature -- discovery is broken" >&2 - exit 2 -fi - -if [ "$errors" -gt 0 ]; then - echo - echo "FFI panic containment check failed: $errors violation(s) across $checked edge(s)" - exit 1 -fi - -echo "FFI panic containment check passed for $checked edge(s) ($skipped crate(s) without the feature skipped)" From 3f81ab7383d138ba74df157534c66ed6be437384 Mon Sep 17 00:00:00 2001 From: Edmund Kump Date: Mon, 3 Aug 2026 17:34:53 -0400 Subject: [PATCH 6/7] improve test coverage by validating builder/Cargo.toml contains catch panic and that all catch panic enabled crates are in the catch panic list --- builder/src/builder.rs | 2 +- builder/src/features.rs | 105 ++++++++++++++++++++++++++++++++++------ 2 files changed, 92 insertions(+), 15 deletions(-) diff --git a/builder/src/builder.rs b/builder/src/builder.rs index bce396f102..7ef6472ab5 100644 --- a/builder/src/builder.rs +++ b/builder/src/builder.rs @@ -40,7 +40,7 @@ use crate::utils::{file_replace, project_root}; /// Ok(()) /// } /// } -/// let mut builder = Builder::new("source", "target", "arch", "features", "profile", "version"); +/// let mut builder = Builder::new("source", "target", "arch", "profile", "features", "version"); /// let core = Box::new(Core { /// version: builder.version.clone(), /// }); diff --git a/builder/src/features.rs b/builder/src/features.rs index b34738acd7..2bb355f5b8 100644 --- a/builder/src/features.rs +++ b/builder/src/features.rs @@ -117,6 +117,9 @@ pub fn profiling_features(selection: &Selection) -> Vec { #[cfg(test)] mod tests { use super::*; + use std::fs; + use std::path::{Path, PathBuf}; + use toml::Value; /// Every module-selecting flag, so the exhaustive test below cannot silently stop /// covering a field that someone adds to [`Selection`]. @@ -144,20 +147,6 @@ mod tests { selection } - #[test] - fn selecting_catch_panic_emits_the_feature() { - let selection = Selection { - catch_panic: true, - ..Default::default() - }; - assert!(profiling_features(&selection).contains(&"catch_panic".to_string())); - } - - #[test] - fn not_selecting_catch_panic_omits_the_feature() { - assert!(!profiling_features(&Selection::default()).contains(&"catch_panic".to_string())); - } - /// Containment must reach libdd-profiling-ffi for every module combination a project can /// ask for, not just the one our own release happens to build. This is the APMSP-3830 /// regression: the feature was there for some builds and quietly absent for others. @@ -278,4 +267,92 @@ mod tests { assert_eq!(sorted.len(), features.len(), "duplicates for {bits:#b}"); } } + + // The tests above only see a hand-built `Selection`. These two cover the ends of the + // chain: `catch_panic` in builder's `default`, and the fan-out in libdd-profiling-ffi. + // Neither is reachable from examples/ffi/trace_exporter.c. + + fn workspace_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("builder/ has a parent") + .to_path_buf() + } + + fn manifest(path: &Path) -> Value { + fs::read_to_string(path) + .unwrap_or_else(|e| panic!("reading {}: {e}", path.display())) + .parse() + .unwrap_or_else(|e| panic!("parsing {}: {e}", path.display())) + } + + fn declares_feature(manifest: &Value, name: &str) -> bool { + manifest + .get("features") + .and_then(Value::as_table) + .is_some_and(|features| features.contains_key(name)) + } + + /// Entries of a `[features]` list, empty when the feature is absent or has none. + fn feature<'a>(manifest: &'a Value, name: &str) -> Vec<&'a str> { + manifest + .get("features") + .and_then(Value::as_table) + .and_then(|features| features.get(name)) + .and_then(Value::as_array) + .map(|entries| entries.iter().filter_map(Value::as_str).collect()) + .unwrap_or_default() + } + + #[test] + fn builder_defaults_to_panic_containment() { + let builder = manifest(&workspace_root().join("builder/Cargo.toml")); + + assert!( + feature(&builder, "default").contains(&"catch_panic"), + "builder's default must include catch_panic, or the artifact aborts the host \ + process on a Rust panic" + ); + } + + #[test] + fn every_containment_capable_sub_crate_is_fanned_out() { + let root = workspace_root(); + let aggregator_dir = root.join("libdd-profiling-ffi"); + let aggregator = manifest(&aggregator_dir.join("Cargo.toml")); + let fan_out = feature(&aggregator, "catch_panic"); + + let dependencies = aggregator + .get("dependencies") + .and_then(Value::as_table) + .expect("libdd-profiling-ffi declares [dependencies]"); + + let mut found = 0; + for (name, spec) in dependencies { + if spec.get("optional").and_then(Value::as_bool) != Some(true) { + continue; + } + let Some(path) = spec.get("path").and_then(Value::as_str) else { + continue; + }; + let sub_crate = manifest(&aggregator_dir.join(path).join("Cargo.toml")); + if !declares_feature(&sub_crate, "catch_panic") { + continue; + } + + found += 1; + let entry = format!("{name}?/catch_panic"); + assert!( + fan_out.contains(&entry.as_str()), + "{name} has a catch_panic feature that libdd-profiling-ffi's catch_panic does \ + not enable; add \"{entry}\"" + ); + } + + assert!( + found > 0, + "no containment-capable optional dependency found, so this test is no longer \ + checking anything" + ); + } } From 8f71950cfc725a220ac8a5539c89626e9dcf8cbd Mon Sep 17 00:00:00 2001 From: Edmund Kump Date: Mon, 3 Aug 2026 17:41:04 -0400 Subject: [PATCH 7/7] clean up comments --- builder/src/features.rs | 49 +++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/builder/src/features.rs b/builder/src/features.rs index 2bb355f5b8..51cb5692e9 100644 --- a/builder/src/features.rs +++ b/builder/src/features.rs @@ -1,18 +1,13 @@ // Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -//! Mapping from `builder`'s own Cargo features onto the `libdd-profiling-ffi` feature -//! list used to produce the combined artifact. +//! Maps `builder`'s own Cargo features onto the `libdd-profiling-ffi` feature list used to +//! produce the combined artifact. //! -//! Expressed as data plus a pure function rather than a wall of `#[cfg]` attributes so -//! that every combination — including the ones no CI job ever builds — can be asserted -//! in unit tests (RFC 0016, option E). `builder/src/bin/release.rs` was one of the -//! `#[cfg]` hot spots that RFC calls out. +//! Data plus a pure function rather than `#[cfg]` attributes, so that combinations no CI job +//! builds can still be asserted in unit tests. -/// What a release build should contain. -/// -/// Downstream projects select these through `builder`'s Cargo features; see -/// `builder/Cargo.toml`. +/// What a release build should contain. Selected through `builder`'s Cargo features. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub struct Selection { pub telemetry: bool, @@ -26,10 +21,8 @@ pub struct Selection { pub ffe: bool, pub shared_runtime: bool, pub otel_thread_ctx: bool, - /// Contain panics at the FFI boundary, returning them to the caller as errors instead - /// of aborting the process. A `builder` default, so our own release artifacts get it; - /// a project that turns off `builder`'s default features and wants it back asks for - /// `catch_panic` like any other feature. + /// Return panics at the FFI boundary as errors instead of aborting the process. A + /// `builder` default; projects opting out of those defaults must ask for it explicitly. pub catch_panic: bool, } @@ -53,21 +46,18 @@ impl Selection { } } -/// `libdd-profiling-ffi` features the combined artifact always needs, whatever else is -/// selected. `ddcommon-ffi` is one of them: it is a *default* feature of that crate, and -/// [`profiling_features`] is passed alongside `--no-default-features`. +/// Needed whatever else is selected. `ddcommon-ffi` is here because it is a *default* of +/// `libdd-profiling-ffi`, which `--no-default-features` discards. const ALWAYS: &[&str] = &["cbindgen", "ddcommon-ffi"]; /// The complete `--features` list for `cargo rustc -p libdd-profiling-ffi`. /// -/// Callers **must** also pass `--no-default-features`. The list is exhaustive by design: -/// the artifact's feature set is exactly what this function returns, with nothing -/// inherited implicitly. Relying on inherited defaults is what let `catch_panic` go -/// missing from the shipped artifact in APMSP-3830 without any build failing. +/// Callers **must** also pass `--no-default-features`: the returned list is exhaustive, so +/// nothing is inherited. Inheriting defaults is how `catch_panic` went missing from the +/// shipped artifact in APMSP-3830 without any build failing. pub fn profiling_features(selection: &Selection) -> Vec { let mut features: Vec<&str> = ALWAYS.to_vec(); - // Kept first so a reader of any emitted command line sees the panic policy immediately. if selection.catch_panic { features.push("catch_panic"); } @@ -147,9 +137,8 @@ mod tests { selection } - /// Containment must reach libdd-profiling-ffi for every module combination a project can - /// ask for, not just the one our own release happens to build. This is the APMSP-3830 - /// regression: the feature was there for some builds and quietly absent for others. + /// APMSP-3830 was the feature being present for some builds and absent for others, so + /// cover every combination a project can ask for, not just the one we release. #[test] fn catch_panic_survives_every_module_combination() { for bits in 0..(1u32 << MODULE_SETTERS.len()) { @@ -174,10 +163,8 @@ mod tests { } } - /// Exhaustive struct literal on purpose: no `..Default::default()`. Adding a field to - /// [`Selection`] stops this compiling, which is the prompt to also give it a branch in - /// [`profiling_features`] and an entry in `MODULE_SETTERS`. Without this, a new field - /// could be silently ignored by the mapping and every other test here would still pass. + /// Exhaustive struct literal on purpose, no `..Default::default()`: adding a field to + /// [`Selection`] breaks this until it also gets a branch and a `MODULE_SETTERS` entry. #[test] fn all_fields_set_emits_every_module_feature() { let everything = Selection { @@ -219,8 +206,8 @@ mod tests { ); } - /// `--no-default-features` means nothing is inherited, so the defaults the artifact - /// still needs have to be listed explicitly. + /// Nothing is inherited under `--no-default-features`, so still-needed defaults must be + /// listed explicitly. #[test] fn always_lists_features_that_no_longer_come_from_defaults() { for bits in 0..(1u32 << MODULE_SETTERS.len()) {