support for 32bit environenments like xtensa, mips ..#533
Conversation
|
Welcome @gb6881! It looks like this is your first PR to tikv/rust-prometheus 🎉 |
Signed-off-by: Georg Buschbeck <gb@viingx.com>
…rdering - proto/proto_model.rs: regenerate with rust-protobuf 2.28.0 (was 2.2.5), no logic changes - atomic64.rs, histogram.rs, timer.rs: clean up whitespace and cfg-gated use blocks left over from the 32-bit/xtensa/mips portable_atomic support (12958bb) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
📝 WalkthroughWalkthroughThis PR adds a target-specific ChangesPortable-atomic backend selection
Regenerated proto_model.rs
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)Not applicable — changes are conditional-compilation dependency selection and generated protobuf code updates without new multi-component control flow. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 35-37: Document the Xtensa portable-atomic dependency by adding a
short note near the target-specific dependency block in Cargo.toml or in the
README, explaining that Xtensa builds use portable-atomic and require
critical-section or unsafe-assume-single-core for CAS paths. Reference the
existing target dependency stanza for target.'cfg(any(target_arch = "powerpc",
target_arch = "mips", target_arch = "xtensa"))'.dependencies so the guidance is
easy to find for builds like ESP32-S3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 851f894c-dadf-4d4e-9dd9-cb216ce476aa
📒 Files selected for processing (5)
Cargo.tomlproto/proto_model.rssrc/atomic64.rssrc/histogram.rssrc/timer.rs
| [target.'cfg(any(target_arch = "powerpc", target_arch = "mips", target_arch = "xtensa"))'.dependencies] | ||
| portable-atomic = "1.10.0" | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does portable-atomic require the critical-section feature to compile compare_exchange on Xtensa targets, and is unsafe-assume-single-core safe for dual-core ESP32-S3?
💡 Result:
The portable-atomic crate does not strictly require the critical-section feature to compile compare_exchange on Xtensa targets; however, it requires either the critical-section feature or the unsafe-assume-single-core feature (or portable_atomic_unsafe_assume_single_core cfg) to provide atomic CAS functionality where it is not natively available in the standard library [1][2][3]. Regarding your second question, it is unsafe to use unsafe-assume-single-core on a dual-core ESP32-S3 [3]. The unsafe-assume-single-core feature is strictly for single-core targets [3][4]. Enabling this feature on a multi-core system is unsound because the mechanism used to provide atomicity—disabling interrupts—only affects the current core and does not provide atomicity across multiple cores, leading to potential race conditions [3][5]. For multi-core environments like the ESP32-S3, you should use the critical-section feature, which allows you to provide a cross-core-aware implementation (e.g., using a global spinlock) to ensure atomicity across all cores [1][2][3].
Citations:
- 1: https://docs.rs/portable-atomic/latest/portable_atomic/
- 2: https://github.com/taiki-e/portable-atomic/blob/HEAD/src/imp/interrupt/README.md
- 3: https://docs.rs/portable-atomic/latest/portable_atomic/index.html
- 4: https://github.com/taiki-e/portable-atomic/
- 5: https://docs.rs/crate/portable-atomic/latest/source/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Cargo.toml context ==\n'
nl -ba Cargo.toml | sed -n '1,80p'
printf '\n== portable-atomic references ==\n'
rg -n "portable-atomic|portable_atomic|compare_exchange|unsafe-assume-single-core|critical-section" -S .
printf '\n== target-specific dependency section ==\n'
python3 - <<'PY'
from pathlib import Path
text = Path("Cargo.toml").read_text()
for i, line in enumerate(text.splitlines(), 1):
if "portable-atomic" in line or "target.'cfg(any(target_arch = \"powerpc\", target_arch = \"mips\", target_arch = \"xtensa\"))'" in line:
print(f"{i}: {line}")
PYRepository: tikv/rust-prometheus
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Cargo.toml excerpt ==\n'
sed -n '1,90p' Cargo.toml | cat -n
printf '\n== searchable references ==\n'
rg -n -S "portable-atomic|portable_atomic|critical-section|unsafe-assume-single-core|xtensa|ESP32-S3" .
printf '\n== target-specific dependency lines ==\n'
python3 - <<'PY'
from pathlib import Path
text = Path("Cargo.toml").read_text().splitlines()
for i, line in enumerate(text, 1):
if "portable-atomic" in line or "target.'cfg(any(target_arch = \"powerpc\", target_arch = \"mips\", target_arch = \"xtensa\"))'" in line:
print(f"{i}: {line}")
PYRepository: tikv/rust-prometheus
Length of output: 3965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/atomic64.rs ==\n'
sed -n '1,220p' src/atomic64.rs | cat -n
printf '\n== src/histogram.rs ==\n'
sed -n '1,220p' src/histogram.rs | cat -n
printf '\n== src/timer.rs ==\n'
sed -n '1,220p' src/timer.rs | cat -n
printf '\n== compare_exchange / fetch_update usage ==\n'
rg -n -S "compare_exchange|compare_exchange_weak|fetch_update|swap\\(" srcRepository: tikv/rust-prometheus
Length of output: 19623
Document the Xtensa portable-atomic requirement. Xtensa builds use portable_atomic here, and the CAS paths need critical-section (or unsafe-assume-single-core on truly single-core chips). A short note in the manifest or README would avoid confusing build failures for targets like ESP32-S3.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 35 - 37, Document the Xtensa portable-atomic
dependency by adding a short note near the target-specific dependency block in
Cargo.toml or in the README, explaining that Xtensa builds use portable-atomic
and require critical-section or unsafe-assume-single-core for CAS paths.
Reference the existing target dependency stanza for target.'cfg(any(target_arch
= "powerpc", target_arch = "mips", target_arch = "xtensa"))'.dependencies so the
guidance is easy to find for builds like ESP32-S3.
1 participant
Hi
i wanted to use this together with opentelemetry-prometheus on my esp32s3, but there i don't have Atomic[UI]64,
For Opentelemetry there is a already a patch iin the wild, doing this here,
maybe you're interesseted.
Thanks,
Georg
Summary by CodeRabbit
New Features
Bug Fixes