refactor(otel-thread-ctx): use inline assembly instead of C shim by yannham · Pull Request #2197 · DataDog/libdatadog

yannham · 2026-07-06T09:22:34Z

What does this PR do?

This revives #2129, which has been merged in a different branch than main. Uses inline assembly instead of a C shim to enable TLSDESC access from Rust.

Motivation

This get rids of all toolchain-related issues, at the cost of some inline ASM. See the original PR for more discussion.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d726f8d89d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-07-06T09:37:33Z

+            // x1 is guaranteed not to be clobbered by the call
+            "blr   x2",
+            "add   x0, x1, x0",


Re-read TP after the AArch64 TLSDESC resolver

On AArch64 this keeps the thread pointer in x1 across blr x2, but the TLSDESC resolver calling convention allows the resolver to clobber x1 (only registers other than x0, x1, x30, and flags must be saved). On loaders/resolvers that use that freedom, add x0, x1, x0 computes the TLS slot from a trashed register, so attach/update/detach can read or write an invalid address; read tpidr_el0 after the TLSDESC call or preserve the TP somewhere the resolver must not clobber.

Useful? React with 👍 / 👎.

Indeed, and what's more ARM ABI actually mandates the exact uses of register as well, and we should use x1 instead of x2. Swapping the two in dbb0eeb.

Though it's surprising that the check for the exact sequence of instructions succeeded in the CI before 🤔

Ok, so after investigation, the previous sequence was the exact one generated by a relatively oldish GCC version (the one we have on CI basically on Alpine and CentOS). The new one is the clang version, exactly. Some GCC versions also move the TP offset calculation around a bit.

For now I've rooted for the clang version, because Rust uses LLVM under the hood, it clobbers one less register, and respect the ABI described above byte per byte. However cc is often gcc in the test, so I've extended the test to make it a bit more lax and accommodate for different compilers.

But it starts to be a lot of code, just for testing something "static" (and just because it's annoying to ensure clang in the CI). I start to question the usefulness of this check: somehow it only has to be tested "once".

TBH I wouldn't go too much into "try to support all compilers in CI"; I would instead suggest going the "gold/known version" approach, e.g. something like:

In CI we only assert that "assembly -> matches some known hash". If you change the assembly, you need to update the "some known hash"

And then we have some script/test that can recalculate the hash; and next to it is a comment saying "To get the latest assembly hash, we ran this on some standard docker image (ubuntu 26.06 with clang XX)"

I like that idea, implemented in latest version. It doesn't remove so much code (some of it has just moved to the gen_tls_shim_hash executable), but the annoying part (compiling the C shim with the exact right toolchain) has been offloaded to this run-once-for-a-long-time-outside-of-tests executable to generate hash, which is indeed win for tests.

datadog-prod-us1-5 · 2026-07-06T09:39:04Z

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 61.20%
• Overall Coverage: 74.67% (-0.05%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 6b39e91 | Docs | Datadog PR Page | Give us feedback!}

dd-octo-sts · 2026-07-06T17:07:04Z

Artifact Size Benchmark Report

aarch64-alpine-linux-musl

Artifact	Baseline	Commit	Change
/aarch64-alpine-linux-musl/lib/libdatadog_profiling.so	7.88 MB	7.88 MB	0% (0 B) 👌
/aarch64-alpine-linux-musl/lib/libdatadog_profiling.a	86.33 MB	86.33 MB	0% (0 B) 👌

aarch64-unknown-linux-gnu

Artifact	Baseline	Commit	Change
/aarch64-unknown-linux-gnu/lib/libdatadog_profiling.a	97.54 MB	97.54 MB	0% (0 B) 👌
/aarch64-unknown-linux-gnu/lib/libdatadog_profiling.so	10.62 MB	10.62 MB	0% (0 B) 👌

libdatadog-x64-windows

Artifact	Baseline	Commit	Change
/libdatadog-x64-windows/debug/dynamic/datadog_profiling_ffi.dll	25.58 MB	25.58 MB	0% (0 B) 👌
/libdatadog-x64-windows/debug/dynamic/datadog_profiling_ffi.lib	89.18 KB	89.18 KB	0% (0 B) 👌
/libdatadog-x64-windows/debug/dynamic/datadog_profiling_ffi.pdb	185.53 MB	185.52 MB	-0% (-8.00 KB) 👌
/libdatadog-x64-windows/debug/static/datadog_profiling_ffi.lib	958.60 MB	958.60 MB	0% (0 B) 👌
/libdatadog-x64-windows/release/dynamic/datadog_profiling_ffi.dll	8.37 MB	8.37 MB	0% (0 B) 👌
/libdatadog-x64-windows/release/dynamic/datadog_profiling_ffi.lib	89.18 KB	89.18 KB	0% (0 B) 👌
/libdatadog-x64-windows/release/dynamic/datadog_profiling_ffi.pdb	24.76 MB	24.76 MB	0% (0 B) 👌
/libdatadog-x64-windows/release/static/datadog_profiling_ffi.lib	49.28 MB	49.28 MB	0% (0 B) 👌

libdatadog-x86-windows

Artifact	Baseline	Commit	Change
/libdatadog-x86-windows/debug/dynamic/datadog_profiling_ffi.dll	22.23 MB	22.23 MB	0% (0 B) 👌
/libdatadog-x86-windows/debug/dynamic/datadog_profiling_ffi.lib	90.58 KB	90.58 KB	0% (0 B) 👌
/libdatadog-x86-windows/debug/dynamic/datadog_profiling_ffi.pdb	189.98 MB	189.98 MB	0% (0 B) 👌
/libdatadog-x86-windows/debug/static/datadog_profiling_ffi.lib	946.54 MB	946.54 MB	0% (0 B) 👌
/libdatadog-x86-windows/release/dynamic/datadog_profiling_ffi.dll	6.47 MB	6.47 MB	0% (0 B) 👌
/libdatadog-x86-windows/release/dynamic/datadog_profiling_ffi.lib	90.58 KB	90.58 KB	0% (0 B) 👌
/libdatadog-x86-windows/release/dynamic/datadog_profiling_ffi.pdb	26.58 MB	26.58 MB	0% (0 B) 👌
/libdatadog-x86-windows/release/static/datadog_profiling_ffi.lib	46.89 MB	46.89 MB	0% (0 B) 👌

x86_64-alpine-linux-musl

Artifact	Baseline	Commit	Change
/x86_64-alpine-linux-musl/lib/libdatadog_profiling.a	77.08 MB	77.08 MB	0% (0 B) 👌
/x86_64-alpine-linux-musl/lib/libdatadog_profiling.so	8.83 MB	8.83 MB	0% (0 B) 👌

x86_64-unknown-linux-gnu

Artifact	Baseline	Commit	Change
/x86_64-unknown-linux-gnu/lib/libdatadog_profiling.a	92.50 MB	92.50 MB	0% (0 B) 👌
/x86_64-unknown-linux-gnu/lib/libdatadog_profiling.so	10.74 MB	10.74 MB	0% (0 B) 👌

pawelchcki

Drive By review

Looks good. I looked for some alternatives and this seems like a solid option

pr-commenter · 2026-07-16T16:58:08Z

Benchmarks

Comparison

Benchmark execution time: 2026-07-17 16:51:00

Comparing candidate commit 6b39e91 in PR branch yannham/thread-ctx-inline-asm with baseline commit 0c6e2a5 in branch main.

Found 0 performance improvements and 2 performance regressions! Performance is the same for 140 metrics, 0 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:profiles_dictionary/profile_string_inserts/threads/1

🟥 execution_time [+14.360µs; +16.472µs] or [+4.691%; +5.381%]
🟥 throughput [-171360.951op/s; -149449.646op/s] or [-5.122%; -4.467%]

Benchmark execution time: 2026-07-17 17:06:22

Comparing candidate commit 6b39e91 in PR branch yannham/thread-ctx-inline-asm with baseline commit 0c6e2a5 in branch main.

Found 3 performance improvements and 4 performance regressions! Performance is the same for 170 metrics, 10 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:alloc_free/sampled_system_fast_path/4096

🟥 execution_time [+6.890ns; +7.078ns] or [+7.135%; +7.330%]

scenario:alloc_free/sampled_system_slow_path/4096

🟥 execution_time [+16.649ns; +16.757ns] or [+10.917%; +10.988%]

scenario:alloc_free/system/4096

🟥 execution_time [+10.104ns; +10.271ns] or [+11.998%; +12.195%]

scenario:datadog_sample_span/name_pattern_rule_not_matching/wall_time

🟩 execution_time [-14.083ns; -13.910ns] or [-8.259%; -8.158%]

scenario:glob_matcher/ascii_wildcard_question_match/wall_time

🟩 execution_time [-21.641ns; -21.622ns] or [-36.982%; -36.950%]

scenario:glob_matcher/ascii_wildcard_star_match/wall_time

🟩 execution_time [-21.138ns; -21.118ns] or [-36.436%; -36.402%]

scenario:receiver_entry_point/report/2644

🟥 execution_time [+180.804µs; +191.838µs] or [+5.018%; +5.324%]

Candidate

Omitted due to size.

Baseline

Omitted due to size.

ivoanjo

👍 Looks great, no concerns (only my usual smattering of small suggestions), and thanks @cataphract for all the work in the previous #2129 .

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>

The libdd-otel-thread-ctx build script only validated the target OS/arch and panicked on unsupported Linux architectures. Move that into a compile_error! gated on cfg and drop the build script entirely. The linux module is now also gated on x86_64/aarch64 so an unsupported Linux arch produces a single clean error instead of a cascade from the module body. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…C shim all the time

Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>

yannham requested review from a team as code owners July 6, 2026 09:22

yannham commented Jul 6, 2026

View reviewed changes

Comment thread libdd-otel-thread-ctx-ffi/tests/elf_properties.rs Outdated

chatgpt-codex-connector Bot reviewed Jul 6, 2026

View reviewed changes

pawelchcki approved these changes Jul 6, 2026

View reviewed changes

yannham force-pushed the yannham/thread-ctx-inline-asm branch from 53e627a to 7976952 Compare July 16, 2026 16:01

yannham requested a review from ivoanjo July 16, 2026 16:02

yannham requested a review from a team as a code owner July 16, 2026 16:07

ivoanjo approved these changes Jul 17, 2026

View reviewed changes

cataphract and others added 18 commits July 17, 2026 16:11

More robust way to inline thread local var resolution

22b2d06

ensure otel_thread_ctx_v1 is aligned

415f048

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>

add comment and test for relaxation

7279882

Compare inline assembly to what's generated by the toolchain

c6696b1

address review comments

fd6ddb3

doc: explain why we don't need to clobber registers for call instruction

9f8225e

fix: use proper register following the official ABI

f1c3132

fix: instruction comparison test

72c7932

refactor: use one less register and match gcc sequence better

8136b6e

fix: relax the compiler test to work with both clang and gcc

f3ceebe

refactor: use hash-based integration test instead of recompiling the …

f60ab7b

…C shim all the time

style: remove empty comment line

371cfdf

style: formatting

09f166b

Update libdd-otel-thread-ctx/src/test_utils/tls_shim_window.rs

e60daa4

Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>

Update libdd-otel-thread-ctx/src/test_utils/tls_shim_window.rs

067fad3

Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>

doc: remove technical ASM from comments

b275fe5

doc: add more context for tls_shim_window helpers

2461573

yannham added 4 commits July 17, 2026 16:11

doc: mention that the x86 access sequence is also checked intest

d93c3f9

refactor: move tls-asm-related tests to otel-thread-ctx

323e95e

test: move from hash to raw byte sequence (tls sequence)

b97308a

chore: undo unrelated reformatting of libdd-profiling-heap-gotter

9d7d17a

yannham force-pushed the yannham/thread-ctx-inline-asm branch from 786ea19 to 9d7d17a Compare July 17, 2026 14:18

yannham added 2 commits July 17, 2026 17:16

ci: include non-default optional binary in the Dockerfile.buld

9ffdebf

ci: fix non-linux tests

6b39e91

gh-worker-dd-devflow-36fce6 Bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Jul 20, 2026

gh-worker-dd-mergequeue-cf854d Bot merged commit 69a1471 into main Jul 20, 2026
91 checks passed

gh-worker-dd-devflow-36fce6 Bot removed the mergequeue-status: in_progress label Jul 20, 2026

gh-worker-dd-mergequeue-cf854d Bot deleted the yannham/thread-ctx-inline-asm branch July 20, 2026 12:50

gh-worker-dd-devflow-36fce6 Bot added the mergequeue-status: done label Jul 20, 2026

Uh oh!

Conversation

yannham commented Jul 6, 2026

What does this PR do?

Motivation

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

yannham Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yannham Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

yannham Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivoanjo Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yannham Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

datadog-prod-us1-5 Bot commented Jul 6, 2026 • edited by datadog-official Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-sts Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Artifact Size Benchmark Report

Uh oh!

pawelchcki left a comment

Choose a reason for hiding this comment

Uh oh!

pr-commenter Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Comparison

Explanation

More details about the CI and significant changes

scenario:profiles_dictionary/profile_string_inserts/threads/1

Explanation

More details about the CI and significant changes

scenario:alloc_free/sampled_system_fast_path/4096

scenario:alloc_free/sampled_system_slow_path/4096

scenario:alloc_free/system/4096

scenario:datadog_sample_span/name_pattern_rule_not_matching/wall_time

scenario:glob_matcher/ascii_wildcard_question_match/wall_time

scenario:glob_matcher/ascii_wildcard_star_match/wall_time

scenario:receiver_entry_point/report/2644

Candidate

Baseline

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

yannham Jul 6, 2026 •

edited

Loading

ivoanjo Jul 7, 2026 •

edited

Loading

datadog-prod-us1-5 Bot commented Jul 6, 2026 •

edited by datadog-official Bot

Loading

dd-octo-sts Bot commented Jul 6, 2026 •

edited

Loading

pr-commenter Bot commented Jul 16, 2026 •

edited

Loading