fix(stringtheory): support big-endian platforms for MetaString - #2032
Conversation
This comment has been minimized.
This comment has been minimized.
Binary Size Analysis (Agent Data Plane)Baseline: f546aa0 · Comparison: 90a2aa6 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
Add a big-endian MetaString fallback backed by shared string storage while keeping the existing tagged-pointer implementation for little-endian targets. Cover the big-endian public API with an AIX integration test and verify the ADP build moves to the next platform constants blocker.
694cc1b to
064c9fa
Compare
Keep MetaString on the existing three-word union representation for both little- and big-endian 64-bit targets. The discriminant byte now follows the high byte of the third machine word, which keeps owned/static/interned/shared tagging consistent across endianness while reducing big-endian inline capacity to 16 contiguous bytes. This removes the separate big-endian fallback implementation and updates tests/docs to reflect the endian-specific inline capacity.
|
|
||
| // High-level invariant checks to ensure `stringtheory` isn't being used on an unsupported platform. | ||
| #[cfg(not(all(target_pointer_width = "64", target_endian = "little")))] | ||
| #[cfg(not(all(target_pointer_width = "64", any(target_endian = "little", target_endian = "big"))))] |
There was a problem hiding this comment.
any(little, big) seems extraneous, can you simplify
There was a problem hiding this comment.
[GPT-5] Simplified to #[cfg(not(target_pointer_width = "64"))]. Also added focused layout tests for the endian-specific tag byte, inline capacity, and contiguous inline storage boundary.
Add unit coverage for the endian-specific MetaString discriminant byte, inline capacity, and contiguous inline storage boundary. These tests run on the native little-endian target and compile under the s390x big-endian test target. Also simplify the platform invariant check per PR feedback since the compile error only enforces the 64-bit platform requirement.
Add focused tests for the off-by-one boundary between inline string data and the endian-specific tag byte. The tests assert that strings use every byte before the tag byte and reject one byte past it.
Introduce TOP_BYTE_INDEX to make the endian-dependent byte offset explicit. MetaString uses this offset to locate the top byte of the third machine word, where the union discriminant is stored.
|
|
||
| #[test] | ||
| fn inlined_string_uses_every_byte_before_tag_byte() { | ||
| let input = "a".repeat(INLINED_STR_TAG_INDEX); |
There was a problem hiding this comment.
Maybe use the length constant for clarity? (I recognize that index == length.)
There was a problem hiding this comment.
[GPT-5] Updated to use INLINED_STR_MAX_LEN in the boundary tests. The tests still validate the off-by-one behavior, but now read in terms of length rather than the underlying tag-byte index.
| #[test] | ||
| fn inlined_string_respects_endian_capacity() { | ||
| let max_len = "a".repeat(INLINED_STR_MAX_LEN); | ||
| let too_long = "a".repeat(INLINED_STR_MAX_LEN + 1); | ||
|
|
||
| assert!(MetaString::try_inline(&max_len).is_some()); | ||
| assert!(MetaString::try_inline(&too_long).is_none()); | ||
| } |
There was a problem hiding this comment.
Seems duplicative: we're already testing this above with the two individual inlining tests.
There was a problem hiding this comment.
[GPT-5] Removed this duplicate test. The two explicit boundary tests cover the max accepted length and the first rejected length.
| @@ -0,0 +1,55 @@ | |||
| #![cfg(target_endian = "big")] | |||
There was a problem hiding this comment.
It's ... unclear to me why this has to be a separate test. Are we somehow not testing the public API in the normal unit tests when run on a big-endian platform?
There was a problem hiding this comment.
Confirm these public methods are all covered in the module unit tests and if so delete this file
There was a problem hiding this comment.
[GPT-5] Confirmed the normal unit tests cover the public API on big-endian now that the implementation is unified. I moved the remaining coverage from this integration test into src/lib.rs (ordering, protobuf Chars conversions, and CheapMetaString::try_cheap_clone) and deleted this cfg-gated test file.
| #[test] | ||
| fn endian_layout_constants_match_discriminant_strategy() { | ||
| let expected_top_byte_index = if cfg!(target_endian = "little") { | ||
| std::mem::size_of::<usize>() - 1 | ||
| } else { | ||
| 0 | ||
| }; | ||
| let expected_tag_index = std::mem::size_of::<usize>() * 2 + expected_top_byte_index; | ||
|
|
||
| assert_eq!(TOP_BYTE_INDEX, expected_top_byte_index); | ||
| assert_eq!(INLINED_STR_TAG_INDEX, expected_tag_index); | ||
| assert_eq!(INLINED_STR_MAX_LEN, expected_tag_index); | ||
| } |
There was a problem hiding this comment.
I think we can drop this? We're just restating the const definitions, and other tests should break if we mess those up somehow.
There was a problem hiding this comment.
[GPT-5] Removed this test. It was restating the constant definitions rather than testing behavior; the inline capacity and raw discriminant tests cover the behavior those constants drive.
Address review feedback by removing tests that only restated constant definitions or duplicated the inline boundary checks. Keep the off-by-one coverage focused on the maximum inline length and the first rejected length.
Move the remaining public API coverage from the big-endian integration test into the crate unit tests, which run on big-endian targets now that the optimized implementation is unified. This removes the redundant cfg-gated integration test file.
MetaString
## Summary Adds AIX platform defaults to `datadog-agent-commons` so platform settings resolve when building ADP on AIX. This is one of the Saluki-side blockers from DADP-142 / the AIX Confluence notes. After getting past `process-memory` and `stringtheory`, the AIX ADP build stopped in `datadog-agent-commons` because `platform/mod.rs` referenced platform constants/functions that are not defined for AIX. ## Key changes - Add `platform/aix_impl.rs` with AIX config, log, and syslog defaults. - Wire the AIX platform module into `platform/mod.rs`. - Add an AIX-only unit test for the expected platform paths. ## AIX defaults The AIX defaults match the standard Datadog Agent paths used in the AIX build notes: - Config directory: `/etc/datadog-agent` - Log directory: `/var/log/datadog` - Local syslog URI: `unixgram:///dev/log` ## Continuation context This PR is part of a three-PR Saluki-side AIX enablement stack: 1. #2031: `process-memory` AIX RSS support. 2. #2032: `stringtheory` big-endian `MetaString` fallback. 3. This PR: `datadog-agent-commons` AIX platform settings. A separate upstream/fork fix is still needed for `aws-lc-rs`; during validation I used the local checkout of Travis's fork/branch on the AIX host. That temporary override is **not** included in this PR. Temporary AIX validation setup used on `soaix499`: ```sh export PATH=/opt/freeware/lib/RustSDK/1.92/bin:/opt/freeware/bin:/usr/sbin:/usr/bin:/bin export CC=/opt/freeware/bin/gcc-8 export CXX=/opt/freeware/bin/g++-8 export AR=/usr/bin/ar export RANLIB=/usr/bin/ranlib export CARGO_HOME=$HOME/dd/cargo-home export CARGO_TARGET_DIR=$HOME/dd/saluki-target export CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse ``` ## Test plan - RED on AIX before implementation: - Added the AIX-specific platform test. - With the proper AIX build env, `cargo test -p datadog-agent-commons platform::tests::aix_uses_standard_agent_paths --no-run` failed because AIX platform constants/functions were missing. - Local: - `cargo fmt --package datadog-agent-commons --all` - `cargo check -p datadog-agent-commons` - `cargo test -p datadog-agent-commons platform::tests` - `git diff --check` - AIX 7.3 (`soaix499`): - `cargo test -p datadog-agent-commons platform::tests::aix_uses_standard_agent_paths` - Result: `aix_uses_standard_agent_paths ... ok` - AIX 7.3 (`soaix499`) with temporary local `aws-lc-rs` patch plus #2031 and #2032 synced: - `cargo build -p agent-data-plane` - Result: completed successfully in debug profile and produced `/home/zkvhn/dd/saluki-target/debug/agent-data-plane`. - Binary identified by `file` as `64-bit XCOFF executable or object module not stripped`. ## Notes - This PR does not include the temporary local `aws-lc-rs` patch used for AIX validation. - I committed with `--no-verify` because the local hook/check path attempted broad workspace/nightly checks and was killed by the local machine while unrelated to this targeted crate change. The targeted commands above were run separately. Co-authored-by: travis.thieman <travis.thieman@datadoghq.com>
## Summary Adds AIX support to the `process-memory` crate by reading `/proc/<pid>/psinfo` and extracting the resident set size from the AIX `psinfo_t` layout. This is one of the Saluki-side blockers from DADP-142 / the AIX Confluence notes. With only the temporary `aws-lc-rs` AIX branch applied, the AIX ADP build previously stopped at `process-memory` because the crate rejected all non-Linux/macOS/Windows targets. ## Key changes - Add an AIX `Querier` backend for resident set size queries. - Include AIX in the crate's supported target list. - Parse `psinfo_t.pr_rssize` as a native-endian `uint64_t` value in KiB units, then convert to bytes with overflow checks. - Add AIX parser coverage for successful parsing, zero RSS, truncated `psinfo`, and overflow. - Add AIX-only integration coverage that compares `Querier` against the raw `/proc/<pid>/psinfo` RSS field. - Keep the `dhat` allocation test active on non-AIX targets and make the harness-free test binary a no-op on AIX, where `dhat` is intentionally not enabled. ## AIX implementation notes - AIX on `soaix499` does not expose `/proc/self/psinfo`; it exposes `/proc/<pid>/psinfo`. - IBM's AIX `/proc` documentation states that `/proc` files provide 64-bit mode-invariant data to observers and that future structure growth appends fields. - IBM documents `psinfo_t.pr_rssize` as `uint64_t pr_rssize`, resident set size in KiB (1024) units. - I verified the AIX 7.3 `psinfo_t` layout on `soaix499` with a small C program using `<sys/procfs.h>`: - `sizeof(psinfo_t) = 448` - `offsetof(psinfo_t, pr_size) = 96` - `offsetof(psinfo_t, pr_rssize) = 104` - `offsetof(psinfo_t, pr_start) = 112` - Manual magnitude validation on `soaix499` showed `Querier`, raw `pr_rssize * 1024`, and `ps v <pid>` report the same RSS magnitude. Treating `pr_rssize` as pages would report roughly 4x too high on the 4 KiB page-size test host. - The implementation is fail-closed: open/read/truncation/overflow failures return `None` rather than panicking. ## Continuation context This PR is part of a three-PR Saluki-side AIX enablement stack: 1. This PR: `process-memory` AIX RSS support. 2. #2032: `stringtheory` big-endian `MetaString` fallback. 3. #2034: `datadog-agent-commons` AIX platform settings. A separate upstream/fork fix is still needed for `aws-lc-rs`; during validation I used the local checkout of Travis's fork/branch on the AIX host. That temporary override is **not** included in this PR. ## Test plan Local: - `make fmt` - `cargo check -p process-memory` - `cargo check -p process-memory --tests` - `cargo test -p process-memory` - Direct parser test compile/run for `lib/process-memory/src/aix.rs`. AIX 7.3 (`soaix499`): - `cargo test -p process-memory` - Result: - AIX unit tests: `4 passed` - AIX integration tests: `1 passed` - `tests/no_runtime_allocations.rs` runs successfully as a no-op on AIX - Doc tests: `0 failed` - Manual RSS magnitude check: - `Querier` and raw `pr_rssize * 1024` reported about `5.9-6.1 MiB` for the test process. - `ps v <pid>` reported the same RSS magnitude. - Treating `pr_rssize` as a page count would have reported about `23.7-24.4 MiB`. ## Notes - This PR does not include the temporary local `aws-lc-rs` patch used for broader AIX validation. - The latest commits were made with the normal pre-commit hook path; the hook completed successfully, including formatting, clippy, license/advisory checks, prose checks, and API docs build. Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com>
## Human Summary Adds a script and Makefile target to build a release ADP binary on AIX. This comes with two intentional divergences from how we build on the other platforms: - We can't use `cargo auditable` because it does not support XCOFF. Clanker tried to get it to work with some manual patching but wasn't able to. Seems like this will be a more significant contribution to upstream if we want this to work. - We can't use link-time optimization (LTO) because it doesn't seem to work on AIX at all, even for a hello-world crate. This is actually called out in IBM's docs [here](https://www.ibm.com/docs/en/osfroa/1.90.0?topic=started-introducing-rustc-compiler). Neither of these should be showstoppers, but please comment if they are. The plan will be to call this script from within the Agent's `packaging/aix` machinery when we do AIX builds. Working on that follow-up now. ## Summary - Adds `make build-adp-aix` for native AIX ADP builds using the IBM Rust SDK and AIX Toolbox GCC toolchain. - Adds `ci/tooling/build-adp-aix.sh` to record the AIX build environment and run the native Cargo build. - Adds an `aix-optimized-release` Cargo profile that keeps the optimized-release binary settings that work on AIX while disabling LTO. - Checks the major toolchain versions for cargo/rustc/gcc/g++ so AIX builds fail loudly if the toolchain drifts unexpectedly. ## Notes - The AIX enablement and AWS-LC dependency changes from #2031, #2032, #2034, #2060, and #2082 are now on `main`; this PR is rebased on top of them and only carries the build helper/profile changes. - `aws-lc-sys` is built from the bumped dependency without patching Cargo's registry cache. - On the AIX test host, `gcc-8` fails in an AWS-LC compiler feature probe; `/opt/freeware/bin/gcc` (`gcc 13.3`) succeeds, so the helper defaults to that compiler. - AIX currently does not use `cargo auditable`: cargo-auditable emits ELF/generic Unix linker inputs and flags that AIX's XCOFF linker rejects. - AIX currently uses `aix-optimized-release` instead of `optimized-release`: the IBM Rust SDK 1.92 fails LTO even for a tiny hello-world crate with `failed to get bitcode from object file for LTO (Can't find section .ipa)`. - The helper still uses the same metadata inputs that affect the ADP binary (`APP_FULL_NAME`, `APP_SHORT_NAME`, `APP_IDENTIFIER`, `APP_VERSION`, `APP_GIT_HASH`, `APP_BUILD_TIME`, `APP_DEV_BUILD`) and passes `BUILD_FEATURES` through to Cargo. - Toolchain version prefixes are overrideable through `ADP_AIX_EXPECTED_*_PREFIX` variables when an intentional toolchain update occurs. ## Test Plan - [x] `bash -n ci/tooling/build-adp-aix.sh` - [x] `ADP_AIX_BUILD_DRY_RUN=true make build-adp-aix` - [x] `cargo metadata --no-deps --format-version 1` - [x] AIX: `CARGO_HOME=/opt/cargo-home-aix-adp-pr-stack CARGO_TARGET_DIR=/opt/saluki-aix-adp-pr-stack-target make build-adp-aix` - [x] AIX: `/opt/saluki-aix-adp-pr-stack-target/aix-optimized-release/agent-data-plane --help` - [x] AIX: `/opt/saluki-aix-adp-pr-stack-target/aix-optimized-release/agent-data-plane version` - [x] Pre-commit hook: `fmt`, `check-clippy`, `check-licenses`, `check-deny`, `check-docs`, `generate-api-docs` AIX toolchain observed: - `cargo 1.92.0 (IBM Open SDK for Rust on AIX 1.92.0.0)` - `rustc 1.92.0 (IBM Open SDK for Rust on AIX 1.92.0.0)` - `gcc (GCC) 13.3.0` - `g++ (GCC) 13.3.0` AIX build result: - `Finished aix-optimized-release profile [optimized + debuginfo] target(s) in 26m 22s` - Binary: `64-bit XCOFF executable or object module not stripped` Co-authored-by: travis.thieman <travis.thieman@datadoghq.com>
Summary
Adds big-endian support to
stringtheory::MetaStringwhile keeping the existing optimized three-word union representation.This is one of the Saluki-side blockers from DADP-142 / the AIX Confluence notes. After getting past
process-memory, the AIX ADP build stopped atstringtheorybecause the previousMetaStringimplementation explicitly required 64-bit little-endian platforms.Key changes
MetaStringon the existing compact three-word representation for both little- and big-endian 64-bit targets.23, preserving the existing layout and 23-byte inline capacity.16, the high byte of the third machine word, with a 16-byte inline capacity so inline string bytes remain contiguous.usizeacross endianness.lib/stringtheory/src/lib.rs.Design notes
MetaStringlayout stores all variants in three machine words and determines the active variant from a single tag byte.usizeis instead the first byte of the third machine word, so the tag byte index must change.Deref<Target = str>can still return a borrowed&strwithout allocating.From<String>now uses the same owned-string variant everywhere.Continuation context
This PR is part of a three-PR Saluki-side AIX enablement stack:
process-memoryAIX RSS support.stringtheorybig-endianMetaStringsupport.datadog-agent-commonsAIX platform settings.A separate upstream/fork fix is still needed for
aws-lc-rs; during validation I used the local checkout of Travis's fork/branch on the AIX host. That temporary override is not included in this PR.Temporary AIX validation setup used on
soaix499:Test plan
cargo test -p stringtheory --test big_endian --no-runfailed at the existing little-endian compile gate.make fmtcargo nextest run -p stringtheorycargo test -p stringtheory inlined_string_uses_every_byte_before_tag_byte -- --exactcargo test -p stringtheory inlined_string_rejects_one_byte_past_tag_byte -- --exactcargo check --workspacecargo check --workspace --testsmake check-denygit diff --checkrustup target add s390x-unknown-linux-gnucargo check -p stringtheory --target s390x-unknown-linux-gnucargo check -p stringtheory --tests --target s390x-unknown-linux-gnusoaix499) on latest PR head90a2aa6560archived to~/dd/saluki-pr2032-latest:cargo test -p stringtheory --lib --ignore-rust-version45 passed; 0 failed, including the endian-specific layout and public API unit tests.soaix499) with temporary localaws-lc-rspatch and the process-memory fix synced:cargo build -p agent-data-planegot paststringtheoryand failed at the next known blocker, AIX platform constants indatadog-agent-commons.Notes
aws-lc-rspatch used for AIX validation.