refactor(otel-thread-ctx): use inline assembly instead of C shim#2197
refactor(otel-thread-ctx): use inline assembly instead of C shim#2197yannham wants to merge 11 commits into
Conversation
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>
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
| compile_object.args(["-O2", "-fPIC", "-fomit-frame-pointer", "-c"]); | ||
|
|
||
| #[cfg(target_arch = "x86_64")] | ||
| compile_object.arg("-mtls-dialect=gnu2"); |
There was a problem hiding this comment.
I fear this brings back some toolchain constraints (basically that -mtls-dialect=gnu2 is available), just moving them from the main crate to tests. I wonder if we should just eschew the test if the compiler doesn't support the GNU2 dialect? After all, what we need is that somewhere in CI there's at least one amd64 target where this tests is run (which is currently the case).
🔒 Cargo Deny Results📦
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| // x1 is guaranteed not to be clobbered by the call | ||
| "blr x2", | ||
| "add x0, x1, x0", |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Though it's surprising that the check for the exact sequence of instructions succeeded in the CI before 🤔
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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)"
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 744c7f1 | Docs | Datadog PR Page | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
pawelchcki
left a comment
pawelchcki
left a comment
There was a problem hiding this comment.
Drive By review
Looks good. I looked for some alternatives and this seems like a solid option
4 participants
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.